diff options
Diffstat (limited to 'doc/developer')
| -rw-r--r-- | doc/developer/fpm.rst | 103 | ||||
| -rw-r--r-- | doc/developer/index.rst | 1 | ||||
| -rw-r--r-- | doc/developer/logging.rst | 190 | ||||
| -rw-r--r-- | doc/developer/ospf-sr.rst | 2 | ||||
| -rw-r--r-- | doc/developer/topotests.rst | 3 | ||||
| -rw-r--r-- | doc/developer/workflow.rst | 77 | ||||
| -rw-r--r-- | doc/developer/zebra.rst | 21 |
7 files changed, 360 insertions, 37 deletions
diff --git a/doc/developer/fpm.rst b/doc/developer/fpm.rst new file mode 100644 index 0000000000..9849869133 --- /dev/null +++ b/doc/developer/fpm.rst @@ -0,0 +1,103 @@ +FPM +=== + +FPM stands for Forwarding Plane Manager and it's a module for use with Zebra. + +The encapsulation header for the messages exchanged with the FPM is +defined by the file :file:`fpm/fpm.h` in the frr tree. The routes +themselves are encoded in Netlink or protobuf format, with Netlink +being the default. + +Netlink is standard format for encoding messages to talk with kernel space +in Linux and it is also the name of the socket type used by it. +The FPM netlink usage differs from Linux's in: + +- Linux netlink sockets use datagrams in a multicast fashion, FPM uses + as a stream and it is unicast. +- FPM netlink messages might have more or less information than a normal + Linux netlink socket message (example: RTM_NEWROUTE might add an extra + route attribute to signalize VxLAN encapsulation). + +Protobuf is one of a number of new serialization formats wherein the +message schema is expressed in a purpose-built language. Code for +encoding/decoding to/from the wire format is generated from the +schema. Protobuf messages can be extended easily while maintaining +backward-compatibility with older code. Protobuf has the following +advantages over Netlink: + +- Code for serialization/deserialization is generated automatically. This + reduces the likelihood of bugs, allows third-party programs to be integrated + quickly, and makes it easy to add fields. +- The message format is not tied to an OS (Linux), and can be evolved + independently. + +.. note:: + + Currently there are two FPM modules in ``zebra``: + + * ``fpm`` + * ``dplane_fpm_nl`` + +fpm +^^^ + +The first FPM implementation that was built using hooks in ``zebra`` route +handling functions. It uses its own netlink/protobuf encoding functions to +translate ``zebra`` route data structures into formatted binary data. + + +dplane_fpm_nl +^^^^^^^^^^^^^ + +The newer FPM implementation that was built using ``zebra``'s data plane +framework as a plugin. It only supports netlink and it shares ``zebra``'s +netlink functions to translate route event snapshots into formatted binary +data. + + +Protocol Specification +---------------------- + +FPM (in any mode) uses a TCP connection to talk with external applications. +It operates as TCP client and uses the CLI configured address/port to connect +to the FPM server (defaults to port ``2620``). + +FPM frames all data with a header to help the external reader figure how +many bytes it has to read in order to read the full message (this helps +simulates datagrams like in the original netlink Linux kernel usage). + +Frame header: + +:: + + 0 1 2 3 + 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 + +---------------+---------------+-------------------------------+ + | Version | Message type | Message length | + +---------------+---------------+-------------------------------+ + | Data... | + +---------------------------------------------------------------+ + + +Version +^^^^^^^ + +Currently there is only one version, so it should be always ``1``. + + +Message Type +^^^^^^^^^^^^ + +Defines what underlining protocol we are using: netlink (``1``) or protobuf (``2``). + + +Message Length +^^^^^^^^^^^^^^ + +Amount of data in this frame in network byte order. + + +Data +^^^^ + +The netlink or protobuf message payload. diff --git a/doc/developer/index.rst b/doc/developer/index.rst index 3a33d9a5ec..26b590c876 100644 --- a/doc/developer/index.rst +++ b/doc/developer/index.rst @@ -11,6 +11,7 @@ FRRouting Developer's Guide library testing bgpd + fpm ospf zebra vtysh diff --git a/doc/developer/logging.rst b/doc/developer/logging.rst index db577c9216..0430ad72a3 100644 --- a/doc/developer/logging.rst +++ b/doc/developer/logging.rst @@ -1,7 +1,7 @@ .. _logging: -Developer's Guide to Logging -============================ +Logging +======= One of the most frequent decisions to make while writing code for FRR is what to log, what level to log it at, and when to log it. Here is a list of @@ -116,8 +116,11 @@ AS-Safety while AS-Safe) * extensions are only AS-Safe if their printer is AS-Safe +Log levels +---------- + Errors and warnings -------------------- +^^^^^^^^^^^^^^^^^^^ If it is something that the user will want to look at and maybe do something, it is either an **error** or a **warning**. @@ -163,7 +166,7 @@ Examples for errors: Informational messages ----------------------- +^^^^^^^^^^^^^^^^^^^^^^ Anything that provides introspection to the user during normal operation is an **info** message. @@ -202,7 +205,7 @@ Examples: Debug messages and asserts --------------------------- +^^^^^^^^^^^^^^^^^^^^^^^^^^ Everything that is only interesting on-demand, or only while developing, is a **debug** message. It might be interesting to the user for a @@ -239,3 +242,180 @@ Examples: * some field that is absolutely needed is :code:`NULL` * any other kind of data structure corruption that will cause the daemon to crash sooner or later, one way or another + +Thread-local buffering +---------------------- + +The core logging code in :file:`lib/zlog.c` allows setting up per-thread log +message buffers in order to improve logging performance. The following rules +apply for this buffering: + +* Only messages of priority *DEBUG* or *INFO* are buffered. +* Any higher-priority message causes the thread's entire buffer to be flushed, + thus message ordering is preserved on a per-thread level. +* There is no guarantee on ordering between different threads; in most cases + this is arbitrary to begin with since the threads essentially race each + other in printing log messages. If an order is established with some + synchronization primitive, add calls to :c:func:`zlog_tls_buffer_flush()`. +* The buffers are only ever accessed by the thread they are created by. This + means no locking is necessary. + +Both the main/default thread and additional threads created by +:c:func:`frr_pthread_new()` with the default :c:func:`frr_run()` handler will +initialize thread-local buffering and call :c:func:`zlog_tls_buffer_flush()` +when idle. + +If some piece of code runs for an extended period, it may be useful to insert +calls to :c:func:`zlog_tls_buffer_flush()` in appropriate places: + +.. c:function:: void zlog_tls_buffer_flush(void) + + Write out any pending log messages that the calling thread may have in its + buffer. This function is safe to call regardless of the per-thread log + buffer being set up / in use or not. + +When working with threads that do not use the :c:type:`struct thread_master` +event loop, per-thread buffers can be managed with: + +.. c:function:: void zlog_tls_buffer_init(void) + + Set up thread-local buffering for log messages. This function may be + called repeatedly without adverse effects, but remember to call + :c:func:`zlog_tls_buffer_fini()` at thread exit. + + .. warning:: + + If this function is called, but :c:func:`zlog_tls_buffer_flush()` is + not used, log message output will lag behind since messages will only be + written out when the buffer is full. + + Exiting the thread without calling :c:func:`zlog_tls_buffer_fini()` + will cause buffered log messages to be lost. + +.. c:function:: void zlog_tls_buffer_fini(void) + + Flush pending messages and tear down thread-local log message buffering. + This function may be called repeatedly regardless of whether + :c:func:`zlog_tls_buffer_init()` was ever called. + +Log targets +----------- + +The actual logging subsystem (in :file:`lib/zlog.c`) is heavily separated +from the actual log writers. It uses an atomic linked-list (`zlog_targets`) +with RCU to maintain the log targets to be called. This list is intended to +function as "backend" only, it **is not used for configuration**. + +Logging targets provide their configuration layer on top of this and maintain +their own capability to enumerate and store their configuration. Some targets +(e.g. syslog) are inherently single instance and just stuff their config in +global variables. Others (e.g. file/fd output) are multi-instance capable. +There is another layer boundary here between these and the VTY configuration +that they use. + +Basic internals +^^^^^^^^^^^^^^^ + +.. c:type:: struct zlog_target + + This struct needs to be filled in by any log target and then passed to + :c:func:`zlog_target_replace()`. After it has been registered, + **RCU semantics apply**. Most changes to associated data should make a + copy, change that, and then replace the entire struct. + + Additional per-target data should be "appended" by embedding this struct + into a larger one, for use with `containerof()`, and + :c:func:`zlog_target_clone()` and :c:func:`zlog_target_free()` should be + used to allocate/free the entire container struct. + + Do not use this structure to maintain configuration. It should only + contain (a copy of) the data needed to perform the actual logging. For + example, the syslog target uses this: + + .. code-block:: c + + struct zlt_syslog { + struct zlog_target zt; + int syslog_facility; + }; + + static void zlog_syslog(struct zlog_target *zt, struct zlog_msg *msgs[], size_t nmsgs) + { + struct zlt_syslog *zte = container_of(zt, struct zlt_syslog, zt); + size_t i; + + for (i = 0; i < nmsgs; i++) + if (zlog_msg_prio(msgs[i]) <= zt->prio_min) + syslog(zlog_msg_prio(msgs[i]) | zte->syslog_facility, "%s", + zlog_msg_text(msgs[i], NULL)); + } + + +.. c:function:: struct zlog_target *zlog_target_clone(struct memtype *mt, struct zlog_target *oldzt, size_t size) + + Allocates a logging target struct. Note that the ``oldzt`` argument may be + ``NULL`` to allocate a "from scratch". If ``oldzt`` is not ``NULL``, the + generic bits in :c:type:`struct zlog_target` are copied. **Target specific + bits are not copied.** + +.. c:function:: struct zlog_target *zlog_target_replace(struct zlog_target *oldzt, struct zlog_target *newzt) + + Adds, replaces or deletes a logging target (either ``oldzt`` or ``newzt`` may be ``NULL``.) + + Returns ``oldzt`` for freeing. The target remains possibly in use by + other threads until the RCU cycle ends. This implies you cannot release + resources (e.g. memory, file descriptors) immediately. + + The replace operation is not atomic; for a brief period it is possible that + messages are delivered on both ``oldzt`` and ``newzt``. + + .. warning:: + + ``oldzt`` must remain **functional** until the RCU cycle ends. + +.. c:function:: void zlog_target_free(struct memtype *mt, struct zlog_target *zt) + + Counterpart to :c:func:`zlog_target_clone()`, frees a target (using RCU.) + +.. c:member:: void (*zlog_target.logfn)(struct zlog_target *zt, struct zlog_msg *msgs[], size_t nmsg) + + Called on a target to deliver "normal" logging messages. ``msgs`` is an + array of opaque structs containing the actual message. Use ``zlog_msg_*`` + functions to access message data (this is done to allow some optimizations, + e.g. lazy formatting the message text and timestamp as needed.) + + .. note:: + + ``logfn()`` must check each individual message's priority value against + the configured ``prio_min``. While the ``prio_min`` field is common to + all targets and used by the core logging code to early-drop unneeded log + messages, the array is **not** filtered for each ``logfn()`` call. + +.. c:member:: void (*zlog_target.logfn_sigsafe)(struct zlog_target *zt, const char *text, size_t len) + + Called to deliver "exception" logging messages (i.e. SEGV messages.) + Must be Async-Signal-Safe (may not allocate memory or call "complicated" + libc functions.) May be ``NULL`` if the log target cannot handle this. + +Standard targets +^^^^^^^^^^^^^^^^ + +:file:`lib/zlog_targets.c` provides the standard file / fd / syslog targets. +The syslog target is single-instance while file / fd targets can be +instantiated as needed. There are 3 built-in targets that are fully +autonomous without any config: + +- startup logging to `stderr`, until either :c:func:`zlog_startup_end()` or + :c:func:`zlog_aux_init()` is called. +- stdout logging for non-daemon programs using :c:func:`zlog_aux_init()` +- crashlogs written to :file:`/var/tmp/frr.daemon.crashlog` + +The regular CLI/command-line logging setup is handled by :file:`lib/log_vty.c` +which makes the appropriate instantiations of syslog / file / fd targets. + +.. todo:: + + :c:func:`zlog_startup_end()` should do an explicit switchover from + startup stderr logging to configured logging. Currently, configured logging + starts in parallel as soon as the respective setup is executed. This results + in some duplicate logging. diff --git a/doc/developer/ospf-sr.rst b/doc/developer/ospf-sr.rst index d798ba78ef..070465db5b 100644 --- a/doc/developer/ospf-sr.rst +++ b/doc/developer/ospf-sr.rst @@ -22,7 +22,7 @@ Interoperability ---------------- * Tested on various topology including point-to-point and LAN interfaces - in a mix of Free Range Routing instance and Cisco IOS-XR 6.0.x + in a mix of FRRouting instance and Cisco IOS-XR 6.0.x * Check OSPF LSA conformity with latest wireshark release 2.5.0-rc Implementation details diff --git a/doc/developer/topotests.rst b/doc/developer/topotests.rst index 33ebe06d2f..7e627781e0 100644 --- a/doc/developer/topotests.rst +++ b/doc/developer/topotests.rst @@ -360,6 +360,7 @@ This is the recommended test writing routine: - Write a topology (Graphviz recommended) - Obtain configuration files - Write the test itself +- Format the new code using `black <https://github.com/psf/black>`_ - Create a Pull Request Topotest File Hierarchy @@ -760,6 +761,8 @@ Requirements: inside folders named after the equipment. - Tests must be able to run without any interaction. To make sure your test conforms with this, run it without the :option:`-s` parameter. +- Use `black <https://github.com/psf/black>`_ code formatter before creating + a pull request. This ensures we have a unified code style. Tips: diff --git a/doc/developer/workflow.rst b/doc/developer/workflow.rst index 8ce3bdeeb2..6885a41e0f 100644 --- a/doc/developer/workflow.rst +++ b/doc/developer/workflow.rst @@ -203,7 +203,6 @@ Submitting Patches and Enhancements FRR accepts patches from two sources: -- Email (git format-patch) - GitHub pull request Contributors are highly encouraged to use GitHub's fork-and-PR workflow. It is @@ -228,29 +227,6 @@ summary of the included patches. The description should provide additional details that will help the reviewer to understand the context of the included patches. -Patch Submission via Mailing List ---------------------------------- - -As an alternative submission method, a patch can be mailed to the -development mailing list. Patches received on the mailing list will be -picked up by Patchwork and tested against the latest development branch. - -The recommended way to send the patch (or series of NN patches) to the -list is by using ``git send-email`` as follows (assuming they are the N -most recent commit(s) in your git history):: - - git send-email -NN --annotate --to=dev@lists.frrouting.org - -If your commits do not already contain a ``Signed-off-by`` line, then -use the following command to add it (after making sure you agree to the -Developer Certificate of Origin as outlined above):: - - git send-email -NN --annotate --signoff --to=dev@lists.frrouting.org - -Submitting multi-commit patches as a GitHub pull request is **strongly -encouraged** and increases the probability of your patch getting reviewed and -merged in a timely manner. - .. _license-for-contributions: License for Contributions @@ -377,6 +353,14 @@ After Submitting Your Changes - An author must never delete or manually dismiss someone else's comments or review. (A review may be overridden by agreement in the weekly technical meeting.) + - When you have addressed someone's review comments, please click the + "re-request review" button (in the top-right corner of the PR page, next + to the reviewer's name, an icon that looks like "reload") + - The responsibility for keeping a PR moving rests with the author at + least as long as there are either negative CI results or negative review + comments. If you forget to mark a review comment as addressed (by + clicking re-request review), the reviewer may very well not notice and + won't come back to your PR. - Automatically generated comments, e.g., those generated by CI systems, may be deleted by authors and others when such comments are not the most recent results from that automated comment source. @@ -459,6 +443,24 @@ Guidelines for code review code change is large enough/significant enough to warrant such a requirement. +For project members with merge permissions, the following patterns have +emerged: + +- a PR with any reviews requesting changes may not be merged. + +- a PR with any negative CI result may not be merged. + +- an open "yellow" review mark ("review requested, but not done") should be + given some time (a few days up to weeks, depending on the size of the PR), + but is not a merge blocker. + +- a "textbubble" review mark ("review comments, but not positive/negative") + should be read through but is not a merge blocker. + +- non-trivial PRs are generally given some time (again depending on the size) + for people to mark an interest in reviewing. Trivial PRs may be merged + immediately when CI is green. + Coding Practices & Style ======================== @@ -539,6 +541,28 @@ your new claim at the end of the list. * ... */ +Defensive coding requirements +----------------------------- + +In general, code submitted into FRR will be rejected if it uses unsafe +programming practices. While there is no enforced overall ruleset, the +following requirements have achieved consensus: + +- ``strcpy``, ``strcat`` and ``sprintf`` are inacceptable without exception. + Use ``strlcpy``, ``strlcat`` and ``snprintf`` instead. (Rationale: even if + you know the operation cannot overflow the buffer, a future code change may + inadvertedly introduce an overflow.) + +- buffer size arguments, particularly to ``strlcpy`` and ``snprintf``, must + use ``sizeof()`` whereever possible. Particularly, do not use a size + constant in these cases. (Rationale: changing a buffer to another size + constant may leave the write operations on a now-incorrect size limit.) + +Other than these specific rules, coding practices from the Linux kernel as +well as CERT or MISRA C guidelines may provide useful input on safe C code. +However, these rules are not applied as-is; some of them expressly collide +with established practice. + Code Formatting --------------- @@ -992,6 +1016,11 @@ Miscellaneous When in doubt, follow the guidelines in the Linux kernel style guide, or ask on the development mailing list / public Slack instance. +JSON Output +^^^^^^^^^^^ + +All JSON keys are to be camelCased, with no spaces. + .. _documentation: diff --git a/doc/developer/zebra.rst b/doc/developer/zebra.rst index e3526d1843..e2f887ef28 100644 --- a/doc/developer/zebra.rst +++ b/doc/developer/zebra.rst @@ -9,13 +9,20 @@ Zebra Overview of the Zebra Protocol ============================== -The Zebra protocol is used by protocol daemons to communicate with the -**zebra** daemon. - -Each protocol daemon may request and send information to and from the **zebra** -daemon such as interface states, routing state, nexthop-validation, and so on. -Protocol daemons may also install routes with **zebra**. The **zebra** daemon -manages which routes are installed into the forwarding table with the kernel. +The Zebra protocol (or ``ZAPI``) is used by protocol daemons to +communicate with the **zebra** daemon. + +Each protocol daemon may request and send information to and from the +**zebra** daemon such as interface states, routing state, +nexthop-validation, and so on. Protocol daemons may also install +routes with **zebra**. The **zebra** daemon manages which routes are +installed into the forwarding table with the kernel. Some daemons use +more than one ZAPI connection. This is supported: each ZAPI session is +identified by a tuple of: ``{protocol, instance, session_id}``. LDPD +is an example: it uses a second, synchronous ZAPI session to manage +label blocks. The default value for ``session_id`` is zero; daemons +who use multiple ZAPI sessions must assign unique values to the +sessions' ids. The Zebra protocol is a streaming protocol, with a common header. Version 0 lacks a version field and is implicitly versioned. Version 1 and all subsequent |