Project

General

Profile

Coding standards » History » Version 33

neels, 01/15/2018 01:37 PM

1 9 laforge
{{>toc}}
2
3 8 laforge
h1. Osmocom coding standards
4 1
5 10 laforge
h2. Coding Style
6 8 laforge
7
In general, for the C-language projects we maintain in the Osmocom project, we follow the coding style
8 27 blobb
of the Linux kernel. See https://www.kernel.org/doc/html/latest/process/coding-style.html for further information. The sole exception to this is that we accept line lengths of up to 120 characters (Linux kernel requires 80 character limit). 
9 8 laforge
10 27 blobb
We will not debate about the coding style and whether some other style is better or worse. The important factor is not so much _which_ coding style is followed, as long as there is one well-defined coding style and that all developers adhere to it.
11 10 laforge
12 31 neels
See also our [[Guidelines for API documentation]].
13
14 10 laforge
h2. Development Process
15
16
Once again, the Osmocom project follows processes defined for the Linux kernel development whenever applicable.  Of course, as Osmocom is  much smaller project, some simplifications apply.
17
18 28 blobb
For further information on the Linux kernel development process, please see https://www.kernel.org/doc/html/latest/process/development-process.html
19 10 laforge
20
The most important parts are summarized below:
21
22
h2. The lifecycle of a patch
23
24 13 laforge
Patches do not go directly from the developer's keyboard into the mainline project.  There is, instead, a somewhat involved (if somewhat informal) process designed to ensure that each patch is reviewed for quality and that each patch implements a change which is desirable to have in the mainline. This process can happen quickly for minor fixes, or, in the case of large and controversial changes, go on for years.  Much developer frustration comes from a lack of understanding of this process or from attempts to circumvent it.
25 10 laforge
26
In the hopes of reducing that frustration, this document will describe how a patch gets into the kernel.  What follows below is an introduction which describes the process in a somewhat idealized way.  A much more detailed
27
treatment will come in later sections.
28
29
The stages that a patch goes through are, generally:
30
31
* *Design*. This is where the real requirements for the patch - and the way those requirements will be met - are laid out.  Design work is often done without involving the community, but it is better to do this work in the open if at all possible; it can save a lot of time redesigning things later.
32
33 19 neels
* *Review*.  Patches are posted to gerrit, and developers with any comments they may have; for projects not on gerrit.osmocom.org, patches and comments are posted to the appropriate mailing list.  This process should turn up any major problems with a patch if all goes well. Please note that most maintainers also have day jobs, so merging your patch may not be their highest priority.  If your patch is getting feedback about changes that are needed, you should either make those changes or justify why they should not be made.  If your patch has no review complaints but is not being merged by its appropriate maintainer, you should be persistent in updating the patch to the current master versoin so that it applies cleanly and keep sending it for review and merging.
34 10 laforge
35 19 neels
* *Merging into the mainline*.  Eventually, a successful patch will be merged into the mainline repository.  See [[Gerrit#Merge-patch-to-master|Gerrit - Merge patch to master]] for details on review and merging on our gerrit.  More comments and/or problems may surface at this time; it is important that the developer be responsive to these and fix any issues which arise.
36 10 laforge
37
* *Long-term maintenance*.  While it is certainly possible for a developer to forget about code after merging it, that sort of behavior tends to leave a poor impression in the development community.  Merging code eliminates some of the maintenance burden, in that others will fix problems caused by API changes.  But the original developer should continue to take responsibility for the code if it is to remain useful in the longer term.
38
39
One of the largest mistakes made by developers (or their employers) is to try to cut the process down to a single *merging into the mainline* step.  This approach invariably leads to frustration for everybody involved.
40
41
42
h3. The importance of submitting your changes to mainline
43
44
Some companies and developers occasionally wonder why they should bother learning how to work with the Osmocom community and get their code into mainline (the "mainline" being the software maintained by the Osmocom team at git.osmocom.org).  In the short term, contributing code can look like an avoidable expense; it seems easier to
45
just keep the code separate and support users directly.  The truth of the matter is that keeping code separate ("out of tree") is a false economy.
46
47
As a way of illustrating the costs of out-of-tree code, here are a few relevant aspects of the kernel development process; most of these will be discussed in greater detail later in this document.  Consider:
48
49
* Code which has been merged into the mainline kernel is available to all users.  There is no need for vendor-specific drivers/builds, downloads, or the hassles of supporting multiple versions of multiple distributions; it all just works, for the developer and for the user.  Incorporation into the mainline solves a large number of distribution and support problems.
50
51
* The internal API of the projects is in constant flux.  The lack of a stable internal interface is a deliberate design decision; it allows fundamental improvements to be made at any time and results in higher-quality code. But one result of that policy is that any out-of-tree code requires constant upkeep if it is to work with new kernels.
52
 
53
Maintaining out-of-tree code requires significant amounts of work just to keep that code working. Code which is in the mainline, instead, does not require this work as the result of a simple rule requiring any developer who makes an API change to also fix any code that breaks as the result of that change.  So code which has been merged into the mainline has significantly lower maintenance costs.
54
55
* Beyond that, code which is in mainline will often be improved by other developers.  Surprising results can come from empowering your user community and customers to improve your product.
56
57
* Code is subjected to review, both before and after merging into the mainline.  No matter how strong the original developer's skills are, this review process invariably finds ways in which the code can be improved.  Often review finds severe bugs and security problems.  This is especially true for code which has been developed in a closed environment; such code benefits strongly from review by outside developers.  Out-of-tree code is lower-quality code.
58
59
* Participation in the development process is your way to influence the direction of the projects development.  Users who complain from the sidelines are heard, but active developers have a stronger voice - and the ability to implement changes which make the kernel work better for their needs.
60
61
* When code is maintained separately, the possibility that a third party will contribute a different implementation of a similar feature always exists.  Should that happen, getting your code merged will become much harder - to the point of impossibility.  Then you will be faced with the unpleasant alternatives of either (1) maintaining a nonstandard features out of tree indefinitely, or (2) abandoning your code and migrating your users over to the in-tree version.
62
63
* Contribution of code is the fundamental action which makes the whole process work.  By contributing your code you can add new functionality to the project and provide capabilities and examples which are of use to other developers.  If you have developed code for a project (or are thinking about doing so), you clearly have an interest in the continued success of this platform; contributing code is one of the best ways to help ensure that success.
64
65
h2. Licensing
66
67 29 blobb
Code is contributed to any Osmocom project under a number of licenses, but all code must be compatible with the respective project license.  This typically is the GNU Affero General Public License, Version 3 (AGPLv3) or Version 2 of the GNU General Public License (GPLv2). Any contributions which are not covered by a compatible license cannot be accepted.
68 1
69 10 laforge
h2. Submitting Patches
70 13 laforge
71 15 neels
Before you send a patch, please read all of the guidelines below.
72 1
73 16 neels
How to submit patches for Osmocom projects depends on the maintainer. Most actively maintained Osmocom projects ("see this list":https://gerrit.osmocom.org/#/admin/projects/) have switched to a Gerrit based submission + review process, see [[Gerrit]] for more details.
74 15 neels
75
For all other projects, we accept patch submissions sent to the appropriate "mailing list":https://lists.osmocom.org/mailman/listinfo.
76
77
Note: Osmocom projects are mirrored on github, but all and any pull requests will be rejected by a bot.
78 1
79 30 blobb
For submitting patches, the best practises established by the Linux kernel development community should be followed, wherever applicable. See https://www.kernel.org/doc/html/latest/process/5.Posting.html for further information.
80 12 neels
81 8 laforge
The generally important topics are:
82
83
h3. Submit patches against a current source tree
84
85
At time of submission, our patches should be based on current @master@ branch of the respective project, or another (more current) feature branch of that repository.
86
87
h3. Use unified diff format
88
89 20 neels
In case of gerrit submissions, using 'git push' or 'git-review' will ensure the correct format.
90
91
In case of mailing list submissions, it's best to let git generate the patches: preferably use 'git send-email' (possibly with 'git format-patch' first).
92 8 laforge
93
h3. Have meaningful descriptions in your patches
94
95 21 neels
Every commit/patch should have an explanation in the commit message, that explains intent and method of the patch efficiently and in a way that the changes can be easily understood without having to guess.
96
97
You should stick to the imperative form -- e.g. "add foo", "fix bar", "drop baz" -- and avoid empty phrasing like "this patch implements". This is shortest to write and, more importantly, most efficient to read for reviewers.
98
99 33 neels
The first line is a summary, separated by a blank line from the rest. It should be kept brief, and *always* name the general area of the code modified: <pre>vty: fix OML link state printing</pre>
100 21 neels
If the patch contains no functional changes, prepend 'cosmetic:' to the summary. For example: <pre>cosmetic: foo.c: drop unused bar()</pre> or <pre>cosmetic: baz.c: rename moo() to goo()</pre>
101
102
Do not re-state the obvious, but explain the context:
103
104 22 neels
* If the patch is related to a redmine issue on osmocom.org, include an issue tag like OS#1234. *Always* use this form, never omit the 'OS', so that we can machine parse these references. For example: <pre>Related: OS#1234</pre>
105 33 neels
* If the patch depends on another patch (e.g. in another repository), include the other patch's gerrit change-id. For example, include a line <pre>Depends: I0fca8e95ed5c2148b1a7440eff3fc9c7583898df (libosmocore)</pre>
106 21 neels
* If fixing an issue reported by coverity scan, include the CID, e.g. include a line: <pre>Fixes: coverity scan CID#99999</pre>
107
* If fixing an error, include a 1:1 paste of the error message fixed and/or explain the scenario.
108
* If adding a feature, explain the use case.
109
110
For commit log examples, see https://cgit.osmocom.org/libosmocore/log/
111 8 laforge
112
h3. Separate your changes
113
114 11 zecke
Separate each _logical change_ into a separate patch. Do not mix different concerns (e.g. do not add functionality and change unrelated functionality at the same time, not even coding style clean-ups)
115 1
116 11 zecke
h3. Have small methods and allow for unit testing
117
118
Large methods are difficult to read (as they don't fit to the screen), large methods have the tendency to do too many things. Try to split them up so that parts can be tested separately. If your code is parsing a message and then doing something with the result in most cases it is better to separate this (fill a struct with the parse result). This allows to test the parser without a big test fixture. Try to do the same for the decision handling...
119
120
h3. Update test results
121
122
If your code is changing test results, have a look at the change and if necessary update it.
123
124 8 laforge
h3. Respond to review comments
125 11 zecke
126 25 neels
h3. On mailing lists, include PATCH in the email subject
127 8 laforge
128
h2. Rules of thumb
129
130
Some general rules of thumb:
131
* Consider using tools like @Lindent@ and @checkpatch.pl@ or @universalindentgui@ before submitting code
132
* Run @make@, @make check@ and @make distcheck@ on each of your commits to ensure the build
133 23 neels
* For mailing lists, use @git send-email@ or @git imap-send@ to send your patches (otherwise your mail client may wrap around lines and corrupt your patch)
134 17 laforge
135
h2. Naming conventions
136
137
For new code, we generally prefer the following naming conventions:
138
139
* symbols (functions, global static data)
140
** always use the *osmo_* prefix for symbols exported by libraries
141 24 neels
** never use the *osmo_* prefix for symbols inside applications (which avoids a naming conflict in case we choose to later move code from an application to a library, adding @osmo_@ in the process)
142 17 laforge
* libraries
143
** use *libosmo-* prefix for library name
144
* executables
145
** use *osmo-* name prefix for executable programs
Add picture from clipboard (Maximum size: 48.8 MB)