Emeric Vigier | 2f62582 | 2012-08-06 11:09:52 -0400 | [diff] [blame] | 1 | The guidelines in this file are the ideals; it's better to send a |
| 2 | not-fully-following-guidelines patch than no patch at all, though. We |
| 3 | can always polish it up. |
| 4 | |
| 5 | Mailing list |
| 6 | === |
| 7 | |
| 8 | The D-Bus mailing list is dbus@lists.freedesktop.org; discussion |
| 9 | of patches, etc. should go there. |
| 10 | |
| 11 | Security |
| 12 | === |
| 13 | |
| 14 | Most of D-Bus is security sensitive. Guidelines related to that: |
| 15 | |
| 16 | - avoid memcpy(), sprintf(), strlen(), snprintf, strlcat(), |
| 17 | strstr(), strtok(), or any of this stuff. Use DBusString. |
| 18 | If DBusString doesn't have the feature you need, add it |
| 19 | to DBusString. |
| 20 | |
| 21 | There are some exceptions, for example |
| 22 | if your strings are just used to index a hash table |
| 23 | and you don't do any parsing/modification of them, perhaps |
| 24 | DBusString is wasteful and wouldn't help much. But definitely |
| 25 | if you're doing any parsing, reallocation, etc. use DBusString. |
| 26 | |
| 27 | - do not include system headers outside of dbus-memory.c, |
| 28 | dbus-sysdeps.c, and other places where they are already |
| 29 | included. This gives us one place to audit all external |
| 30 | dependencies on features in libc, etc. |
| 31 | |
| 32 | - do not use libc features that are "complicated" |
| 33 | and may contain security holes. For example, you probably shouldn't |
| 34 | try to use regcomp() to compile an untrusted regular expression. |
| 35 | Regular expressions are just too complicated, and there are many |
| 36 | different libc's out there. |
| 37 | |
| 38 | - we need to design the message bus daemon (and any similar features) |
| 39 | to use limited privileges, run in a chroot jail, and so on. |
| 40 | |
| 41 | http://vsftpd.beasts.org/ has other good security suggestions. |
| 42 | |
| 43 | Coding Style |
| 44 | === |
| 45 | |
| 46 | - The C library uses GNU coding conventions, with GLib-like |
| 47 | extensions (e.g. lining up function arguments). The |
| 48 | Qt wrapper uses KDE coding conventions. |
| 49 | |
| 50 | - Write docs for all non-static functions and structs and so on. try |
| 51 | "doxygen Doxyfile" prior to commit and be sure there are no |
| 52 | warnings printed. |
| 53 | |
| 54 | - All external interfaces (network protocols, file formats, etc.) |
| 55 | should have documented specifications sufficient to allow an |
| 56 | alternative implementation to be written. Our implementation should |
| 57 | be strict about specification compliance (should not for example |
| 58 | heuristically parse a file and accept not-well-formed |
| 59 | data). Avoiding heuristics is also important for security reasons; |
| 60 | if it looks funny, ignore it (or exit, or disconnect). |
| 61 | |
| 62 | Development |
| 63 | === |
| 64 | |
| 65 | D-Bus uses Git as its version control system. The main repository is |
| 66 | hosted at git.freedesktop.org/dbus/dbus. To clone D-Bus, execute the |
| 67 | following command: |
| 68 | |
| 69 | git clone git://git.freedesktop.org/dbus/dbus |
| 70 | OR |
| 71 | git clone git.freedesktop.org:dbus/dbus |
| 72 | |
| 73 | The latter form is the one that allows pushing, but it also requires |
| 74 | an SSH account on the server. The former form allows anonymous |
| 75 | checkouts. |
| 76 | |
| 77 | D-Bus development happens in two branches in parallel: the current |
| 78 | stable branch, with an even minor number (like 1.0, 1.2 and 1.4), and |
| 79 | the next development branch, with the next odd number. |
| 80 | |
| 81 | The stable branch is named after the version number itself (dbus-1.2, |
| 82 | dbus-1.4), whereas the development branch is simply known as "master". |
| 83 | |
| 84 | When making a change to D-Bus, do the following: |
| 85 | |
| 86 | - check out the earliest branch of D-Bus that makes sense to have |
| 87 | your change in. If it's a bugfix, it's normally the current stable |
| 88 | branch; if it's a feature, it's normally the "master" branch. If |
| 89 | you have an important security fix, you may want to apply to older |
| 90 | branches too. |
| 91 | |
| 92 | - for large changes: |
| 93 | if you're developing a new, large feature, it's recommended |
| 94 | to create a new branch and do your development there. Publish |
| 95 | your branch at a suitable place and ask others to help you |
| 96 | develop and test it. Once your feature is considered finalised, |
| 97 | you may merge it into the "master" branch. |
| 98 | |
| 99 | - for small changes: |
| 100 | . make your change to the source code |
| 101 | . execute tests to guarantee that you're not introducing a |
| 102 | regression. For that, execute: make check |
| 103 | (if possible, add a new test to check the fix you're |
| 104 | introducing) |
| 105 | . commit your change using "git commit" |
| 106 | in the commit message, write a short sentence describing what |
| 107 | you did in the first line. Then write a longer description in |
| 108 | the next paragraph(s). |
| 109 | . repeat the previous steps if necessary to have multiple commits |
| 110 | |
| 111 | - extract your patches and send to the D-Bus mailing list for |
| 112 | review or post them to the D-Bus Bugzilla, attaching them to a bug |
| 113 | report. To extract the patches, execute: |
| 114 | git format-patch origin/master |
| 115 | |
| 116 | - once your code has been reviewed, you may push it to the Git |
| 117 | server: |
| 118 | git push origin my-branch:remote |
| 119 | OR |
| 120 | git push origin dbus-X.Y |
| 121 | OR |
| 122 | git push origin master |
| 123 | (consult the Git manual to know which command applies) |
| 124 | |
| 125 | - (Optional) if you've not worked on "master", merge your changes to |
| 126 | that branch. If you've worked on an earlier branch than the current |
| 127 | stable, merge your changes upwards towards the stable branch, then |
| 128 | from there into "master". |
| 129 | |
| 130 | . execute: git checkout master |
| 131 | . ensure that you have the latest "master" from the server, update |
| 132 | if you don't |
| 133 | . execute: git merge dbus-X.Y |
| 134 | . if you have any conflicts, resolve them, git add the conflicted |
| 135 | files and then git commit |
| 136 | . push the "master" branch to the server as well |
| 137 | |
| 138 | Executing this merge is recommended, but not necessary for all |
| 139 | changes. You should do this step if your bugfix is critical for the |
| 140 | development in "master", or if you suspect that conflicts will arise |
| 141 | (you're usually the best person to resolve conflicts introduced by |
| 142 | your own code), or if it has been too long since the last merge. |
| 143 | |
| 144 | |
| 145 | Making a release |
| 146 | === |
| 147 | |
| 148 | To make a release of D-Bus, do the following: |
| 149 | |
| 150 | - check out a fresh copy from Git |
| 151 | |
| 152 | - verify that the libtool versioning/library soname is |
| 153 | changed if it needs to be, or not changed if not |
| 154 | |
| 155 | - update the file NEWS based on the ChangeLog |
| 156 | |
| 157 | - update the AUTHORS file based on the ChangeLog |
| 158 | |
| 159 | - add a ChangeLog entry containing the version number |
| 160 | you're releasing ("Released 0.3" or something) |
| 161 | so people can see which changes were before and after |
| 162 | a given release |
| 163 | |
| 164 | - the version number should have major.minor.micro even |
| 165 | if micro is 0, i.e. "1.0.0" and "1.2.0" not "1.0"/"1.2" |
| 166 | |
| 167 | - "make distcheck" (DO NOT just "make dist" - pass the check!) |
| 168 | |
| 169 | - if make distcheck fails, fix it. |
| 170 | |
| 171 | - once distcheck succeeds, "git commit -a". This is the version |
| 172 | of the tree that corresponds exactly to the released tarball. |
| 173 | |
| 174 | - tag the tree with "git tag -s -m 'Released X.Y.Z' dbus-X.Y.Z" |
| 175 | where X.Y.Z is the version of the release. If you can't sign |
| 176 | then simply created an unsigned annotated tag: |
| 177 | "git tag -a -m 'Released X.Y.Z' dbus-X.Y.Z". |
| 178 | |
| 179 | - bump the version number up in configure.in, and commit |
| 180 | it. Make sure you do this *after* tagging the previous |
| 181 | release! The idea is that git has a newer version number |
| 182 | than anything released. |
| 183 | |
| 184 | - merge the branch you've released to the chronologically-later |
| 185 | branch (usually "master"). You'll probably have to fix a merge |
| 186 | conflict in configure.in (the version number). |
| 187 | |
| 188 | - push your changes and the tag to the central repository with |
| 189 | git push origin master dbus-X.Y dbus-X.Y.Z |
| 190 | |
| 191 | - scp your tarball to freedesktop.org server and copy it to |
| 192 | dbus.freedesktop.org:/srv/dbus.freedesktop.org/www/releases/dbus/dbus-X.Y.Z.tar.gz. |
| 193 | This should be possible if you're in group "dbus" |
| 194 | |
| 195 | - update the wiki page http://www.freedesktop.org/Software/dbus by |
| 196 | adding the new release under the Download heading. Then, cut the |
| 197 | link and changelog for the previous that was there. |
| 198 | |
| 199 | - update the wiki page |
| 200 | http://www.freedesktop.org/Software/DbusReleaseArchive pasting the |
| 201 | previous release. Note that bullet points for each of the changelog |
| 202 | items must be indented three more spaces to conform to the |
| 203 | formatting of the other releases there. |
| 204 | |
| 205 | - post to dbus@lists.freedesktop.org announcing the release. |
| 206 | |
| 207 | |
| 208 | After making a ".0" stable release |
| 209 | === |
| 210 | |
| 211 | After releasing, when you increment the version number in git, also |
| 212 | move the ChangeLog to ChangeLog.pre-X-Y where X-Y is what you just |
| 213 | released, e.g. ChangeLog.pre-1-0. Then create and cvs add a new empty |
| 214 | ChangeLog. The last entry in ChangeLog.pre-1-0 should be the one about |
| 215 | "Released 1.0". |
| 216 | |
| 217 | Add ChangeLog.pre-X-Y to EXTRA_DIST in Makefile.am. |
| 218 | |
| 219 | We create a branch for each stable release; sometimes the branch is |
| 220 | not done immediately, instead it's possible to wait until someone has |
| 221 | a not-suitable-for-stable change they want to make and then branch to |
| 222 | allow committing that change. |
| 223 | |
| 224 | The branch name should be dbus-X.Y-branch which is a branch that has |
| 225 | releases versioned X.Y.Z |
| 226 | |
| 227 | To branch: |
| 228 | git branch dbus-X.Y-branch |
| 229 | and upload the branch tag to the server: |
| 230 | git-push origin dbus-X.Y-branch |
| 231 | |
| 232 | To develop in this branch: |
| 233 | git-checkout dbus-X.Y-branch |
| 234 | |
| 235 | Environment variables |
| 236 | === |
| 237 | |
| 238 | These are the environment variables that are used by the D-Bus client library |
| 239 | |
| 240 | DBUS_VERBOSE=1 |
| 241 | Turns on printing verbose messages. This only works if D-Bus has been |
| 242 | compiled with --enable-verbose-mode |
| 243 | |
| 244 | DBUS_MALLOC_FAIL_NTH=n |
| 245 | Can be set to a number, causing every nth call to dbus_alloc or |
| 246 | dbus_realloc to fail. This only works if D-Bus has been compiled with |
| 247 | --enable-tests. |
| 248 | |
| 249 | DBUS_MALLOC_FAIL_GREATER_THAN=n |
| 250 | Can be set to a number, causing every call to dbus_alloc or |
| 251 | dbus_realloc to fail if the number of bytes to be allocated is greater |
| 252 | than the specified number. This only works if D-Bus has been compiled with |
| 253 | --enable-tests. |
| 254 | |
| 255 | DBUS_TEST_MALLOC_FAILURES=n |
| 256 | Many of the D-Bus tests will run over and over, once for each malloc |
| 257 | involved in the test. Each run will fail a different malloc, plus some |
| 258 | number of mallocs following that malloc (because a fair number of bugs |
| 259 | only happen if two or more mallocs fail in a row, e.g. error recovery |
| 260 | that itself involves malloc). This env variable sets the number of |
| 261 | mallocs to fail. |
| 262 | Here's why you care: If set to 0, then the malloc checking is skipped, |
| 263 | which makes the test suite a heck of a lot faster. Just run with this |
| 264 | env variable unset before you commit. |
| 265 | |
| 266 | Tests |
| 267 | === |
| 268 | |
| 269 | These are the test programs that are built if dbus is compiled using |
| 270 | --enable-tests. |
| 271 | |
| 272 | dbus/dbus-test |
| 273 | This is the main unit test program that tests all aspects of the D-Bus |
| 274 | client library. |
| 275 | |
| 276 | dbus/bus-test |
| 277 | This it the unit test program for the message bus. |
| 278 | |
| 279 | test/break-loader |
| 280 | A test that tries to break the message loader by passing it randomly |
| 281 | created invalid messages. |
| 282 | |
| 283 | test/name-test/* |
| 284 | This is a suite of programs which are run with a temporary session bus. |
| 285 | If your test involves multiple processes communicating, your best bet |
| 286 | is to add a test in here. |
| 287 | |
| 288 | "make check" runs all the deterministic test programs (i.e. not break-loader). |
| 289 | |
| 290 | "make check-coverage" is available if you configure with --enable-gcov and |
| 291 | gives a complete report on test suite coverage. You can also run |
| 292 | "test/decode-gcov foo.c" on any source file to get annotated source, |
| 293 | after running make check with a gcov-enabled tree. |
| 294 | |
| 295 | Patches |
| 296 | === |
| 297 | |
| 298 | Please file them at http://bugzilla.freedesktop.org under component |
| 299 | dbus, and also post to the mailing list for discussion. The commit |
| 300 | rules are: |
| 301 | |
| 302 | - for fixes that don't affect API or protocol, they can be committed |
| 303 | if any one qualified reviewer other than patch author |
| 304 | reviews and approves |
| 305 | |
| 306 | - for fixes that do affect API or protocol, two people |
| 307 | in the reviewer group have to review and approve the commit, and |
| 308 | posting to the list is definitely mandatory |
| 309 | |
| 310 | - if there's a live unresolved controversy about a change, |
| 311 | don't commit it while the argument is still raging. |
| 312 | |
| 313 | - regardless of reviews, to commit a patch: |
| 314 | - make check must pass |
| 315 | - the test suite must be extended to cover the new code |
| 316 | as much as reasonably feasible (see Tests above) |
| 317 | - the patch has to follow the portability, security, and |
| 318 | style guidelines |
| 319 | - the patch should as much as reasonable do one thing, |
| 320 | not many unrelated changes |
| 321 | No reviewer should approve a patch without these attributes, and |
| 322 | failure on these points is grounds for reverting the patch. |
| 323 | |
| 324 | The reviewer group that can approve patches: |
| 325 | |
| 326 | Havoc Pennington <hp@pobox.net> |
| 327 | Michael Meeks <michael.meeks@novell.com> |
| 328 | Alexander Larsson <alexl@redhat.com> |
| 329 | Zack Rusin <zack@kde.org> |
| 330 | Joe Shaw <joe@assbarn.com> |
| 331 | Mikael Hallendal <micke@imendio.com> |
| 332 | Richard Hult <richard@imendio.com> |
| 333 | Owen Fraser-Green <owen@discobabe.net> |
| 334 | Olivier Andrieu <oliv__a@users.sourceforge.net> |
| 335 | Colin Walters <walters@verbum.org> |
| 336 | Thiago Macieira <thiago@kde.org> |
| 337 | John Palmieri <johnp@redhat.com> |
| 338 | Scott James Remnant <scott@netsplit.com> |
| 339 | Will Thompson <will.thompson@collabora.co.uk> |
| 340 | Simon McVittie <simon.mcvittie@collabora.co.uk> |
| 341 | |
| 342 | |