]>
Commit | Line | Data |
---|---|---|
9de103f0 QY |
1 | .. _process-and-workflow: |
2 | ||
3 | ******************* | |
b22ba015 | 4 | Process & Workflow |
9de103f0 | 5 | ******************* |
d1890d04 | 6 | |
b6820993 QY |
7 | .. highlight:: none |
8 | ||
b22ba015 QY |
9 | FRR is a large project developed by many different groups. This section |
10 | documents standards for code style & quality, commit messages, pull requests | |
11 | and best practices that all contributors are asked to follow. | |
d1890d04 | 12 | |
9de103f0 QY |
13 | This chapter is "descriptive/post-factual" in that it documents pratices that |
14 | are in use; it is not "definitive/pre-factual" in prescribing practices. This | |
b22ba015 QY |
15 | means that when a procedure changes, it is agreed upon, then put into practice, |
16 | and then documented here. If this document doesn't match reality, it's the | |
17 | document that needs to be updated, not reality. | |
d1890d04 | 18 | |
9de103f0 QY |
19 | Mailing Lists |
20 | ============= | |
d1890d04 | 21 | |
b22ba015 QY |
22 | The FRR development group maintains multiple mailing lists for use by the |
23 | community. Italicized lists are private. | |
d1890d04 QY |
24 | |
25 | +----------------------------------+--------------------------------+ | |
26 | | Topic | List | | |
27 | +==================================+================================+ | |
28 | | Development | dev@lists.frrouting.org | | |
29 | +----------------------------------+--------------------------------+ | |
30 | | Users & Operators | frog@lists.frrouting.org | | |
31 | +----------------------------------+--------------------------------+ | |
32 | | Announcements | announce@lists.frrouting.org | | |
33 | +----------------------------------+--------------------------------+ | |
34 | | *Security* | security@lists.frrouting.org | | |
35 | +----------------------------------+--------------------------------+ | |
36 | | *Technical Steering Committee* | tsc@lists.frrouting.org | | |
37 | +----------------------------------+--------------------------------+ | |
38 | ||
9de103f0 | 39 | The Development list is used to discuss and document general issues related to |
b6820993 QY |
40 | project development and governance. The public |
41 | `Slack instance <https://frrouting.slack.com>`_ and weekly technical meetings | |
42 | provide a higher bandwidth channel for discussions. The results of such | |
43 | discussions must be reflected in updates, as appropriate, to code (i.e., | |
44 | merges), `GitHub issues`_, and for governance or process changes, updates to | |
45 | the Development list and either this file or information posted at | |
46 | https://frrouting.org/. | |
47 | ||
48 | Development & Release Cycle | |
49 | =========================== | |
50 | ||
51 | Development | |
52 | ----------- | |
53 | ||
54 | .. figure:: ../figures/git_branches.png | |
55 | :align: center | |
56 | :scale: 55% | |
57 | :alt: Merging Git branches into a central trunk | |
58 | ||
59 | Rough outline of FRR development workflow | |
60 | ||
61 | The master Git for FRR resides on `GitHub`_. | |
62 | ||
63 | There is one main branch for development, ``master``. For each major release | |
64 | (2.0, 3.0 etc) a new release branch is created based on the master. Significant | |
65 | bugfixes should be backported to upcoming and existing release branches no more | |
66 | than 1 year old. As a general rule new features are not backported to release | |
67 | branches. | |
8ce7861f | 68 | |
b6820993 | 69 | Subsequent point releases based on a major branch are handled with git tags. |
c804874a | 70 | |
b6820993 QY |
71 | Releases |
72 | -------- | |
73 | FRR employs a ``<MAJOR>.<MINOR>.<BUGFIX>`` versioning scheme. | |
c804874a | 74 | |
b6820993 | 75 | ``MAJOR`` |
ac97970d DL |
76 | Significant new features or multiple minor features. This should mostly |
77 | cover any kind of disruptive change that is visible or "risky" to operators. | |
78 | New features or protocols do not necessarily trigger this. (This was changed | |
79 | for FRR 7.x after feedback from users that the pace of major version number | |
80 | increments was too high.) | |
c804874a | 81 | |
b6820993 | 82 | ``MINOR`` |
ac97970d DL |
83 | General incremental development releases, excluding "major" changes |
84 | mentioned above. Not necessarily fully backwards compatible, as smaller | |
85 | (but still visible) changes or deprecated feature removals may still happen. | |
86 | However, there shouldn't be any huge "surprises" between minor releases. | |
c804874a | 87 | |
b6820993 | 88 | ``BUGFIX`` |
ac97970d | 89 | Fixes for actual bugs and/or security issues. Fully compatible. |
c804874a | 90 | |
dc1c0bc2 DL |
91 | Releases are scheduled in a 4-month cycle on the first Tuesday each |
92 | March/July/November. Walking backwards from this date: | |
93 | ||
94 | - 6 weeks earlier, ``master`` is frozen for new features, and feature PRs | |
95 | are considered lowest priority (regardless of when they were opened.) | |
96 | ||
97 | - 4 weeks earlier, the stable branch separates from master (named | |
c3e69122 | 98 | ``dev/MAJOR.MINOR`` at this point) and tagged as ``base_X.Y``. |
5568f9d1 | 99 | Master is unfrozen and new features may again proceed. |
dc1c0bc2 | 100 | |
16044e7f DL |
101 | Part of unfreezing master is editing the ``AC_INIT`` statement in |
102 | :file:`configure.ac` to reflect the new development version that master | |
103 | now refers to. This is accompanied by a ``frr-X.Y-dev`` tag on master, | |
104 | which should always be on the first commit on master *after* the stable | |
105 | branch was forked (even if that is not the edit to ``AC_INIT``; it's more | |
106 | important to have it on the very first commit on master after the fork.) | |
107 | ||
108 | (The :file:`configure.ac` edit and tag push are considered git housekeeping | |
109 | and are pushed directly to ``master``, not through a PR.) | |
110 | ||
5568f9d1 DA |
111 | Below is the snippet of the commands to use in this step. |
112 | ||
113 | .. code-block:: console | |
114 | ||
115 | % git remote --verbose | |
116 | upstream git@github.com:frrouting/frr (fetch) | |
117 | upstream git@github.com:frrouting/frr (push) | |
118 | ||
119 | % git checkout master | |
120 | % git pull upstream master | |
121 | % git checkout -b dev/8.2 | |
122 | % git tag base_8.2 | |
123 | % git push upstream base_8.2 | |
124 | % git push upstream dev/8.2 | |
125 | % git checkout master | |
126 | % sed -i 's/8.2-dev/8.3-dev/' configure.ac | |
5568f9d1 DA |
127 | % git add configure.ac |
128 | % git commit -s -m "build: FRR 8.3 development version" | |
c1242b7b | 129 | % git tag -a frr-8.3-dev -m "frr-8.3-dev" |
5568f9d1 | 130 | % git push upstream master |
c1242b7b | 131 | % git push upstream frr-8.3-dev |
5568f9d1 DA |
132 | |
133 | In this step, we also have to update package versions to reflect | |
134 | the development version. Versions need to be updated using | |
135 | a standard way of development (Pull Requests) based on master branch. | |
136 | ||
137 | Only change the version number with no other changes. This will produce | |
138 | packages with the a version number that is higher than any previous | |
139 | version. Once the release is done, whatever updates we make to changelog | |
140 | files on the release branch need to be cherry-picked to the master branch. | |
141 | ||
f4ebc6f0 DA |
142 | Update essential dates in advance for reference table (below) when |
143 | the next freeze, dev/X.Y, RC, and release phases are scheduled. This should | |
144 | go in the ``master`` branch. | |
145 | ||
5568f9d1 | 146 | - 2 weeks earlier, a ``frr-X.Y-rc`` release candidate is tagged. |
dc1c0bc2 | 147 | |
0a3fa828 DA |
148 | .. code-block:: console |
149 | ||
150 | % git remote --verbose | |
151 | upstream git@github.com:frrouting/frr (fetch) | |
152 | upstream git@github.com:frrouting/frr (push) | |
153 | ||
154 | % git checkout dev/8.2 | |
155 | % git tag frr-8.2-rc | |
156 | % git push upstream frr-8.2-rc | |
157 | ||
dc1c0bc2 DL |
158 | - on release date, the branch is renamed to ``stable/MAJOR.MINOR``. |
159 | ||
160 | The 2 week window between each of these events should be used to run any and | |
161 | all testing possible for the release in progress. However, the current | |
162 | intention is to stick to the schedule even if known issues remain. This would | |
163 | hopefully occur only after all avenues of fixing issues are exhausted, but to | |
164 | achieve this, an as exhaustive as possible list of issues needs to be available | |
165 | as early as possible, i.e. the first 2-week window. | |
166 | ||
167 | For reference, the expected release schedule according to the above is: | |
168 | ||
b28e2c59 DA |
169 | +---------+------------+------------+------------+ |
170 | | Release | 2023-07-04 | 2023-10-31 | 2024-02-27 | | |
171 | +---------+------------+------------+------------+ | |
172 | | RC | 2023-06-20 | 2023-10-17 | 2024-02-13 | | |
173 | +---------+------------+------------+------------+ | |
174 | | dev/X.Y | 2023-06-06 | 2023-10-03 | 2024-01-30 | | |
175 | +---------+------------+------------+------------+ | |
176 | | freeze | 2023-05-23 | 2023-09-19 | 2024-01-16 | | |
177 | +---------+------------+------------+------------+ | |
dc1c0bc2 | 178 | |
9de44c0a DA |
179 | Here is the hint on how to get the dates easily: |
180 | ||
181 | .. code-block:: console | |
182 | ||
183 | ~$ # Last freeze date was 2023-09-19 | |
184 | ~$ date +%F --date='2023-09-19 +119 days' # Next freeze date | |
185 | 2024-01-16 | |
186 | ~$ date +%F --date='2024-01-16 +14 days' # Next dev/X.Y date | |
187 | 2024-01-30 | |
188 | ~$ date +%F --date='2024-01-30 +14 days' # Next RC date | |
189 | 2024-02-13 | |
190 | ~$ date +%F --date='2024-02-13 +14 days' # Next Release date | |
191 | 2024-02-27 | |
192 | ||
dc1c0bc2 | 193 | Each release is managed by one or more volunteer release managers from the FRR |
add70bc3 DS |
194 | community. These release managers are expected to handle the branch for a period |
195 | of one year. To spread and distribute this workload, this should be rotated for | |
dc1c0bc2 DL |
196 | subsequent releases. The release managers are currently assumed/expected to |
197 | run a release management meeting during the weeks listed above. Barring other | |
198 | constraints, this would be scheduled before the regular weekly FRR community | |
199 | call such that important items can be carried over into that call. | |
c804874a | 200 | |
add70bc3 DS |
201 | Bugfixes are applied to the two most recent releases. It is expected that |
202 | each bugfix backported should include some reasoning for its inclusion | |
203 | as well as receiving approval by the release managers for that release before | |
204 | accepted into the release branch. This does not necessarily preclude backporting of | |
205 | bug fixes to older than the two most recent releases. | |
07ff01d2 PG |
206 | |
207 | Security fixes are backported to all releases less than or equal to at least one | |
208 | year old. Security fixes may also be backported to older releases depending on | |
209 | severity. | |
210 | ||
f4bcc72f QY |
211 | For detailed instructions on how to produce an FRR release, refer to |
212 | :ref:`frr-release-procedure`. | |
213 | ||
bd2b4fc3 PG |
214 | |
215 | Long term support branches ( LTS ) | |
216 | ----------------------------------------- | |
217 | ||
218 | This kind of branch is not yet officially supported, and need experimentation | |
219 | before being effective. | |
220 | ||
221 | Previous definition of releases prevents long term support of previous releases. | |
222 | For instance, bug and security fixes are not applied if the stable branch is too | |
223 | old. | |
224 | ||
225 | Because the FRR users have a need to backport bug and security fixes after the | |
226 | stable branch becomes too old, there is a need to provide support on a long term | |
227 | basis on that stable branch. If that support is applied on that stable branch, | |
228 | then that branch is a long term support branch. | |
229 | ||
230 | Having a LTS branch requires extra-work and requires one person to be in charge | |
231 | of that maintenance branch for a certain amount of time. The amount of time will | |
232 | be by default set to 4 months, and can be increased. 4 months stands for the time | |
233 | between two releases, this time can be applied to the decision to continue with a | |
234 | LTS release or not. In all cases, that time period will be well-defined and | |
235 | published. Also, a self nomination from a person that proposes to handle the LTS | |
236 | branch is required. The work can be shared by multiple people. In all cases, there | |
237 | must be at least one person that is in charge of the maintenance branch. The person | |
238 | on people responsible for a maintenance branch must be a FRR maintainer. Note that | |
239 | they may choose to abandon support for the maintenance branch at any time. If | |
56f0bea7 | 240 | no one takes over the responsibility of the LTS branch, then the support will be |
bd2b4fc3 PG |
241 | discontinued. |
242 | ||
243 | The LTS branch duties are the following ones: | |
244 | ||
245 | - organise meetings on a (bi-)weekly or monthly basis, the handling of issues | |
246 | and pull requested relative to that branch. When time permits, this may be done | |
247 | during the regularly scheduled FRR meeting. | |
248 | ||
249 | - ensure the stability of the branch, by using and eventually adapting the | |
250 | checking the CI tools of FRR ( indeed, maintaining may lead to create | |
251 | maintenance branches for topotests or for CI). | |
252 | ||
253 | It will not be possible to backport feature requests to LTS branches. Actually, it | |
254 | is a false good idea to use LTS for that need. Introducing feature requests may | |
255 | break the paradigm where all more recent releases should also include the feature | |
256 | request. This would require the LTS maintainer to ensure that all more recent | |
257 | releases have support for this feature request. Moreover, introducing features | |
258 | requests may result in breaking the stability of the branch. LTS branches are first | |
259 | done to bring long term support for stability. | |
8ce7861f | 260 | |
16318c5c DS |
261 | Development Branches |
262 | -------------------- | |
263 | ||
264 | Occassionally the community will desire the ability to work together | |
265 | on a feature that is considered useful to FRR. In this case the | |
266 | parties may ask the Maintainers for the creation of a development | |
267 | branch in the main FRR repository. Requirements for this to happen | |
268 | are: | |
269 | ||
270 | - A one paragraph description of the feature being implemented to | |
271 | allow for the facilitation of discussion about the feature. This | |
272 | might include pointers to relevant RFC's or presentations that | |
273 | explain what is planned. This is intended to set a somewhat | |
274 | low bar for organization. | |
275 | - A branch maintainer must be named. This person is responsible for | |
276 | keeping the branch up to date, and general communication about the | |
277 | project with the other FRR Maintainers. Additionally this person | |
278 | must already be a FRR Maintainer. | |
279 | - Commits to this branch must follow the normal PR and commit process | |
280 | as outlined in other areas of this document. The goal of this is | |
281 | to prevent the current state where large features are submitted | |
282 | and are so large they are difficult to review. | |
283 | ||
284 | After a development branch has completed the work together, a final | |
285 | review can be made and the branch merged into master. If a development | |
286 | branch is becomes un-maintained or not being actively worked on after | |
287 | three months then the Maintainers can decide to remove the branch. | |
288 | ||
32db86a9 DL |
289 | Debian Branches |
290 | --------------- | |
291 | ||
292 | The Debian project contains "official" packages for FRR. While FRR | |
293 | Maintainers may participate in creating these, it is entirely the Debian | |
294 | project's decision what to ship and how to work on this. | |
295 | ||
296 | As a courtesy and for FRR's benefit, this packaging work is currently visible | |
297 | in git branches named ``debian/*`` on the main FRR git repository. These | |
298 | branches are for the exclusive use by people involved in Debian packaging work | |
299 | for FRR. Direct commit access may be handed out and FRR git rules (review, | |
300 | testing, etc.) do not apply. Do not push to these branches without talking | |
301 | to the people noted under ``Maintainer:`` and ``Uploaders:`` in | |
302 | ``debian/control`` on the target branch -- even if you are a FRR Maintainer. | |
303 | ||
d1890d04 | 304 | Changelog |
b6820993 | 305 | --------- |
b22ba015 QY |
306 | The changelog will be the base for the release notes. A changelog entry for |
307 | your changes is usually not required and will be added based on your commit | |
308 | messages by the maintainers. However, you are free to include an update to the | |
309 | changelog with some better description. | |
d1890d04 | 310 | |
ad5fef3d DL |
311 | Accords: non-code community consensus |
312 | ===================================== | |
313 | ||
314 | The FRR repository has a place for "accords" - these are items of | |
315 | consideration for FRR that influence how we work as a community, but either | |
316 | haven't resulted in code *yet*, or may *never* result in code being written. | |
317 | They are placed in the ``doc/accords/`` directory. | |
318 | ||
319 | The general idea is to simply pass small blurbs of text through our normal PR | |
320 | procedures, giving them the same visibility, comment and review mechanisms as | |
321 | code PRs - and changing them later is another PR. Please refer to the README | |
322 | file in ``doc/accords/`` for further details. The file names of items in that | |
323 | directory are hopefully helpful in determining whether some of them might be | |
324 | relevant to your work. | |
325 | ||
d1890d04 | 326 | Submitting Patches and Enhancements |
9de103f0 | 327 | =================================== |
d1890d04 | 328 | |
85c6ecca | 329 | FRR accepts patches using GitHub pull requests. |
b22ba015 QY |
330 | |
331 | The base branch for new contributions and non-critical bug fixes should be | |
332 | ``master``. Please ensure your pull request is based on this branch when you | |
333 | submit it. | |
334 | ||
85c6ecca | 335 | Code submitted by pull request will be automatically tested by one or more CI |
b6820993 QY |
336 | systems. Once the automated tests succeed, other developers will review your |
337 | code for quality and correctness. After any concerns are resolved, your code | |
338 | will be merged into the branch it was submitted against. | |
d1890d04 | 339 | |
01bf2ccb LB |
340 | The title of the pull request should provide a high level technical |
341 | summary of the included patches. The description should provide | |
342 | additional details that will help the reviewer to understand the context | |
343 | of the included patches. | |
344 | ||
843427dd DA |
345 | Squash commits |
346 | -------------- | |
347 | ||
348 | Before merging make sure a PR has squashed the following kinds of commits: | |
349 | ||
350 | - Fixes/review feedback | |
351 | - Typos | |
352 | - Merges and rebases | |
353 | - Work in progress | |
354 | ||
355 | This helps to automatically generate human-readable changelog messages. | |
356 | ||
19b8d68c DA |
357 | Commit Guidelines |
358 | ----------------- | |
359 | ||
360 | There is a built-in commit linter. Basic rules: | |
361 | ||
362 | - Commit messages must be prefixed with the name of the changed subsystem, followed | |
363 | by a colon and a space and start with an imperative verb. | |
364 | ||
365 | `Check <https://github.com/FRRouting/frr/tree/master/.github/commitlint.config.js>`_ all | |
366 | the supported subsystems. | |
367 | ||
19b8d68c | 368 | - Commit messages must not end with a period ``.`` |
843427dd | 369 | |
2be1c400 DA |
370 | Why was my pull request closed? |
371 | ------------------------------- | |
372 | ||
373 | Pull requests older than 180 days will be closed. Exceptions can be made for | |
374 | pull requests that have active review comments, or that are awaiting other | |
375 | dependent pull requests. Closed pull requests are easy to recreate, and little | |
376 | work is lost by closing a pull request that subsequently needs to be reopened. | |
377 | ||
378 | We want to limit the total number of pull requests in flight to: | |
379 | ||
380 | - Maintain a clean project | |
381 | - Remove old pull requests that would be difficult to rebase as the underlying code has changed over time | |
382 | - Encourage code velocity | |
383 | ||
b6820993 | 384 | .. _license-for-contributions: |
d1890d04 | 385 | |
b6820993 QY |
386 | License for Contributions |
387 | ------------------------- | |
388 | FRR is under a “GPLv2 or later” license. Any code submitted must be released | |
389 | under the same license (preferred) or any license which allows redistribution | |
390 | under this GPLv2 license (eg MIT License). | |
e2abcff8 PG |
391 | It is forbidden to push any code that prevents from using GPLv3 license. This |
392 | becomes a community rule, as FRR produces binaries that links with Apache 2.0 | |
393 | libraries. Apache 2.0 and GPLv2 license are incompatible, if put together. | |
394 | Please see `<http://www.apache.org/licenses/GPL-compatibility.html>`_ for | |
395 | more information. This rule guarantees the user to distribute FRR binary code | |
396 | without any licensing issues. | |
b22ba015 | 397 | |
b6820993 QY |
398 | Pre-submission Checklist |
399 | ------------------------ | |
400 | - Format code (see `Code Formatting <#code-formatting>`__) | |
401 | - Verify and acknowledge license (see :ref:`license-for-contributions`) | |
402 | - Ensure you have properly signed off (see :ref:`signing-off`) | |
403 | - Test building with various configurations: | |
d1890d04 | 404 | |
b6820993 | 405 | - ``buildtest.sh`` |
d1890d04 | 406 | |
b6820993 | 407 | - Verify building source distribution: |
d1890d04 | 408 | |
b6820993 | 409 | - ``make dist`` (and try rebuilding from the resulting tar file) |
d1890d04 | 410 | |
b6820993 | 411 | - Run unit tests: |
d1890d04 | 412 | |
b6820993 | 413 | - ``make test`` |
d1890d04 | 414 | |
b6820993 | 415 | - In the case of a major new feature or other significant change, document |
8bc6e629 DS |
416 | plans for continued maintenance of the feature. In addition it is a |
417 | requirement that automated testing must be written that exercises | |
431dd37e | 418 | the new feature within our existing CI infrastructure. Also the |
8bc6e629 | 419 | addition of automated testing to cover any pull request is encouraged. |
d1890d04 | 420 | |
e605a239 DS |
421 | - All new code must use the current latest version of acceptable code. |
422 | ||
423 | - If a daemon is converted to YANG, then new code must use YANG. | |
424 | - DEFPY's must be used for new cli | |
425 | - Typesafe lists must be used | |
426 | - printf formatting changes must be used | |
427 | ||
b6820993 | 428 | .. _signing-off: |
d1890d04 | 429 | |
b6820993 QY |
430 | Signing Off |
431 | ----------- | |
432 | Code submitted to FRR must be signed off. We have the same requirements for | |
433 | using the signed-off-by process as the Linux kernel. In short, you must include | |
434 | a ``Signed-off-by`` tag in every patch. | |
d1890d04 | 435 | |
118cf7ed SW |
436 | An easy way to do this is to use ``git commit -s`` where ``-s`` will automatically |
437 | append a signed-off line to the end of your commit message. Also, if you commit | |
438 | and forgot to add the line you can use ``git commit --amend -s`` to add the | |
439 | signed-off line to the last commit. | |
440 | ||
b6820993 QY |
441 | ``Signed-off-by`` is a developer's certification that they have the right to |
442 | submit the patch for inclusion into the project. It is an agreement to the | |
443 | :ref:`Developer's Certificate of Origin <developers-certificate-of-origin>`. | |
444 | Code without a proper ``Signed-off-by`` line cannot and will not be merged. | |
d1890d04 | 445 | |
b6820993 QY |
446 | If you are unfamiliar with this process, you should read the |
447 | `official policy at kernel.org <https://www.kernel.org/doc/html/latest/process/submitting-patches.html>`_. | |
448 | You might also find | |
449 | `this article <http://www.linuxfoundation.org/content/how-participate-linux-community-0>`_ | |
450 | about participating in the Linux community on the Linux Foundation website to | |
451 | be a helpful resource. | |
d1890d04 | 452 | |
b6820993 | 453 | .. _developers-certificate-of-origin: |
d1890d04 | 454 | |
b6820993 QY |
455 | In short, when you sign off on a commit, you assert your agreement to all of |
456 | the following:: | |
d1890d04 | 457 | |
b6820993 | 458 | Developer's Certificate of Origin 1.1 |
d1890d04 | 459 | |
b6820993 | 460 | By making a contribution to this project, I certify that: |
d1890d04 | 461 | |
b6820993 QY |
462 | (a) The contribution was created in whole or in part by me and I |
463 | have the right to submit it under the open source license | |
464 | indicated in the file; or | |
d1890d04 | 465 | |
b6820993 QY |
466 | (b) The contribution is based upon previous work that, to the best |
467 | of my knowledge, is covered under an appropriate open source | |
468 | license and I have the right under that license to submit that | |
469 | work with modifications, whether created in whole or in part by | |
470 | me, under the same open source license (unless I am permitted to | |
471 | submit under a different license), as indicated in the file; or | |
d1890d04 | 472 | |
b6820993 QY |
473 | (c) The contribution was provided directly to me by some other |
474 | person who certified (a), (b) or (c) and I have not modified it. | |
d1890d04 | 475 | |
b6820993 QY |
476 | (d) I understand and agree that this project and the contribution |
477 | are public and that a record of the contribution (including all | |
478 | personal information I submit with it, including my sign-off) is | |
479 | maintained indefinitely and may be redistributed consistent with | |
480 | this project or the open source license(s) involved. | |
d1890d04 | 481 | |
b6820993 | 482 | After Submitting Your Changes |
d1890d04 QY |
483 | ----------------------------- |
484 | ||
b6820993 | 485 | - Watch for Continuous Integration (CI) test results |
d1890d04 QY |
486 | |
487 | - You should automatically receive an email with the test results | |
488 | within less than 2 hrs of the submission. If you don’t get the | |
b6820993 | 489 | email, then check status on the GitHub pull request. |
d1890d04 | 490 | - Please notify the development mailing list if you think something |
b22ba015 | 491 | doesn't work. |
d1890d04 QY |
492 | |
493 | - If the tests failed: | |
494 | ||
495 | - In general, expect the community to ignore the submission until | |
496 | the tests pass. | |
497 | - It is up to you to fix and resubmit. | |
498 | ||
499 | - This includes fixing existing unit (“make test”) tests if your | |
500 | changes broke or changed them. | |
501 | - It also includes fixing distribution packages for the failing | |
502 | platforms (ie if new libraries are required). | |
503 | - Feel free to ask for help on the development list. | |
504 | ||
505 | - Go back to the submission process and repeat until the tests pass. | |
506 | ||
507 | - If the tests pass: | |
508 | ||
509 | - Wait for reviewers. Someone will review your code or be assigned | |
510 | to review your code. | |
493e3eed LB |
511 | - Respond to any comments or concerns the reviewer has. Use e-mail or |
512 | add a comment via github to respond or to let the reviewer know how | |
513 | their comment or concern is addressed. | |
514 | - An author must never delete or manually dismiss someone else's comments | |
515 | or review. (A review may be overridden by agreement in the weekly | |
516 | technical meeting.) | |
70aa675d DL |
517 | - When you have addressed someone's review comments, please click the |
518 | "re-request review" button (in the top-right corner of the PR page, next | |
519 | to the reviewer's name, an icon that looks like "reload") | |
520 | - The responsibility for keeping a PR moving rests with the author at | |
521 | least as long as there are either negative CI results or negative review | |
522 | comments. If you forget to mark a review comment as addressed (by | |
523 | clicking re-request review), the reviewer may very well not notice and | |
524 | won't come back to your PR. | |
493e3eed LB |
525 | - Automatically generated comments, e.g., those generated by CI systems, |
526 | may be deleted by authors and others when such comments are not the most | |
22265b35 | 527 | recent results from that automated comment source. |
d1890d04 QY |
528 | - After all comments and concerns are addressed, expect your patch |
529 | to be merged. | |
530 | ||
531 | - Watch out for questions on the mailing list. At this time there will | |
532 | be a manual code review and further (longer) tests by various | |
533 | community members. | |
534 | - Your submission is done once it is merged to the master branch. | |
535 | ||
9de103f0 QY |
536 | Programming Languages, Tools and Libraries |
537 | ========================================== | |
538 | ||
539 | The core of FRR is written in C (gcc or clang supported) and makes | |
2ac74f0d CH |
540 | use of GNU compiler extensions. Additionally, the CLI generation |
541 | tool, `clippy`, requires Python. A few other non-essential scripts are | |
9de103f0 QY |
542 | implemented in Perl and Python. FRR requires the following tools |
543 | to build distribution packages: automake, autoconf, texinfo, libtool and | |
544 | gawk and various libraries (i.e. libpam and libjson-c). | |
545 | ||
546 | If your contribution requires a new library or other tool, then please | |
547 | highlight this in your description of the change. Also make sure it’s | |
548 | supported by all FRR platform OSes or provide a way to build | |
549 | without the library (potentially without the new feature) on the other | |
550 | platforms. | |
551 | ||
552 | Documentation should be written in reStructuredText. Sphinx extensions may be | |
553 | utilized but pure ReST is preferred where possible. See | |
554 | :ref:`documentation`. | |
555 | ||
ca9dfee0 DL |
556 | Use of C++ |
557 | ---------- | |
558 | ||
559 | While C++ is not accepted for core components of FRR, extensions, modules or | |
560 | other distinct components may want to use C++ and include FRR header files. | |
561 | There is no requirement on contributors to work to retain C++ compatibility, | |
562 | but fixes for C++ compatibility are welcome. | |
563 | ||
564 | This implies that the burden of work to keep C++ compatibility is placed with | |
565 | the people who need it, and they may provide it at their leisure to the extent | |
566 | it is useful to them. So, if only a subset of header files, or even parts of | |
567 | a header file are made available to C++, this is perfectly fine. | |
568 | ||
590a7368 QY |
569 | Code Reviews |
570 | ============ | |
571 | ||
572 | Code quality is paramount for any large program. Consequently we require | |
573 | reviews of all submitted patches by at least one person other than the | |
574 | submitter before the patch is merged. | |
575 | ||
576 | Because of the nature of the software, FRR's maintainer list (i.e. those with | |
577 | commit permissions) tends to contain employees / members of various | |
578 | organizations. In order to prevent conflicts of interest, we use an honor | |
579 | system in which submissions from an individual representing one company should | |
580 | be merged by someone unaffiliated with that company. | |
581 | ||
582 | Guidelines for code review | |
924947e4 | 583 | -------------------------- |
590a7368 QY |
584 | |
585 | - As a rule of thumb, the depth of the review should be proportional to the | |
586 | scope and / or impact of the patch. | |
587 | ||
588 | - Anyone may review a patch. | |
589 | ||
590 | - When using GitHub reviews, marking "Approve" on a code review indicates | |
591 | willingness to merge the PR. | |
592 | ||
593 | - For individuals with merge rights, marking "Changes requested" is equivalent | |
594 | to a NAK. | |
595 | ||
596 | - For a PR you marked with "Changes requested", please respond to updates in a | |
597 | timely manner to avoid impeding the flow of development. | |
598 | ||
7e678379 LB |
599 | - Rejected or obsolete PRs are generally closed by the submitter based |
600 | on requests and/or agreement captured in a PR comment. The comment | |
601 | may originate with a reviewer or document agreement reached on Slack, | |
602 | the Development mailing list, or the weekly technical meeting. | |
603 | ||
8bc6e629 DS |
604 | - Reviewers may ask for new automated testing if they feel that the |
605 | code change is large enough/significant enough to warrant such | |
606 | a requirement. | |
607 | ||
70aa675d DL |
608 | For project members with merge permissions, the following patterns have |
609 | emerged: | |
610 | ||
611 | - a PR with any reviews requesting changes may not be merged. | |
612 | ||
613 | - a PR with any negative CI result may not be merged. | |
614 | ||
615 | - an open "yellow" review mark ("review requested, but not done") should be | |
616 | given some time (a few days up to weeks, depending on the size of the PR), | |
617 | but is not a merge blocker. | |
618 | ||
619 | - a "textbubble" review mark ("review comments, but not positive/negative") | |
620 | should be read through but is not a merge blocker. | |
621 | ||
622 | - non-trivial PRs are generally given some time (again depending on the size) | |
623 | for people to mark an interest in reviewing. Trivial PRs may be merged | |
624 | immediately when CI is green. | |
625 | ||
590a7368 | 626 | |
b22ba015 | 627 | Coding Practices & Style |
9de103f0 | 628 | ======================== |
d1890d04 QY |
629 | |
630 | Commit messages | |
9de103f0 | 631 | --------------- |
d1890d04 QY |
632 | |
633 | Commit messages should be formatted in the same way as Linux kernel | |
b6820993 | 634 | commit messages. The format is roughly:: |
d1890d04 QY |
635 | |
636 | dir: short summary | |
637 | ||
638 | extended summary | |
639 | ||
b6820993 QY |
640 | ``dir`` should be the top level source directory under which the change was |
641 | made. For example, a change in :file:`bgpd/rfapi` would be formatted as:: | |
d1890d04 | 642 | |
9de103f0 | 643 | bgpd: short summary |
d1890d04 | 644 | |
b6820993 QY |
645 | ... |
646 | ||
647 | The first line should be no longer than 50 characters. Subsequent lines should | |
648 | be wrapped to 72 characters. | |
d1890d04 | 649 | |
7bd4560b QY |
650 | The purpose of commit messages is to briefly summarize what the commit is |
651 | changing. Therefore, the extended summary portion should be in the form of an | |
652 | English paragraph. Brief examples of program output are acceptable but if | |
653 | present should be short (on the order of 10 lines) and clearly demonstrate what | |
654 | has changed. The goal should be that someone with only passing familiarity with | |
655 | the code in question can understand what is being changed. | |
656 | ||
657 | Commit messages consisting entirely of program output are *unacceptable*. These | |
658 | do not describe the behavior changed. For example, putting VTYSH output or the | |
659 | result of test runs as the sole content of commit messages is unacceptable. | |
660 | ||
b6820993 QY |
661 | You must also sign off on your commit. |
662 | ||
663 | .. seealso:: :ref:`signing-off` | |
664 | ||
7bd4560b | 665 | |
b6820993 | 666 | Source File Header |
9de103f0 | 667 | ------------------ |
d1890d04 | 668 | |
b6820993 QY |
669 | New files must have a copyright header (see :ref:`license-for-contributions` |
670 | above) added to the file. The header should be: | |
d1890d04 | 671 | |
b6820993 | 672 | .. code-block:: c |
d1890d04 | 673 | |
0d60d63f | 674 | // SPDX-License-Identifier: GPL-2.0-or-later |
d1890d04 QY |
675 | /* |
676 | * Title/Function of file | |
677 | * Copyright (C) YEAR Author’s Name | |
d1890d04 QY |
678 | */ |
679 | ||
680 | #include <zebra.h> | |
681 | ||
0d60d63f DL |
682 | A ``SPDX-License-Identifier`` header is required in all source files, i.e. |
683 | ``.c``, ``.h``, ``.cpp`` and ``.py`` files. The license boilerplate should be | |
684 | removed in these files. Some existing files are missing this header, this is | |
685 | slowly being fixed. | |
686 | ||
687 | A ``SPDX-License-Identifier`` header *and* the full license boilerplate is | |
688 | required in schema definition files, i.e. ``.yang`` and ``.proto``. The | |
689 | rationale for this is that these files are likely to be individually copied to | |
690 | places outside FRR, and having only the SPDX header would become a "dangling | |
691 | pointer". | |
692 | ||
693 | .. warning:: | |
694 | ||
695 | **DO NOT REMOVE A "Copyright" LINE OR AUTHOR NAME, EVER.** | |
696 | ||
697 | **DO NOT APPLY AN SPDX HEADER WHEN THE LICENSE IS UNCLEAR, UNLESS YOU HAVE | |
698 | CHECKED WITH *ALL* SIGNIFICANT AUTHORS.** | |
699 | ||
700 | Please to keep ``#include <zebra.h>``. The absolute first header included in | |
701 | any C file **must** be either ``zebra.h`` or ``config.h`` (with HAVE_CONFIG_H | |
702 | guard.) | |
b6820993 | 703 | |
b6820993 QY |
704 | |
705 | Adding Copyright Claims to Existing Files | |
9de103f0 | 706 | ----------------------------------------- |
d1890d04 | 707 | |
b6820993 QY |
708 | When adding copyright claims for modifications to an existing file, please |
709 | add a ``Portions:`` section as shown below. If this section already exists, add | |
710 | your new claim at the end of the list. | |
d1890d04 | 711 | |
b6820993 | 712 | .. code-block:: c |
d1890d04 | 713 | |
b6820993 QY |
714 | /* |
715 | * Title/Function of file | |
716 | * Copyright (C) YEAR Author’s Name | |
717 | * Portions: | |
718 | * Copyright (C) 2010 Entity A .... | |
719 | * Copyright (C) 2016 Your name [optional brief change description] | |
720 | * ... | |
721 | */ | |
d1890d04 | 722 | |
08cffeb5 DL |
723 | Defensive coding requirements |
724 | ----------------------------- | |
725 | ||
726 | In general, code submitted into FRR will be rejected if it uses unsafe | |
727 | programming practices. While there is no enforced overall ruleset, the | |
728 | following requirements have achieved consensus: | |
729 | ||
7533cad7 | 730 | - ``strcpy``, ``strcat`` and ``sprintf`` are unacceptable without exception. |
08cffeb5 DL |
731 | Use ``strlcpy``, ``strlcat`` and ``snprintf`` instead. (Rationale: even if |
732 | you know the operation cannot overflow the buffer, a future code change may | |
733 | inadvertedly introduce an overflow.) | |
734 | ||
735 | - buffer size arguments, particularly to ``strlcpy`` and ``snprintf``, must | |
736 | use ``sizeof()`` whereever possible. Particularly, do not use a size | |
737 | constant in these cases. (Rationale: changing a buffer to another size | |
738 | constant may leave the write operations on a now-incorrect size limit.) | |
739 | ||
2787347d QY |
740 | - For stack allocated structs and arrays that should be zero initialized, |
741 | prefer initializer expressions over ``memset()`` wherever possible. This | |
742 | helps prevent ``memset()`` calls being missed in branches, and eliminates the | |
743 | error class of an incorrect ``size`` argument to ``memset()``. | |
744 | ||
745 | For example, instead of: | |
746 | ||
747 | .. code-block:: c | |
748 | ||
749 | struct foo mystruct; | |
750 | ... | |
751 | memset(&mystruct, 0x00, sizeof(struct foo)); | |
752 | ||
753 | Prefer: | |
754 | ||
755 | .. code-block:: c | |
756 | ||
757 | struct foo mystruct = {}; | |
758 | ||
759 | - Do not zero initialize stack allocated values that must be initialized with a | |
760 | nonzero value in order to be used. This way the compiler and memory checking | |
761 | tools can catch uninitialized value use that would otherwise be suppressed by | |
762 | the (incorrect) zero initialization. | |
763 | ||
08cffeb5 DL |
764 | Other than these specific rules, coding practices from the Linux kernel as |
765 | well as CERT or MISRA C guidelines may provide useful input on safe C code. | |
766 | However, these rules are not applied as-is; some of them expressly collide | |
767 | with established practice. | |
768 | ||
161ed8a6 DL |
769 | |
770 | Container implementations | |
771 | ^^^^^^^^^^^^^^^^^^^^^^^^^ | |
772 | ||
773 | In particular to gain defensive coding benefits from better compiler type | |
774 | checks, there is a set of replacement container data structures to be found | |
775 | in :file:`lib/typesafe.h`. They're documented under :ref:`lists`. | |
776 | ||
777 | Unfortunately, the FRR codebase is quite large, and migrating existing code to | |
778 | use these new structures is a tedious and far-reaching process (even if it | |
779 | can be automated with coccinelle, the patches would touch whole swaths of code | |
780 | and create tons of merge conflicts for ongoing work.) Therefore, little | |
781 | existing code has been migrated. | |
782 | ||
783 | However, both **new code and refactors of existing code should use the new | |
784 | containers**. If there are any reasons this can't be done, please work to | |
785 | remove these reasons (e.g. by adding necessary features to the new containers) | |
786 | rather than falling back to the old code. | |
787 | ||
788 | In order of likelyhood of removal, these are the old containers: | |
789 | ||
790 | - :file:`nhrpd/list.*`, ``hlist_*`` ⇒ ``DECLARE_LIST`` | |
791 | - :file:`nhrpd/list.*`, ``list_*`` ⇒ ``DECLARE_DLIST`` | |
792 | - :file:`lib/skiplist.*`, ``skiplist_*`` ⇒ ``DECLARE_SKIPLIST`` | |
793 | - :file:`lib/*_queue.h` (BSD), ``SLIST_*`` ⇒ ``DECLARE_LIST`` | |
794 | - :file:`lib/*_queue.h` (BSD), ``LIST_*`` ⇒ ``DECLARE_DLIST`` | |
795 | - :file:`lib/*_queue.h` (BSD), ``STAILQ_*`` ⇒ ``DECLARE_LIST`` | |
796 | - :file:`lib/*_queue.h` (BSD), ``TAILQ_*`` ⇒ ``DECLARE_DLIST`` | |
797 | - :file:`lib/hash.*`, ``hash_*`` ⇒ ``DECLARE_HASH`` | |
798 | - :file:`lib/linklist.*`, ``list_*`` ⇒ ``DECLARE_DLIST`` | |
799 | - open-coded linked lists ⇒ ``DECLARE_LIST``/``DECLARE_DLIST`` | |
800 | ||
801 | ||
c964e511 | 802 | Code Formatting |
9de103f0 | 803 | --------------- |
d1890d04 | 804 | |
6f7a9254 QY |
805 | C Code |
806 | ^^^^^^ | |
807 | ||
808 | For C code, FRR uses Linux kernel style except where noted below. Code which | |
809 | does not comply with these style guidelines will not be accepted. | |
d1890d04 | 810 | |
281ba953 QY |
811 | The project provides multiple tools to allow you to correctly style your code |
812 | as painlessly as possible, primarily built around ``clang-format``. | |
813 | ||
814 | clang-format | |
815 | In the project root there is a :file:`.clang-format` configuration file | |
816 | which can be used with the ``clang-format`` source formatter tool from the | |
817 | LLVM project. Most of the time, this is the easiest and smartest tool to | |
818 | use. It can be run in a variety of ways. If you point it at a C source file | |
819 | or directory of source files, it will format all of them. In the LLVM source | |
820 | tree there are scripts that allow you to integrate it with ``git``, ``vim`` | |
821 | and ``emacs``, and there are third-party plugins for other editors. The | |
822 | ``git`` integration is particularly useful; suppose you have some changes in | |
823 | your git index. Then, with the integration installed, you can do the | |
824 | following: | |
825 | ||
826 | :: | |
827 | ||
828 | git clang-format | |
829 | ||
830 | This will format *only* the changes present in your index. If you have just | |
831 | made a few commits and would like to correctly style only the changes made | |
832 | in those commits, you can use the following syntax: | |
833 | ||
834 | :: | |
835 | ||
836 | git clang-format HEAD~X | |
837 | ||
838 | Where X is one more than the number of commits back from the tip of your | |
839 | branch you would like ``clang-format`` to look at (similar to specifying the | |
840 | target for a rebase). | |
841 | ||
842 | The ``vim`` plugin is particularly useful. It allows you to select lines in | |
843 | visual line mode and press a key binding to invoke ``clang-format`` on only | |
844 | those lines. | |
845 | ||
846 | When using ``clang-format``, it is recommended to use the latest version. | |
847 | Each consecutive version generally has better handling of various edge | |
848 | cases. You may notice on occasion that two consecutive runs of | |
849 | ``clang-format`` over the same code may result in changes being made on the | |
850 | second run. This is an unfortunate artifact of the tool. Please check with | |
851 | the kernel style guide if in doubt. | |
852 | ||
853 | One stylistic problem with the FRR codebase is the use of ``DEFUN`` macros | |
854 | for defining CLI commands. ``clang-format`` will happily format these macro | |
855 | invocations, but the result is often unsightly and difficult to read. | |
856 | Consequently, FRR takes a more relaxed position with how these are | |
857 | formatted. In general you should lean towards using the style exemplified in | |
858 | the section on :ref:`command-line-interface`. Because ``clang-format`` | |
859 | mangles this style, there is a Python script named ``tools/indent.py`` that | |
860 | wraps ``clang-format`` and handles ``DEFUN`` macros as well as some other | |
861 | edge cases specific to FRR. If you are submitting a new file, it is | |
862 | recommended to run that script over the new file, preferably after ensuring | |
863 | that the latest stable release of ``clang-format`` is in your ``PATH``. | |
864 | ||
865 | Documentation on ``clang-format`` and its various integrations is maintained | |
866 | on the LLVM website. | |
867 | ||
868 | https://clang.llvm.org/docs/ClangFormat.html | |
869 | ||
870 | checkpatch.sh | |
871 | In the Linux kernel source tree there is a Perl script used to check | |
872 | incoming patches for style errors. FRR uses an adapted version of this | |
873 | script for the same purpose. It can be found at | |
2780ae0c | 874 | :file:`tools/checkpatch.sh`. This script takes a git-formatted diff or |
281ba953 QY |
875 | patch file, applies it to a clean FRR tree, and inspects the result to catch |
876 | potential style errors. Running this script on your patches before | |
877 | submission is highly recommended. The CI system runs this script as well and | |
878 | will comment on the PR with the results if style errors are found. | |
879 | ||
b6820993 | 880 | It is run like this:: |
281ba953 | 881 | |
b6820993 | 882 | ./checkpatch.sh <patch> <tree> |
281ba953 QY |
883 | |
884 | Reports are generated on ``stderr`` and the exit code indicates whether | |
885 | issues were found (2, 1) or not (0). | |
886 | ||
887 | Where ``<patch>`` is the path to the diff or patch file and ``<tree>`` is | |
888 | the path to your FRR source tree. The tree should be on the branch that you | |
889 | intend to submit the patch against. The script will make a best-effort | |
890 | attempt to save the state of your working tree and index before applying the | |
891 | patch, and to restore it when it is done, but it is still recommended that | |
892 | you have a clean working tree as the script does perform a hard reset on | |
893 | your tree during its run. | |
894 | ||
895 | The script reports two classes of issues, namely WARNINGs and ERRORs. Please | |
896 | pay attention to both of them. The script will generally report WARNINGs | |
897 | where it cannot be 100% sure that a particular issue is real. In most cases | |
898 | WARNINGs indicate an issue that needs to be fixed. Sometimes the script will | |
899 | report false positives; these will be handled in code review on a | |
900 | case-by-case basis. Since the script only looks at changed lines, | |
901 | occasionally changing one part of a line can cause the script to report a | |
902 | style issue already present on that line that is unrelated to the change. | |
903 | When convenient it is preferred that these be cleaned up inline, but this is | |
904 | not required. | |
905 | ||
115e70a1 PZ |
906 | In general, a developer should heed the information reported by checkpatch. |
907 | However, some flexibility is needed for cases where human judgement yields | |
908 | better clarity than the script. Accordingly, it may be appropriate to | |
909 | ignore some checkpatch.sh warnings per discussion among the submitter(s) | |
910 | and reviewer(s) of a change. Misreporting of errors by the script is | |
d3c2e316 QY |
911 | possible. When this occurs, the exception should be handled either by |
912 | patching checkpatch to correct the false error report, or by documenting the | |
913 | exception in this document under :ref:`style-exceptions`. If the incorrect | |
914 | report is likely to appear again, a checkpatch update is preferred. | |
115e70a1 | 915 | |
281ba953 QY |
916 | If the script finds one or more WARNINGs it will exit with 1. If it finds |
917 | one or more ERRORs it will exit with 2. | |
918 | ||
919 | ||
920 | Please remember that while FRR provides these tools for your convenience, | |
921 | responsibility for properly formatting your code ultimately lies on the | |
922 | shoulders of the submitter. As such, it is recommended to double-check the | |
923 | results of these tools to avoid delays in merging your submission. | |
d1890d04 | 924 | |
115e70a1 PZ |
925 | In some cases, these tools modify or flag the format in ways that go beyond or |
926 | even conflict [#tool_style_conflicts]_ with the canonical documented Linux | |
927 | kernel style. In these cases, the Linux kernel style takes priority; | |
928 | non-canonical issues flagged by the tools are not compulsory but rather are | |
929 | opportunities for discussion among the submitter(s) and reviewer(s) of a change. | |
930 | ||
d1890d04 QY |
931 | **Whitespace changes in untouched parts of the code are not acceptable |
932 | in patches that change actual code.** To change/fix formatting issues, | |
933 | please create a separate patch that only does formatting changes and | |
934 | nothing else. | |
935 | ||
d1890d04 QY |
936 | Kernel and BSD styles are documented externally: |
937 | ||
938 | - https://www.kernel.org/doc/html/latest/process/coding-style.html | |
939 | - http://man.openbsd.org/style | |
940 | ||
941 | For GNU coding style, use ``indent`` with the following invocation: | |
942 | ||
943 | :: | |
944 | ||
945 | indent -nut -nfc1 file_for_submission.c | |
946 | ||
28ac5a03 QY |
947 | |
948 | Historically, FRR used fixed-width integral types that do not exist in any | |
949 | standard but were defined by most platforms at some point. Officially these | |
950 | types are not guaranteed to exist. Therefore, please use the fixed-width | |
951 | integral types introduced in the C99 standard when contributing new code to | |
952 | FRR. If you need to convert a large amount of code to use the correct types, | |
953 | there is a shell script in :file:`tools/convert-fixedwidth.sh` that will do the | |
954 | necessary replacements. | |
955 | ||
956 | +-----------+--------------------------+ | |
957 | | Incorrect | Correct | | |
958 | +===========+==========================+ | |
959 | | u_int8_t | uint8_t | | |
960 | +-----------+--------------------------+ | |
961 | | u_int16_t | uint16_t | | |
962 | +-----------+--------------------------+ | |
963 | | u_int32_t | uint32_t | | |
964 | +-----------+--------------------------+ | |
965 | | u_int64_t | uint64_t | | |
966 | +-----------+--------------------------+ | |
967 | | u_char | uint8_t or unsigned char | | |
968 | +-----------+--------------------------+ | |
969 | | u_short | unsigned short | | |
970 | +-----------+--------------------------+ | |
971 | | u_int | unsigned int | | |
972 | +-----------+--------------------------+ | |
973 | | u_long | unsigned long | | |
974 | +-----------+--------------------------+ | |
975 | ||
3f115705 DL |
976 | FRR also uses unnamed struct fields, enabled with ``-fms-extensions`` (cf. |
977 | https://gcc.gnu.org/onlinedocs/gcc/Unnamed-Fields.html). The following two | |
978 | patterns can/should be used where contextually appropriate: | |
979 | ||
980 | .. code-block:: c | |
981 | ||
982 | struct outer { | |
983 | struct inner; | |
984 | }; | |
985 | ||
986 | .. code-block:: c | |
987 | ||
988 | struct outer { | |
989 | union { | |
990 | struct inner; | |
991 | struct inner inner_name; | |
992 | }; | |
993 | }; | |
994 | ||
995 | ||
d3c2e316 QY |
996 | .. _style-exceptions: |
997 | ||
d1890d04 | 998 | Exceptions |
3656e87b | 999 | """""""""" |
d1890d04 QY |
1000 | |
1001 | FRR project code comes from a variety of sources, so there are some | |
1002 | stylistic exceptions in place. They are organized here by branch. | |
1003 | ||
3656e87b | 1004 | For ``master``: |
d1890d04 QY |
1005 | |
1006 | BSD coding style applies to: | |
1007 | ||
1008 | - ``ldpd/`` | |
1009 | ||
1010 | ``babeld`` uses, approximately, the following style: | |
1011 | ||
1012 | - K&R style braces | |
1013 | - Indents are 4 spaces | |
1014 | - Function return types are on their own line | |
1015 | ||
3656e87b | 1016 | For ``stable/3.0`` and ``stable/2.0``: |
d1890d04 QY |
1017 | |
1018 | GNU coding style apply to the following parts: | |
1019 | ||
1020 | - ``lib/`` | |
1021 | - ``zebra/`` | |
1022 | - ``bgpd/`` | |
1023 | - ``ospfd/`` | |
1024 | - ``ospf6d/`` | |
1025 | - ``isisd/`` | |
1026 | - ``ripd/`` | |
1027 | - ``ripngd/`` | |
1028 | - ``vtysh/`` | |
1029 | ||
1030 | BSD coding style applies to: | |
1031 | ||
1032 | - ``ldpd/`` | |
1033 | ||
3656e87b QY |
1034 | |
1035 | Python Code | |
1036 | ^^^^^^^^^^^ | |
1037 | ||
1038 | Format all Python code with `black <https://github.com/psf/black>`_. | |
1039 | ||
1040 | In a line:: | |
1041 | ||
1042 | python3 -m black <file.py> | |
1043 | ||
1044 | Run this on any Python files you modify before committing. | |
1045 | ||
1046 | FRR's Python code has been formatted with black version 19.10b. | |
1047 | ||
1048 | ||
6f7a9254 QY |
1049 | YANG |
1050 | ^^^^ | |
1051 | ||
1052 | FRR uses YANG to define data models for its northbound interface. YANG models | |
1053 | should follow conventions used by the IETF standard models. From a practical | |
1054 | standpoint, this corresponds to the output produced by the ``yanglint`` tool | |
1055 | included in the ``libyang`` project, which is used by FRR to parse and validate | |
1056 | YANG models. You should run the following command on all YANG documents you | |
1057 | write: | |
1058 | ||
1059 | .. code-block:: console | |
1060 | ||
1061 | yanglint -f yang <model> | |
1062 | ||
1063 | The output of this command should be identical to the input file. The sole | |
1064 | exception to this is comments. ``yanglint`` does not support comments and will | |
1065 | strip them from its output. You may include comments in your YANG documents, | |
1066 | but they should be indented appropriately (use spaces). Where possible, | |
1067 | comments should be eschewed in favor of a suitable ``description`` statement. | |
1068 | ||
1069 | In short, a diff between your input file and the output of ``yanglint`` should | |
1070 | either be empty or contain only comments. | |
d3c2e316 QY |
1071 | |
1072 | Specific Exceptions | |
1073 | ^^^^^^^^^^^^^^^^^^^ | |
1074 | ||
1075 | Most of the time checkpatch errors should be corrected. Occasionally as a group | |
1076 | maintainers will decide to ignore certain stylistic issues. Usually this is | |
1077 | because correcting the issue is not possible without large unrelated code | |
1078 | changes. When an exception is made, if it is unlikely to show up again and | |
1079 | doesn't warrant an update to checkpatch, it is documented here. | |
1080 | ||
1081 | +------------------------------------------+---------------------------------------------------------------+ | |
1082 | | Issue | Ignore Reason | | |
1083 | +==========================================+===============================================================+ | |
1084 | | DEFPY_HIDDEN, DEFPY_ATTR: complex macros | DEF* macros cannot be wrapped in parentheses without updating | | |
1085 | | should be wrapped in parentheses | all usages of the macro, which would be highly disruptive. | | |
1086 | +------------------------------------------+---------------------------------------------------------------+ | |
1087 | ||
b6660a65 DL |
1088 | Types of configurables |
1089 | ---------------------- | |
1090 | ||
1091 | .. note:: | |
1092 | ||
1093 | This entire section essentially just argues to not make configuration | |
1094 | unnecessarily involved for the user. Rather than rules, this is more of | |
1095 | a list of conclusions intended to help make FRR usable for operators. | |
1096 | ||
1097 | ||
1098 | Almost every feature FRR has comes with its own set of switches and options. | |
1099 | There are several stages at which configuration can be applied. In order of | |
1100 | preference, these are: | |
1101 | ||
1102 | - at configuration/runtime, through YANG. | |
1103 | ||
1104 | This is the preferred way for all FRR knobs. Not all daemons and features | |
1105 | are fully YANGified yet, so in some cases new features cannot rely on a | |
424117e5 DL |
1106 | YANG interface. If a daemon already implements a YANG interface (even |
1107 | partial), new CLI options must be implemented through a YANG model. | |
1108 | ||
1109 | .. warning:: | |
1110 | ||
1111 | Unlike everything else in this section being guidelines with some slack, | |
1112 | implementing and using a YANG interface for new CLI options in (even | |
1113 | partially!) YANGified daemons is a hard requirement. | |
1114 | ||
b6660a65 DL |
1115 | |
1116 | - at configuration/runtime, through the CLI. | |
1117 | ||
1118 | The "good old" way for all regular configuration. More involved for users | |
1119 | to automate *correctly* than YANG. | |
1120 | ||
1121 | - at startup, by loading additional modules. | |
1122 | ||
1123 | If a feature introduces a dependency on additional libraries (e.g. libsnmp, | |
1124 | rtrlib, etc.), this is the best way to encapsulate the dependency. Having | |
1125 | a separate module allows the distribution to create a separate package | |
1126 | with the extra dependency, so FRR can still be installed without pulling | |
1127 | everything in. | |
1128 | ||
1129 | A module may also be appropriate if a feature is large and reasonably well | |
1130 | isolated. Reducing the amount of running the code is a security benefit, | |
1131 | so even if there are no new external dependencies, modules can be useful. | |
1132 | ||
1133 | While modules cannot currently be loaded at runtime, this is a tradeoff | |
1134 | decision that was made to allow modules to change/extend code that is very | |
1135 | hard to (re)adjust at runtime. If there is a case for runtime (un)loading | |
1136 | of modules, this tradeoff can absolutely be reevaluated. | |
1137 | ||
1138 | - at startup, with command line options. | |
1139 | ||
1140 | This interface is only appropriate for options that have an effect very | |
1141 | early in FRR startup, i.e. before configuration is loaded. Anything that | |
1142 | affects configuration load itself should be here, as well as options | |
1143 | changing the environment FRR runs in. | |
1144 | ||
1145 | If a tunable can be changed at runtime, a command line option is only | |
1146 | acceptable if the configured value has an effect before configuration is | |
1147 | loaded (e.g. zebra reads routes from the kernel before loading config, so | |
1148 | the netlink buffer size is an appropriate command line option.) | |
1149 | ||
1150 | - at compile time, with ``./configure`` options. | |
1151 | ||
1152 | This is the absolute last preference for tunables, since the distribution | |
1153 | needs to make the decision for the user and/or the user needs to rebuild | |
1154 | FRR in order to change the option. | |
1155 | ||
1156 | "Good" configure options do one of three things: | |
1157 | ||
1158 | - set distribution-specific parameters, most prominently all the path | |
1159 | options. File system layout is a distribution/packaging choice, so the | |
1160 | user would hopefully never need to adjust these. | |
1161 | ||
1162 | - changing toolchain behavior, e.g. instrumentation, warnings, | |
1163 | optimizations and sanitizers. | |
1164 | ||
1165 | - enabling/disabling parts of the build, especially if they need | |
1166 | additional dependencies. Being able to build only parts of FRR, or | |
1167 | without some library, is useful. **The only effect these options should | |
1168 | have is adding or removing files from the build result.** If a knob | |
1169 | in this category causes the same binary to exist in different variants, | |
1170 | it is likely implemented incorrectly! | |
1171 | ||
1172 | .. note:: | |
1173 | ||
1174 | This last guideline is currently ignored by several configure options. | |
1175 | ``vtysh`` in general depends on the entire list of enabled daemons, | |
1176 | and options like ``--enable-bgp-vnc`` and ``--enable-ospfapi`` change | |
1177 | daemons internally. Consider this more of an "ideal" than a "rule". | |
1178 | ||
1179 | ||
1180 | Whenever adding new knobs, please try reasonably hard to go up as far as | |
1181 | possible on the above list. Especially ``./configure`` flags are often enough | |
1182 | the "easy way out" but should be avoided when at all possible. To a lesser | |
1183 | degree, the same applies to command line options. | |
1184 | ||
1185 | ||
d1890d04 | 1186 | Compile-time conditional code |
9de103f0 | 1187 | ----------------------------- |
d1890d04 QY |
1188 | |
1189 | Many users access FRR via binary packages from 3rd party sources; | |
1190 | compile-time code puts inclusion/exclusion in the hands of the package | |
1191 | maintainer. Please think very carefully before making code conditional | |
1192 | at compile time, as it increases regression testing, maintenance | |
1193 | burdens, and user confusion. In particular, please avoid gratuitous | |
1194 | ``--enable-…`` switches to the configure script - in general, code | |
1195 | should be of high quality and in working condition, or it shouldn’t be | |
1196 | in FRR at all. | |
1197 | ||
1198 | When code must be compile-time conditional, try have the compiler make | |
1199 | it conditional rather than the C pre-processor so that it will still be | |
1200 | checked by the compiler, even if disabled. For example, | |
1201 | ||
1202 | :: | |
1203 | ||
1204 | if (SOME_SYMBOL) | |
1205 | frobnicate(); | |
1206 | ||
1207 | is preferred to | |
1208 | ||
1209 | :: | |
1210 | ||
1211 | #ifdef SOME_SYMBOL | |
1212 | frobnicate (); | |
1213 | #endif /* SOME_SYMBOL */ | |
1214 | ||
b6820993 QY |
1215 | Note that the former approach requires ensuring that ``SOME_SYMBOL`` will be |
1216 | defined (watch your ``AC_DEFINE``\ s). | |
d1890d04 QY |
1217 | |
1218 | Debug-guards in code | |
9de103f0 | 1219 | -------------------- |
d1890d04 | 1220 | |
b6820993 QY |
1221 | Debugging statements are an important methodology to allow developers to fix |
1222 | issues found in the code after it has been released. The caveat here is that | |
1223 | the developer must remember that people will be using the code at scale and in | |
1224 | ways that can be unexpected for the original implementor. As such debugs | |
1225 | **MUST** be guarded in such a way that they can be turned off. FRR has the | |
1226 | ability to turn on/off debugs from the CLI and it is expected that the | |
1227 | developer will use this convention to allow control of their debugs. | |
d1890d04 | 1228 | |
81047bc5 DL |
1229 | Custom syntax-like block macros |
1230 | ------------------------------- | |
1231 | ||
1232 | FRR uses some macros that behave like the ``for`` or ``if`` C keywords. These | |
1233 | macros follow these patterns: | |
1234 | ||
1235 | - loop-style macros are named ``frr_each_*`` (and ``frr_each``) | |
1236 | - single run macros are named ``frr_with_*`` | |
1237 | - to avoid confusion, ``frr_with_*`` macros must always use a ``{ ... }`` | |
1238 | block even if the block only contains one statement. The ``frr_each`` | |
1239 | constructs are assumed to be well-known enough to use normal ``for`` rules. | |
1240 | - ``break``, ``return`` and ``goto`` all work correctly. For loop-style | |
1241 | macros, ``continue`` works correctly too. | |
1242 | ||
1243 | Both the ``each`` and ``with`` keywords are inspired by other (more | |
1244 | higher-level) programming languages that provide these constructs. | |
1245 | ||
1246 | There are also some older iteration macros, e.g. ``ALL_LIST_ELEMENTS`` and | |
1247 | ``FOREACH_AFI_SAFI``. These macros in some cases do **not** fulfill the above | |
1248 | pattern (e.g. ``break`` does not work in ``FOREACH_AFI_SAFI`` because it | |
1249 | expands to 2 nested loops.) | |
1250 | ||
9e001286 QY |
1251 | Static Analysis and Sanitizers |
1252 | ------------------------------ | |
81af0317 DL |
1253 | Clang/LLVM and GCC come with a variety of tools that can be used to help find |
1254 | bugs in FRR. | |
9e001286 QY |
1255 | |
1256 | clang-analyze | |
1257 | This is a static analyzer that scans the source code looking for patterns | |
1258 | that are likely to be bugs. The tool is run automatically on pull requests | |
1259 | as part of CI and new static analysis warnings will be placed in the CI | |
1260 | results. FRR aims for absolutely zero static analysis errors. While the | |
1261 | project is not quite there, code that introduces new static analysis errors | |
1262 | is very unlikely to be merged. | |
1263 | ||
1264 | AddressSanitizer | |
1265 | This is an excellent tool that provides runtime instrumentation for | |
1266 | detecting memory errors. As part of CI FRR is built with this | |
1267 | instrumentation and run through a series of tests to look for any results. | |
1268 | Testing your own code with this tool before submission is encouraged. You | |
1269 | can enable it by passing:: | |
d5403d4f | 1270 | |
9e001286 QY |
1271 | --enable-address-sanitizer |
1272 | ||
1273 | to ``configure``. | |
1274 | ||
1275 | ThreadSanitizer | |
1276 | Similar to AddressSanitizer, this tool provides runtime instrumentation for | |
1277 | detecting data races. If you are working on or around multithreaded code, | |
1278 | extensive testing with this instrumtation enabled is *highly* recommended. | |
1279 | You can enable it by passing:: | |
d5403d4f | 1280 | |
9e001286 QY |
1281 | --enable-thread-sanitizer |
1282 | ||
1283 | to ``configure``. | |
1284 | ||
1285 | MemorySanitizer | |
1286 | Similar to AddressSanitizer, this tool provides runtime instrumentation for | |
1287 | detecting use of uninitialized heap memory. Testing your own code with this | |
1288 | tool before submission is encouraged. You can enable it by passing:: | |
d5403d4f | 1289 | |
9e001286 QY |
1290 | --enable-memory-sanitizer |
1291 | ||
1292 | to ``configure``. | |
1293 | ||
1294 | All of the above tools are available in the Clang/LLVM toolchain since 3.4. | |
1295 | AddressSanitizer and ThreadSanitizer are available in recent versions of GCC, | |
1296 | but are no longer actively maintained. MemorySanitizer is not available in GCC. | |
1297 | ||
81af0317 DL |
1298 | .. note:: |
1299 | ||
1300 | The different Sanitizers are mostly incompatible with each other. Please | |
1301 | refer to GCC/LLVM documentation for details. | |
1302 | ||
f62de63c DL |
1303 | frr-format plugin |
1304 | This is a GCC plugin provided with FRR that does extended type checks for | |
1305 | ``%pFX``-style printfrr extensions. To use this plugin, | |
1306 | ||
1307 | 1. install GCC plugin development files, e.g.:: | |
1308 | ||
1309 | apt-get install gcc-10-plugin-dev | |
1310 | ||
1311 | 2. **before** running ``configure``, compile the plugin with:: | |
1312 | ||
1313 | make -C tools/gcc-plugins CXX=g++-10 | |
1314 | ||
1315 | (Edit the GCC version to what you're using, it should work for GCC 9 or | |
1316 | newer.) | |
1317 | ||
1318 | After this, the plugin should be automatically picked up by ``configure``. | |
1319 | The plugin does not change very frequently, so you can keep it around across | |
1320 | work on different FRR branches. After a ``git clean -x``, the ``make`` line | |
1321 | will need to be run again. You can also add ``--with-frr-format`` to the | |
1322 | ``configure`` line to make sure the plugin is used, otherwise if something | |
1323 | is not set up correctly it might be silently ignored. | |
1324 | ||
1325 | .. warning:: | |
1326 | ||
1327 | Do **not** enable this plugin for package/release builds. It is intended | |
1328 | for developer/debug builds only. Since it modifies the compiler, it may | |
1329 | cause silent corruption of the executable files. | |
1330 | ||
1331 | Using the plugin also changes the string for ``PRI[udx]64`` from the | |
1332 | system value to ``%L[udx]`` (normally ``%ll[udx]`` or ``%l[udx]``.) | |
1333 | ||
9e001286 QY |
1334 | Additionally, the FRR codebase is regularly scanned with Coverity. |
1335 | Unfortunately Coverity does not have the ability to handle scanning pull | |
1336 | requests, but after code is merged it will send an email notifying project | |
1337 | members with Coverity access of newly introduced defects. | |
1338 | ||
81af0317 DL |
1339 | Executing non-installed dynamic binaries |
1340 | ---------------------------------------- | |
1341 | ||
1342 | Since FRR uses the GNU autotools build system, it inherits its shortcomings. | |
1343 | To execute a binary directly from the build tree under a wrapper like | |
1344 | `valgrind`, `gdb` or `strace`, use:: | |
1345 | ||
1346 | ./libtool --mode=execute valgrind [--valgrind-opts] zebra/zebra [--zebra-opts] | |
1347 | ||
1348 | While replacing valgrind/zebra as needed. The `libtool` script is found in | |
1349 | the root of the build directory after `./configure` has completed. Its purpose | |
1350 | is to correctly set up `LD_LIBRARY_PATH` so that libraries from the build tree | |
1351 | are used. (On some systems, `libtool` is also available from PATH, but this is | |
1352 | not always the case.) | |
1353 | ||
b44b66c7 CH |
1354 | .. _cli-workflow: |
1355 | ||
d1890d04 | 1356 | CLI changes |
9de103f0 | 1357 | ----------- |
d1890d04 | 1358 | |
b6820993 | 1359 | CLI's are a complicated ugly beast. Additions or changes to the CLI should use |
264274da DS |
1360 | a DEFPY to encapsulate one setting as much as is possible. Additionally as new |
1361 | DEFPY's are added to the system, documentation should be provided for the new | |
b6820993 | 1362 | commands. |
d1890d04 QY |
1363 | |
1364 | Backwards Compatibility | |
9de103f0 | 1365 | ----------------------- |
d1890d04 | 1366 | |
b6820993 QY |
1367 | As a general principle, changes to CLI and code in the lib/ directory should be |
1368 | made in a backwards compatible fashion. This means that changes that are purely | |
1369 | stylistic in nature should be avoided, e.g., renaming an existing macro or | |
1370 | library function name without any functional change. When adding new parameters | |
1371 | to common functions, it is also good to consider if this too should be done in | |
1372 | a backward compatible fashion, e.g., by preserving the old form in addition to | |
d1890d04 QY |
1373 | adding the new form. |
1374 | ||
b6820993 QY |
1375 | This is not to say that minor or even major functional changes to CLI and |
1376 | common code should be avoided, but rather that the benefit gained from a change | |
1377 | should be weighed against the added cost/complexity to existing code. Also, | |
1378 | that when making such changes, it is good to preserve compatibility when | |
1379 | possible to do so without introducing maintenance overhead/cost. It is also | |
1380 | important to keep in mind, existing code includes code that may reside in | |
1381 | private repositories (and is yet to be submitted) or code that has yet to be | |
1382 | migrated from Quagga to FRR. | |
110bb121 | 1383 | |
b6820993 QY |
1384 | That said, compatibility measures can (and should) be removed when either: |
1385 | ||
1386 | - they become a significant burden, e.g. when data structures change and the | |
1387 | compatibility measure would need a complex adaptation layer or becomes | |
1388 | flat-out impossible | |
1389 | - some measure of time (dependent on the specific case) has passed, so that | |
1390 | the compatibility grace period is considered expired. | |
1391 | ||
e12ea4bb QY |
1392 | For CLI commands, the deprecation period is 1 year. |
1393 | ||
b6820993 QY |
1394 | In all cases, compatibility pieces should be marked with compiler/preprocessor |
1395 | annotations to print warnings at compile time, pointing to the appropriate | |
1396 | update path. A ``-Werror`` build should fail if compatibility bits are used. To | |
1397 | avoid compilation issues in released code, such compiler/preprocessor | |
1398 | annotations must be ignored non-development branches. For example: | |
1399 | ||
1400 | .. code-block:: c | |
1401 | ||
e60dd6ca | 1402 | #if CONFDATE > 20180403 |
b6820993 QY |
1403 | CPP_NOTICE("Use of <XYZ> is deprecated, please use <ABC>") |
1404 | #endif | |
d1890d04 | 1405 | |
cab3f811 LB |
1406 | Preferably, the shell script :file:`tools/fixup-deprecated.py` will be |
1407 | updated along with making non-backwards compatible code changes, or an | |
1408 | alternate script should be introduced, to update the code to match the | |
1409 | change. When the script is updated, there is no need to preserve the | |
1410 | deprecated code. Note that this does not apply to user interface | |
1411 | changes, just internal code, macros and libraries. | |
1412 | ||
d1890d04 | 1413 | Miscellaneous |
9de103f0 | 1414 | ------------- |
d1890d04 | 1415 | |
b6820993 QY |
1416 | When in doubt, follow the guidelines in the Linux kernel style guide, or ask on |
1417 | the development mailing list / public Slack instance. | |
9de103f0 | 1418 | |
e9f2bc24 QY |
1419 | JSON Output |
1420 | ^^^^^^^^^^^ | |
1421 | ||
b44b66c7 CH |
1422 | New JSON output in FRR needs to be backed by schema, in particular a YANG model. |
1423 | When adding new JSON, first search for an existing YANG model, either in FRR or | |
1424 | a standard model (e.g., IETF) and use that model as the basis for any JSON | |
1425 | structure and *especially* for key names and canonical values formats. | |
1426 | ||
1427 | If no YANG model exists to support the JSON then an FRR YANG model needs to be | |
1428 | added to or created to support the JSON format. | |
1429 | ||
1430 | * All JSON keys are to be ``camelCased``, with no spaces. YANG modules almost | |
1431 | always use ``kebab-case`` (i.e., all lower case with hyphens to separate | |
1432 | words), so these identifiers need to be mapped to ``camelCase`` by removing | |
1433 | the hyphen (or symbol) and capitalizing the following letter, for | |
1434 | example "router-id" becomes "routerId" | |
47563324 QY |
1435 | * Commands which output JSON should produce ``{}`` if they have nothing to |
1436 | display | |
b44b66c7 CH |
1437 | * In general JSON commands include a ``json`` keyword typically at the end of |
1438 | the CLI command (e.g., ``show ip ospf json``) | |
e9f2bc24 | 1439 | |
7d68dd44 MS |
1440 | Use of const |
1441 | ^^^^^^^^^^^^ | |
1442 | ||
1443 | Please consider using ``const`` when possible: it's a useful hint to | |
1444 | callers about the limits to side-effects from your apis, and it makes | |
1445 | it possible to use your apis in paths that involve ``const`` | |
1446 | objects. If you encounter existing apis that *could* be ``const``, | |
1447 | consider including changes in your own pull-request. | |
1448 | ||
e5af0fc8 DL |
1449 | Help with specific warnings |
1450 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ | |
1451 | ||
1452 | FRR's configure script enables a whole batch of extra warnings, some of which | |
1453 | may not be obvious in how to fix. Here are some notes on specific warnings: | |
1454 | ||
1455 | * ``-Wstrict-prototypes``: you probably just forgot the ``void`` in a function | |
1456 | declaration with no parameters, i.e. ``static void foo() {...}`` rather than | |
1457 | ``static void foo(void) {...}``. | |
1458 | ||
1459 | Without the ``void``, in C, it's a function with *unspecified* parameters | |
1460 | (and varargs calling convention.) This is a notable difference to C++, where | |
1461 | the ``void`` is optional and an empty parameter list means no parameters. | |
1462 | ||
f62de63c DL |
1463 | * ``"strict match required"`` from the frr-format plugin: check if you are |
1464 | using a cast in a printf parameter list. The frr-format plugin cannot | |
1465 | access correct full type information for casts like | |
1466 | ``printfrr(..., (uint64_t)something, ...)`` and will print incorrect | |
1467 | warnings particularly if ``uint64_t``, ``size_t`` or ``ptrdiff_t`` are | |
1468 | involved. The problem is *not* triggered with a variable or function return | |
1469 | value of the exact same type (without a cast). | |
1470 | ||
1471 | Since these cases are very rare, community consensus is to just work around | |
1472 | the warning even though the code might be correct. If you are running into | |
1473 | this, your options are: | |
1474 | ||
1475 | 1. try to avoid the cast altogether, maybe using a different printf format | |
1476 | specifier (e.g. ``%lu`` instead of ``%zu`` or ``PRIu64``). | |
1477 | 2. fix the type(s) of the function/variable/struct member being printed | |
1478 | 3. create a temporary variable with the value and print that without a cast | |
1479 | (this is the last resort and was not necessary anywhere so far.) | |
1480 | ||
9de103f0 QY |
1481 | |
1482 | .. _documentation: | |
1483 | ||
1484 | Documentation | |
1485 | ============= | |
1486 | ||
1487 | FRR uses Sphinx+RST as its documentation system. The document you are currently | |
1488 | reading was generated by Sphinx from RST source in | |
1489 | :file:`doc/developer/workflow.rst`. The documentation is structured as follows: | |
1490 | ||
d5403d4f QY |
1491 | +-----------------------+-------------------------------------------+ |
1492 | | Directory | Contents | | |
1493 | +=======================+===========================================+ | |
1494 | | :file:`doc/user` | User documentation; configuration guides; | | |
1495 | | | protocol overviews | | |
1496 | +-----------------------+-------------------------------------------+ | |
1497 | | :file:`doc/developer` | Developer's documentation; API specs; | | |
1498 | | | datastructures; architecture overviews; | | |
1499 | | | project management procedure | | |
1500 | +-----------------------+-------------------------------------------+ | |
1501 | | :file:`doc/manpages` | Source for manpages | | |
1502 | +-----------------------+-------------------------------------------+ | |
1503 | | :file:`doc/figures` | Images and diagrams | | |
1504 | +-----------------------+-------------------------------------------+ | |
1505 | | :file:`doc/extra` | Miscellaneous Sphinx extensions, scripts, | | |
1506 | | | customizations, etc. | | |
1507 | +-----------------------+-------------------------------------------+ | |
1508 | ||
1509 | Each of these directories, with the exception of :file:`doc/figures` and | |
1510 | :file:`doc/extra`, contains a Sphinx-generated Makefile and configuration | |
1511 | script :file:`conf.py` used to set various document parameters. The makefile | |
1512 | can be used for a variety of targets; invoke `make help` in any of these | |
1513 | directories for a listing of available output formats. For convenience, there | |
1514 | is a top-level :file:`Makefile.am` that has targets for PDF and HTML | |
1515 | documentation for both developer and user documentation, respectively. That | |
1516 | makefile is also responsible for building manual pages packed with distribution | |
1517 | builds. | |
9de103f0 QY |
1518 | |
1519 | Indent and styling should follow existing conventions: | |
1520 | ||
1521 | - 3 spaces for indents under directives | |
1522 | - Cross references may contain only lowercase alphanumeric characters and | |
1523 | hyphens ('-') | |
1524 | - Lines wrapped to 80 characters where possible | |
1525 | ||
1526 | Characters for header levels should follow Python documentation guide: | |
1527 | ||
1528 | - ``#`` with overline, for parts | |
1529 | - ``*`` with overline, for chapters | |
1530 | - ``=``, for sections | |
1531 | - ``-``, for subsections | |
1532 | - ``^``, for subsubsections | |
1533 | - ``"``, for paragraphs | |
1534 | ||
1535 | After you have made your changes, please make sure that you can invoke | |
1536 | ``make latexpdf`` and ``make html`` with no warnings. | |
1537 | ||
1538 | The documentation is currently incomplete and needs love. If you find a broken | |
1539 | cross-reference, figure, dead hyperlink, style issue or any other nastiness we | |
1540 | gladly accept documentation patches. | |
1541 | ||
c91e9b8f QY |
1542 | To build the docs, please ensure you have installed a recent version of |
1543 | `Sphinx <http://www.sphinx-doc.org/en/stable/install.html>`_. If you want to | |
1544 | build LaTeX or PDF docs, you will also need a full LaTeX distribution | |
1545 | installed. | |
1546 | ||
9de103f0 QY |
1547 | Code |
1548 | ---- | |
1549 | ||
1550 | FRR is a large and complex software project developed by many different people | |
1551 | over a long period of time. Without adequate documentation, it can be | |
1552 | exceedingly difficult to understand code segments, APIs and other interfaces. | |
1553 | In the interest of keeping the project healthy and maintainable, you should | |
1554 | make every effort to document your code so that other people can understand | |
1555 | what it does without needing to closely read the code itself. | |
1556 | ||
1557 | Some specific guidelines that contributors should follow are: | |
1558 | ||
1559 | - Functions exposed in header files should have descriptive comments above | |
1560 | their signatures in the header file. At a minimum, a function comment should | |
1561 | contain information about the return value, parameters, and a general summary | |
1562 | of the function's purpose. Documentation on parameter values can be omitted | |
1563 | if it is (very) obvious what they are used for. | |
1564 | ||
1565 | Function comments must follow the style for multiline comments laid out in | |
1566 | the kernel style guide. | |
1567 | ||
1568 | Example: | |
1569 | ||
1570 | .. code-block:: c | |
1571 | ||
1572 | /* | |
1573 | * Determines whether or not a string is cool. | |
1574 | * | |
b6820993 QY |
1575 | * text |
1576 | * the string to check for coolness | |
1577 | * | |
1578 | * is_clccfc | |
1579 | * whether capslock is cruise control for cool | |
1580 | * | |
1581 | * Returns: | |
1582 | * 7 if the text is cool, 0 otherwise | |
9de103f0 QY |
1583 | */ |
1584 | int check_coolness(const char *text, bool is_clccfc); | |
1585 | ||
b6820993 QY |
1586 | Function comments should make it clear what parameters and return values are |
1587 | used for. | |
9de103f0 QY |
1588 | |
1589 | - Static functions should have descriptive comments in the same form as above | |
1590 | if what they do is not immediately obvious. Use good engineering judgement | |
1591 | when deciding whether a comment is necessary. If you are unsure, document | |
1592 | your code. | |
1593 | - Global variables, static or not, should have a comment describing their use. | |
1594 | - **For new code in lib/, these guidelines are hard requirements.** | |
1595 | ||
1596 | If you make significant changes to portions of the codebase covered in the | |
1597 | Developer's Manual, add a major subsystem or feature, or gain arcane mastery of | |
1598 | some undocumented or poorly documented part of the codebase, please document | |
1599 | your work so others can benefit. If you add a major feature or introduce a new | |
1600 | API, please document the architecture and API to the best of your abilities in | |
1601 | the Developer's Manual, using good judgement when choosing where to place it. | |
1602 | ||
1603 | Finally, if you come across some code that is undocumented and feel like | |
1604 | going above and beyond, document it! We absolutely appreciate and accept | |
1605 | patches that document previously undocumented code. | |
1606 | ||
1607 | User | |
1608 | ---- | |
1609 | ||
1610 | If you are contributing code that adds significant user-visible functionality | |
1611 | please document how to use it in :file:`doc/user`. Use good judgement when | |
1612 | choosing where to place documentation. For example, instructions on how to use | |
1613 | your implementation of a new BGP draft should go in the BGP chapter instead of | |
1614 | being its own chapter. If you are adding a new protocol daemon, please create a | |
1615 | new chapter. | |
1616 | ||
d5403d4f QY |
1617 | FRR Specific Markup |
1618 | ------------------- | |
1619 | ||
1620 | FRR has some customizations applied to the Sphinx markup that go a long way | |
1621 | towards making documentation easier to use, write and maintain. | |
1622 | ||
1623 | CLI Commands | |
1624 | ^^^^^^^^^^^^ | |
1625 | ||
e1ac6ff4 QY |
1626 | When documenting CLI please use the ``.. clicmd::`` directive. This directive |
1627 | will format the command and generate index entries automatically. For example, | |
1628 | the command :clicmd:`show pony` would be documented as follows: | |
9de103f0 QY |
1629 | |
1630 | .. code-block:: rest | |
1631 | ||
9de103f0 QY |
1632 | .. clicmd:: show pony |
1633 | ||
1634 | Prints an ASCII pony. Example output::: | |
1635 | ||
1636 | >>\. | |
1637 | /_ )`. | |
1638 | / _)`^)`. _.---. _ | |
1639 | (_,' \ `^-)"" `.\ | |
1640 | | | \ | |
1641 | \ / | | |
1642 | / \ /.___.'\ (\ (_ | |
1643 | < ,"|| \ |`. \`-' | |
1644 | \\ () )| )/ | |
1645 | hjw |_>|> /_] // | |
1646 | /_] /_] | |
1647 | ||
e1ac6ff4 | 1648 | |
9de103f0 QY |
1649 | When documented this way, CLI commands can be cross referenced with the |
1650 | ``:clicmd:`` inline markup like so: | |
1651 | ||
1652 | .. code-block:: rest | |
1653 | ||
1654 | :clicmd:`show pony` | |
1655 | ||
1656 | This is very helpful for users who want to quickly remind themselves what a | |
1657 | particular command does. | |
1658 | ||
e1ac6ff4 QY |
1659 | When documenting a cli that has a ``no`` form, please do not include the ``no`` |
1660 | form. I.e. ``no show pony`` would not be documented anywhere. Since most | |
1661 | commands have ``no`` forms, users should be able to infer these or get help | |
1662 | from vtysh's completions. | |
1663 | ||
1664 | When documenting commands that have lots of possible variants, just document | |
1665 | the single command in summary rather than enumerating each possible variant. | |
1666 | E.g. for ``show pony [foo|bar]``, do not: | |
1667 | ||
1668 | .. code-block:: rest | |
1669 | ||
1670 | .. clicmd:: show pony | |
1671 | .. clicmd:: show pony foo | |
1672 | .. clicmd:: show pony bar | |
1673 | ||
1674 | Do: | |
1675 | ||
1676 | .. code-block:: rest | |
1677 | ||
1678 | .. clicmd:: show pony [foo|bar] | |
1679 | ||
41cb383f | 1680 | |
d5403d4f QY |
1681 | Configuration Snippets |
1682 | ^^^^^^^^^^^^^^^^^^^^^^ | |
1683 | ||
1684 | When putting blocks of example configuration please use the | |
1685 | ``.. code-block::`` directive and specify ``frr`` as the highlighting language, | |
1686 | as in the following example. This will tell Sphinx to use a custom Pygments | |
1687 | lexer to highlight FRR configuration syntax. | |
1688 | ||
1689 | .. code-block:: rest | |
1690 | ||
1691 | .. code-block:: frr | |
1692 | ||
1693 | ! | |
1694 | ! Example configuration file. | |
1695 | ! | |
1696 | log file /tmp/log.log | |
1697 | service integrated-vtysh-config | |
1698 | ! | |
1699 | ip route 1.2.3.0/24 reject | |
1700 | ipv6 route de:ea:db:ee:ff::/64 reject | |
1701 | ! | |
1702 | ||
1703 | ||
9de103f0 QY |
1704 | .. _GitHub: https://github.com/frrouting/frr |
1705 | .. _GitHub issues: https://github.com/frrouting/frr/issues | |
115e70a1 PZ |
1706 | |
1707 | .. rubric:: Footnotes | |
1708 | ||
1709 | .. [#tool_style_conflicts] For example, lines over 80 characters are allowed | |
1710 | for text strings to make it possible to search the code for them: please | |
1711 | see `Linux kernel style (breaking long lines and strings) <https://www.kernel.org/doc/html/v4.10/process/coding-style.html#breaking-long-lines-and-strings>`_ | |
1712 | and `Issue #1794 <https://github.com/FRRouting/frr/issues/1794>`_. |