]>
Commit | Line | Data |
---|---|---|
fa482834 PJ |
1 | %% -*- mode: text; -*- |
2 | %% $QuaggaId: Format:%an, %ai, %h$ $ | |
3 | ||
4 | \documentclass[oneside]{article} | |
5 | \usepackage{parskip} | |
6 | \usepackage[bookmarks,colorlinks=true]{hyperref} | |
7 | ||
8 | \title{Conventions for working on Quagga} | |
9 | ||
10 | \begin{document} | |
11 | \maketitle | |
12 | ||
13 | This is a living document. Suggestions for updates, via the | |
14 | \href{http://lists.quagga.net/mailman/listinfo/quagga-dev}{quagga-dev list}, | |
15 | are welcome. | |
16 | ||
17 | \tableofcontents | |
18 | ||
19 | \section{GUIDELINES FOR HACKING ON QUAGGA} | |
20 | \label{sec:guidelines} | |
21 | ||
22 | ||
23 | GNU coding standards apply. Indentation follows the result of | |
24 | invoking GNU indent (as of 2.2.8a) with no arguments. Note that this | |
25 | uses tabs instead of spaces where possible for leading whitespace, and | |
26 | assumes that tabs are every 8 columns. Do not attempt to redefine the | |
27 | location of tab stops. Note also that some indentation does not | |
28 | follow GNU style. This is a historical accident, and we generally | |
29 | only clean up whitespace when code is unmaintainable due to whitespace | |
30 | issues, to minimise merging conflicts. | |
31 | ||
32 | For GNU emacs, use indentation style ``gnu''. | |
33 | ||
34 | For Vim, use the following lines (note that tabs are at 8, and that | |
35 | softtabstop sets the indentation level): | |
36 | ||
37 | set tabstop=8 | |
38 | set softtabstop=2 | |
39 | set shiftwidth=2 | |
40 | set noexpandtab | |
41 | ||
42 | Be particularly careful not to break platforms/protocols that you | |
43 | cannot test. | |
44 | ||
45 | New code should have good comments, which explain why the code is correct. | |
46 | Changes to existing code should in many cases upgrade the comments when | |
47 | necessary for a reviewer to conclude that the change has no unintended | |
48 | consequences. | |
49 | ||
50 | Each file in the Git repository should have a git format-placeholder (like | |
51 | an RCS Id keyword), somewhere very near the top, commented out appropriately | |
52 | for the file type. The placeholder used for Quagga (replacing <dollar> with | |
53 | \$) is: | |
54 | ||
55 | \verb|$QuaggaId: <dollar>Format:%an, %ai, %h<dollar> $| | |
56 | ||
57 | See line 2 of HACKING.tex, the source for this document, for an example. | |
58 | ||
59 | This placeholder string will be expanded out by the `git archive' commands, | |
60 | wihch is used to generate the tar archives for snapshots and releases. | |
61 | ||
62 | Please document fully the proper use of a new function in the header file | |
63 | in which it is declared. And please consult existing headers for | |
64 | documentation on how to use existing functions. In particular, please consult | |
65 | these header files: | |
66 | ||
67 | \begin{description} | |
68 | \item{lib/log.h} logging levels and usage guidance | |
69 | \item{[more to be added]} | |
70 | \end{description} | |
71 | ||
72 | If changing an exported interface, please try to deprecate the interface in | |
73 | an orderly manner. If at all possible, try to retain the old deprecated | |
74 | interface as is, or functionally equivalent. Make a note of when the | |
75 | interface was deprecated and guard the deprecated interface definitions in | |
76 | the header file, ie: | |
77 | ||
78 | \begin{verbatim} | |
79 | /* Deprecated: 20050406 */ | |
80 | #if !defined(QUAGGA_NO_DEPRECATED_INTERFACES) | |
81 | #warning "Using deprecated <libname> (interface(s)|function(s))" | |
82 | ... | |
83 | #endif /* QUAGGA_NO_DEPRECATED_INTERFACES */ | |
84 | \end{verbatim} | |
85 | ||
86 | This is to ensure that the core Quagga sources do not use the deprecated | |
87 | interfaces (you should update Quagga sources to use new interfaces, if | |
88 | applicable), while allowing external sources to continue to build. | |
89 | Deprecated interfaces should be excised in the next unstable cycle. | |
90 | ||
91 | Note: If you wish, you can test for GCC and use a function | |
92 | marked with the 'deprecated' attribute. However, you must provide the | |
93 | warning for other compilers. | |
94 | ||
95 | If changing or removing a command definition, \emph{ensure} that you | |
96 | properly deprecate it - use the \_DEPRECATED form of the appropriate DEFUN | |
97 | macro. This is \emph{critical}. Even if the command can no longer | |
98 | function, you \emph{MUST} still implement it as a do-nothing stub. | |
99 | ||
100 | Failure to follow this causes grief for systems administrators, as an | |
101 | upgrade may cause daemons to fail to start because of unrecognised commands. | |
102 | Deprecated commands should be excised in the next unstable cycle. A list of | |
103 | deprecated commands should be collated for each release. | |
104 | ||
105 | See also section~\ref{sec:dll-versioning} below regarding SHARED LIBRARY | |
106 | VERSIONING. | |
107 | ||
108 | ||
109 | \section{COMPILE-TIME CONDITIONAL CODE} | |
110 | ||
111 | Please think very carefully before making code conditional at compile time, | |
112 | as it increases maintenance burdens and user confusion. In particular, | |
113 | please avoid gratuitious --enable-\ldots switches to the configure script - | |
114 | typically code should be good enough to be in Quagga, or it shouldn't be | |
115 | there at all. | |
116 | ||
117 | When code must be compile-time conditional, try have the compiler make it | |
118 | conditional rather than the C pre-processor - so that it will still be | |
119 | checked by the compiler, even if disabled. I.e. this: | |
120 | ||
121 | \begin{verbatim} | |
122 | if (SOME_SYMBOL) | |
123 | frobnicate(); | |
124 | \end{verbatim} | |
125 | ||
126 | rather than: | |
127 | ||
128 | \begin{verbatim} | |
129 | #ifdef SOME_SYMBOL | |
130 | frobnicate (); | |
131 | #endif /* SOME_SYMBOL */ | |
132 | \end{verbatim} | |
133 | ||
134 | Note that the former approach requires ensuring that SOME\_SYMBOL will be | |
135 | defined (watch your AC\_DEFINEs). | |
136 | ||
137 | ||
138 | \section{COMMIT MESSAGES} | |
139 | ||
140 | The commit message requirements are: | |
141 | ||
142 | \begin{itemize} | |
143 | ||
144 | \item The message \emph{MUST} provide a suitable one-line summary followed | |
145 | by a blank line as the very first line of the message, in the form: | |
146 | ||
147 | \verb|topic: high-level, one line summary| | |
148 | ||
149 | Where topic would tend to be name of a subdirectory, and/or daemon, unless | |
150 | there's a more suitable topic (e.g. 'build'). This topic is used to | |
151 | organise change summaries in release announcements. | |
152 | ||
153 | \item It should have a suitable "body", which tries to address the | |
154 | following areas, so as to help reviewers and future browsers of the | |
155 | code-base understand why the change is correct (note also the code | |
156 | comment requirements): | |
157 | ||
158 | \begin{itemize} | |
159 | ||
160 | \item The motivation for the change (does it fix a bug, if so which? | |
161 | add a feature?) | |
162 | ||
163 | \item The general approach taken, and trade-offs versus any other | |
164 | approaches. | |
165 | ||
166 | \item Any testing undertaken or other information affecting the confidence | |
167 | that can be had in the change. | |
168 | ||
169 | \item Information to allow reviewers to be able to tell which specific | |
170 | changes to the code are intended (and hence be able to spot any accidental | |
171 | unintended changes). | |
172 | ||
173 | \end{itemize} | |
174 | \end{itemize} | |
175 | ||
176 | The one-line summary must be limited to 54 characters, and all other | |
177 | lines to 72 characters. | |
178 | ||
179 | Commit message bodies in the Quagga project have typically taken the | |
180 | following form: | |
181 | ||
182 | \begin{itemize} | |
183 | \item An optional introduction, describing the change generally. | |
184 | \item A short description of each specific change made, preferably: | |
185 | \begin{itemize} \item file by file | |
186 | \begin{itemize} \item function by function (use of "ditto", or globs is | |
187 | allowed) | |
188 | \end{itemize} | |
189 | \end{itemize} | |
190 | \end{itemize} | |
191 | ||
192 | Contributors are strongly encouraged to follow this form. | |
193 | ||
194 | This itemised commit messages allows reviewers to have confidence that the | |
195 | author has self-reviewed every line of the patch, as well as providing | |
196 | reviewers a clear index of which changes are intended, and descriptions for | |
197 | them (C-to-english descriptions are not desireable - some discretion is | |
198 | useful). For short patches, a per-function/file break-down may be | |
199 | redundant. For longer patches, such a break-down may be essential. A | |
200 | contrived example (where the general discussion is obviously somewhat | |
201 | redundant, given the one-line summary): | |
202 | ||
203 | \begin{quote}\begin{verbatim} | |
204 | zebra: Enhance frob FSM to detect loss of frob | |
205 | ||
206 | Add a new DOWN state to the frob state machine to allow the barinator to | |
207 | detect loss of frob. | |
208 | ||
209 | * frob.h: (struct frob) Add DOWN state flag. | |
210 | * frob.c: (frob\_change) set/clear DOWN appropriately on state change. | |
211 | * bar.c: (barinate) Check frob for DOWN state. | |
212 | \end{verbatim}\end{quote} | |
213 | ||
214 | Please have a look at the git commit logs to get a feel for what the norms | |
215 | are. | |
216 | ||
217 | Note that the commit message format follows git norms, so that ``git | |
218 | log --oneline'' will have useful output. | |
219 | ||
220 | \section{HACKING THE BUILD SYSTEM} | |
221 | ||
222 | If you change or add to the build system (configure.ac, any Makefile.am, | |
223 | etc.), try to check that the following things still work: | |
224 | ||
225 | \begin{itemize} | |
226 | \item make dist | |
227 | \item resulting dist tarball builds | |
228 | \item out-of-tree builds | |
229 | \end{itemize} | |
230 | ||
231 | The quagga.net site relies on make dist to work to generate snapshots. It | |
232 | must work. Common problems are to forget to have some additional file | |
233 | included in the dist, or to have a make rule refer to a source file without | |
234 | using the srcdir variable. | |
235 | ||
236 | ||
237 | \section{RELEASE PROCEDURE} | |
238 | ||
239 | \begin{itemize} | |
240 | \item Tag the apppropriate commit with a release tag (follow existing | |
241 | conventions). | |
242 | ||
243 | [This enables recreating the release, and is just good CM practice.] | |
244 | ||
245 | \item Create a fresh tar archive of the quagga.net repository, and do a test | |
246 | build: | |
247 | ||
248 | \begin{verbatim} | |
249 | git-clone git:///code.quagga.net/quagga.git quagga | |
250 | git-archive --remote=git://code.quagga.net/quagga.git \ | |
251 | --prefix=quagga-release/ master | tar -xf - | |
252 | cd quagga-release | |
253 | ||
254 | autoreconf -i | |
255 | ./configure | |
256 | make | |
257 | make dist | |
258 | \end{verbatim} | |
259 | \end{itemize} | |
260 | ||
261 | The tarball which `make dist' creates is the tarball to be released! The | |
262 | git-archive step ensures you're working with code corresponding to that in | |
263 | the official repository, and also carries out keyword expansion. If any | |
264 | errors occur, move tags as needed and start over from the fresh checkouts. | |
265 | Do not append to tarballs, as this has produced non-standards-conforming | |
266 | tarballs in the past. | |
267 | ||
268 | See also: \url{http://wiki.quagga.net/index.php/Main/Processes} | |
269 | ||
270 | [TODO: collation of a list of deprecated commands. Possibly can be scripted | |
271 | to extract from vtysh/vtysh\_cmd.c] | |
272 | ||
273 | ||
274 | \section{TOOL VERSIONS} | |
275 | ||
276 | Require versions of support tools are listed in INSTALL.quagga.txt. | |
277 | Required versions should only be done with due deliberation, as it can | |
278 | cause environments to no longer be able to compile quagga. | |
279 | ||
280 | ||
281 | \section{SHARED LIBRARY VERSIONING} | |
282 | \label{sec:dll-versioning} | |
283 | ||
284 | [this section is at the moment just gdt's opinion] | |
285 | ||
286 | Quagga builds several shared libaries (lib/libzebra, ospfd/libospf, | |
287 | ospfclient/libsopfapiclient). These may be used by external programs, | |
288 | e.g. a new routing protocol that works with the zebra daemon, or | |
289 | ospfapi clients. The libtool info pages (node Versioning) explain | |
290 | when major and minor version numbers should be changed. These values | |
291 | are set in Makefile.am near the definition of the library. If you | |
292 | make a change that requires changing the shared library version, | |
293 | please update Makefile.am. | |
294 | ||
295 | libospf exports far more than it should, and is needed by ospfapi | |
296 | clients. Only bump libospf for changes to functions for which it is | |
297 | reasonable for a user of ospfapi to call, and please err on the side | |
298 | of not bumping. | |
299 | ||
300 | There is no support intended for installing part of zebra. The core | |
301 | library libzebra and the included daemons should always be built and | |
302 | installed together. | |
303 | ||
304 | ||
305 | \section{GIT COMMIT SUBMISSION} | |
306 | \label{sec:git-submission} | |
307 | ||
308 | The preferred method for submitting changes is to provide git commits via a | |
309 | publically-accessible git repository, which the maintainers can easily pull. | |
310 | ||
311 | The commits should be in a branch based off the Quagga.net master - a | |
312 | "feature branch". Ideally there should be no commits to this branch other | |
313 | than those in master, and those intended to be submitted. However, merge | |
314 | commits to this branch from the Quagga master are permitted, though strongly | |
315 | discouraged - use another (potentially local and throw-away) branch to test | |
316 | merge with the latest Quagga master. | |
317 | ||
318 | Recommended practice is to keep different logical sets of changes on | |
319 | separate branches - "topic" or "feature" branches. This allows you to still | |
320 | merge them together to one branch (potentially local and/or "throw-away") | |
321 | for testing or use, while retaining smaller, independent branches that are | |
322 | easier to merge. | |
323 | ||
324 | All content guidelines in section \ref{sec:patch-submission}, PATCH | |
325 | SUBMISSION apply. | |
326 | ||
327 | ||
328 | \section{PATCH SUBMISSION} | |
329 | \label{sec:patch-submission} | |
330 | ||
331 | \begin{itemize} | |
332 | ||
333 | \item For complex changes, contributors are strongly encouraged to first | |
334 | start a design discussion on the quagga-dev list \emph{before} | |
335 | starting any coding. | |
336 | ||
337 | \item Send a clean diff against the 'master' branch of the quagga.git | |
338 | repository, in unified diff format, preferably with the '-p' argument to | |
339 | show C function affected by any chunk, and with the -w and -b arguments to | |
340 | minimise changes. E.g: | |
341 | ||
342 | git diff -up mybranch..remotes/quagga.net/master | |
343 | ||
344 | It is preferable to use git format-patch, and even more preferred to | |
345 | publish a git repository (see GIT COMMIT SUBMISSION, section | |
346 | \ref{sec:git-submission}). | |
347 | ||
348 | If not using git format-patch, Include the commit message in the email. | |
349 | ||
350 | \item After a commit, code should have comments explaining to the reviewer | |
351 | why it is correct, without reference to history. The commit message | |
352 | should explain why the change is correct. | |
353 | ||
354 | \item Include NEWS entries as appropriate. | |
355 | ||
356 | \item Include only one semantic change or group of changes per patch. | |
357 | ||
358 | \item Do not make gratuitous changes to whitespace. See the w and b arguments | |
359 | to diff. | |
360 | ||
361 | \item Changes should be arranged so that the least contraversial and most | |
362 | trivial are first, and the most complex or more contraversial are | |
363 | last. This will maximise how many the Quagga maintainers can merge, | |
364 | even if some other commits need further work. | |
365 | ||
366 | \item Providing a unit-test is strongly encouraged. Doing so will make it | |
367 | much easier for maintainers to have confidence that they will be able | |
368 | to support your change. | |
369 | ||
370 | \item New code should be arranged so that it easy to verify and test. E.g. | |
371 | stateful logic should be separated out from functional logic as much as | |
372 | possible: wherever possible, move complex logic out to smaller helper | |
373 | functions which access no state other than their arguments. | |
374 | ||
375 | \item State on which platforms and with what daemons the patch has been | |
376 | tested. Understand that if the set of testing locations is small, | |
377 | and the patch might have unforeseen or hard to fix consequences that | |
378 | there may be a call for testers on quagga-dev, and that the patch | |
379 | may be blocked until test results appear. | |
380 | ||
381 | If there are no users for a platform on quagga-dev who are able and | |
382 | willing to verify -current occasionally, that platform may be | |
383 | dropped from the "should be checked" list. | |
384 | ||
385 | \end{itemize} | |
386 | ||
387 | \section{PATCH APPLICATION} | |
388 | ||
389 | \begin{itemize} | |
390 | ||
391 | \item Only apply patches that meet the submission guidelines. | |
392 | ||
393 | \item If the patch might break something, issue a call for testing on the | |
394 | mailinglist. | |
395 | ||
396 | \item Give an appropriate commit message (see above), and use the --author | |
397 | argument to git-commit, if required, to ensure proper attribution (you | |
398 | should still be listed as committer) | |
399 | ||
400 | \item Immediately after commiting, double-check (with git-log and/or gitk). | |
401 | If there's a small mistake you can easily fix it with `git commit | |
402 | --amend ..' | |
403 | ||
404 | \item When merging a branch, always use an explicit merge commit. Giving | |
405 | --no-ff ensures a merge commit is created which documents ``this human | |
406 | decided to merge this branch at this time''. | |
407 | \end{itemize} | |
408 | ||
409 | \section{STABLE PLATFORMS AND DAEMONS} | |
410 | ||
411 | The list of platforms that should be tested follow. This is a list | |
412 | derived from what quagga is thought to run on and for which | |
413 | maintainers can test or there are people on quagga-dev who are able | |
414 | and willing to verify that -current does or does not work correctly. | |
415 | ||
416 | \begin{itemize} | |
417 | \item BSD (Free, Net or Open, any platform) | |
418 | \item GNU/Linux (any distribution, i386) | |
419 | \item Solaris (strict alignment, any platform) | |
420 | \item future: NetBSD/sparc64 | |
421 | \end{itemize} | |
422 | ||
423 | The list of daemons that are thought to be stable and that should be | |
424 | tested are: | |
425 | ||
426 | \begin{itemize} | |
427 | \item zebra | |
428 | \item bgpd | |
429 | \item ripd | |
430 | \item ospfd | |
431 | \item ripngd | |
432 | \end{itemize} | |
433 | Daemons which are in a testing phase are | |
434 | ||
435 | \begin{itemize} | |
436 | \item ospf6d | |
437 | \item isisd | |
438 | \item watchquagga | |
439 | \end{itemize} | |
440 | ||
441 | \section{IMPORT OR UPDATE VENDOR SPECIFIC ROUTING PROTOCOLS} | |
442 | ||
443 | The source code of Quagga is based on two vendors: | |
444 | ||
445 | \verb|zebra_org| (\url{http://www.zebra.org/}) | |
446 | \verb|isisd_sf| (\url{http://isisd.sf.net/}) | |
447 | ||
448 | To import code from further sources, e.g. for archival purposes without | |
449 | necessarily having to review and/or fix some changeset, create a branch from | |
450 | `master': | |
451 | ||
452 | \begin{verbatim} | |
453 | git checkout -b archive/foo master | |
454 | <apply changes> | |
455 | git commit -a "Joe Bar <joe@example.com>" | |
456 | git push quagga archive/foo | |
457 | \end{verbatim} | |
458 | ||
459 | presuming `quagga' corresponds to a file in your .git/remotes with | |
460 | configuration for the appropriate Quagga.net repository. | |
461 | ||
462 | \end{document} |