Differences between revisions 3 and 4
Revision 3 as of 2007-09-25 11:05:03
Size: 5684
Comment:
Revision 4 as of 2008-01-10 19:42:38
Size: 5684
Editor: localhost
Comment: converted to 1.6 markup
No differences found!

Summary

The following guidelines are designed to speed up review of patches for device-mapper kernel code.

  • Follow existing device-mapper and kernel coding style.
  • Create small and self-contained patches.
  • Document new code and ideas within the source code tree.
  • Provide wider information about the patch in the patch header.

Coding-style

In general, patches get reviewed for coding style before anything else because a consistent coding style aids readability. Follow existing device-mapper and kernel coding style.

  1. Limit lines to 80 characters (rare exceptions).
  2. Use 8 character-wide tabs.
  3. Avoid trailing whitespace on lines.
  4. Never have two or more adjacent blank lines.
  5. Never use unnecessary braces.
  6. In 'if (X) {A} else {B}' conditions, X should be visible while reading B, and A should normally take up fewer lines than B.
  7. Keep functions short with a single logical purpose.
  8. Define all variables at the start of functions. If you find yourself wanting to define a local variable within a for loop, for example, it's probably an indication that you should be using a separate function.
  9. Avoid lots of nesting within functions: split into multiple functions or use 'goto label'.
  10. Use descriptive labels - avoid numbering them, consider a descriptive suffix instead.
    • 'out:' denotes code used in both error and non-error code paths.
    • 'error:' denotes code only used in error paths.
  11. Don't use 'unsigned int' where 'unsigned' will suffice.
  12. Take care to choose correctly between signed and unsigned variables: a variable (often an index) that should never be negative should be 'unsigned'.
  13. For numeric types choose carefully between explicit widths (for exposed data and derived variables - we use uint32_t style strictly) and native widths (internal variables).
  14. Use variable names consistently in different functions and files and aim for short-but-descriptive names. Avoid introducing new abbreviations without good reason - and if you do, use them consistently everywhere.
  15. Single letter variables are generally only used locally within a few lines of code (e.g. local counters).
  16. Variables are never used for multiple purposes within the same function.
  17. A variable must not hold a quantity in different units at different places. For example, 'num_sectors' holds a number of sectors at all times, even while calculating it.
  18. Check return values from functions in separate statements:
            r = fn();
            if (r)
                    return r;
  19. Non-static functions (intended for export) should normally begin with 'dm_'.
  20. Constants and macros etc. should normally begin with DM (or sometimes dm). This may be followed by a unique secondary prefix to group related items together.

Comments

  1. Comments within the code should describe anything that is unlikely to be obvious to a programmer reading it. Use the following format on separate lines from the code:
    /*
     * Comment goes here
     * and continues here.
     */
  2. If stating that the programmer must do something in a particular way, always explain the reason, so that other programmers can judge whether the statement has become false due to subsequent code changes.
  3. Mark incomplete code or inferior design with a FIXME comment.
  4. Comments in header files should describe how to use the interface and mention any gotchas.
  5. Comments in the patch header should describe everything a reviewer who is not intimately familiar with that particular area of code needs to know about the patch. Every change, however trivial, should be covered by the comments.
  6. Checklist of things to consider mentioning.
    • Reason for the change.
    • Any background information people need to know to understand it, such as the purpose of existing functions.
    • Complete description of everything that is changing.
    • If it's a bug fix, full explanation of the problem, how to reproduce it, impact if it isn't fixed, how/when it was introduced into the code,
    • references to any related bug reports.
    • If it's an enhancement, why it's being done, detailed design information including why it's been done this particular way rather than any other possible ways, why things will be better after the patch is applied, any ABI/API implications, any implications on backwards/forwards compatibility.
    • Any dependencies on other patches.
    • What testing has been done.
    • What testing still ought to be done.
    • Any other information that needs taking into account in the risk analysis.

Patches

  1. When fixing bugs, resist the temptation to change anything in the same patch except that which is strictly necessary to fix the existing bug. If code simplification is possible as a result of the bug fix patch, do that in a follow-up patch that is labelled as not changing any functionality.
  2. Patches should be small and self-contained.
  3. If code is being reorganised to support some change, do as much reorganisation as possible in advance in patches that don't change existing functionality and mention that in the headers. Keep the patches that really make functional changes as small as possible. If desired, have follow-up patches that tidy up after the functional change. Never make any changes to functions in the same patch as reordering them or moving them between files.
  4. Patch names begin with 'dm-' followed by the device-mapper sub-component (e.g. 'crypt-') and then a unique and succint summary. If it's a bug fix, include 'fix' in the name of the patch.

None: KernelPatchGuidelines (last edited 2008-01-10 19:42:38 by localhost)