http://wiki.old.lustre.org/api.php?action=feedcontributions&user=Eeb&feedformat=atomObsolete Lustre Wiki - User contributions [en]2024-03-29T12:04:02ZUser contributionsMediaWiki 1.35.5http://wiki.old.lustre.org/index.php?title=Coding_Guidelines&diff=7363Coding Guidelines2009-09-22T10:59:28Z<p>Eeb: </p>
<hr />
<div>== Beautiful Code == <br />
<br />
''A note from Eric Barton, our lead engineer:''<br />
<br />
More important than the physical layout of code (which is covered in detail below) is the idea that the code should be ''beautiful'' to read.<br />
<br />
What makes code beautiful to me? Fundamentally, it's readability and obviousness. The code must not have secrets but should flow easily, pleasurably and ''accurately'' off the page and into the mind of the reader.<br />
<br />
How do I think beautiful code is written? Like this...<br />
<br />
* The author must be confident and knowledgeable and proud of her work. She must understand what the code should do, the environment it must work in, all the combinations of inputs, all the valid outputs, all the possible races and all the reachable states. She must [http://en.wikipedia.org/wiki/Grok grok] it.<br />
<br />
* Names must be well chosen. The meaning a human reader attaches to a name can be orthogonal to what the compiler does with it, so it's just as easy to mislead as it is to inform. ''[http://en.wikipedia.org/wiki/Does_what_it_says_on_the_tin "Does exactly what it says on the tin"]'' is a popular UK English expression describing something that does ''exactly'' what it tells you it's going to do, no more and no less. For example, if I open a tin labeled "soap", I expect the contents to help me wash and maybe even smell nice. If it's no good at removing dirt, I'll be disappointed. If it removes the dirt but burns off a layer of skin with it, I'll be positively upset. The name of a procedure, a variable or a structure member should tell you something informative about the entity without misleading - just "what it says on the tin".<br />
<br />
* Names must be well chosen. Local, temporary variables can almost always remain relatively short and anonymous, while names in global scope must be unique. In general, the wider the context you expect to use the name in, the more unique and informative the name should be. Don't be scared of long names if they help to ''make_the_code_clearer'', but ''do_not_let_things_get_out_of_hand'' either - we don't write COBOL. Related names should be obvious, unambiguous and avoid naming conflicts with other unrelated names, e.g. by using a consistent prefix. This applies to all API procedures (if not all procedures period) within a given subsystem. Similarly, unique member names for global structures, using a prefix to identify the parent structure type, helps readability.<br />
<br />
* Names must be well chosen. Don't choose names that are easily confused - especially not if the compiler can't even tell the difference when you make a spelling mistake. ''i'' and ''j'' aren't the worst example - ''req_portal'' and ''rep_portal'' are much worse (and taken from our own code!!!).<br />
<br />
* Names must be well chosen. I can't emphasize this issue enough - I hope you get the point.<br />
<br />
* Assertions must be used intelligently. They combine the roles of ''active comment'' and ''software fuse''. As an ''active comment'' they tell you something about the program that you can trust more than a comment. And as a ''software fuse'', they provide fault isolation between subsystems by letting you know when and where invariant assumptions are violated. Overuse must be avoided - it hurts performance without helping readability - and any other use is just plain wrong. For example, assertions must '''never''' be used to validate data read from disk or the network. Network and disk hardware ''does'' fail and Lustre has to handle that - it can't just crash. The same goes for user input. Checking data copied in from userspace with assertions just opens the door for a denial of service attack.<br />
<br />
* Formatting and indentation rules should be followed intelligently. The visual layout of the code on the page should lend itself to being read easily and accurately - it just looks clean and good.<br />
** Separate "ideas" should be separated clearly in the code layout using blank lines that group related statements and separate unrelated statements.<br />
** Procedures should not ramble on. You must be able to take in the meaning of a procedure without scrolling past page after page of code or parsing deeply nested conditionals and loops. The 80-character rule is there for a reason.<br />
** Declarations are easier to refer to while scanning the code if placed in a block locally to, but visually separate from, the code that uses them. Readability is further enhanced by limiting declarations to one per line and aligning types and names vertically.<br />
** Parameters in multi-line procedure calls should be aligned so that they are visually contained by their brackets.<br />
** Brackets should be used in expressions to make operator precedence clear.<br />
** Conditional boolean (''if (expr)''), scalar (''if (val != 0)'') and pointer (''if (ptr != NULL)'') expressions should be written consistently.<br />
** Formatting and indentation rules should not be followed slavishly. If you're faced with either breaking the 80-chars-per-line rule or the parameter indentation rule or creating an obscure helper function, then the 80-chars-per-line rule might have to suffer. The overriding consideration is how the code reads.<br />
<br />
I could go on, but I hope you get the idea. Just think about the poor reader when you're writing, and whether your code will convey its meaning naturally, quickly and accurately, without room for misinterpretation. <br />
<br />
I didn't mention ''clever'' as a feature of beautiful code because it's only one step from ''clever'' to ''tricky'' - consider...<br />
<br />
t = a; a = b; b = t; /* dumb swap */<br />
<br />
a ^= b; b ^= a; a ^= b; /* clever swap */<br />
<br />
You could feel quite pleased that the clever swap avoids the need for a local temporary variable - but is that such a big deal compared with how quickly, easily and accurately the reader will read it? This is a very minor example which can almost be excused because the "cleverness" is confined to a tiny part of the code. But when ''clever'' code gets spread out, it becomes much harder to modify without adding defects. You can only work on code without screwing up if you understand the code ''and'' the environment it works in completely. Or to put it more succinctly...<br />
<br />
:''Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it.'' - [http://en.wikipedia.org/wiki/Brian_Kernighan Brian W. Kernighan]<br />
<br />
IMHO, beautiful code helps code quality because it improves communication between the code author and the code reader. Since everyone maintaining and developing the code is a code reader as well as a code author, the quality of this communication can lead either to a virtuous circle of improving quality, or a vicious circle of degrading quality. You, dear reader, will determine which.<br />
<br />
----<br />
<br />
== Style and Formatting Guidlelines ==<br />
<br />
All of our rules for formatting, wrapping, parenthesis, brace placement, etc., are originally derived from the [http://www.kernel.org/doc/Documentation/CodingStyle Linux kernel rules], which are basically K&R style.<br />
<br />
=== Whitespace ===<br />
<br />
Whitespace gets its own section because unnecessary whitespace changes can cause spurious merge conflicts when code is landed and updated in a distributed development environment. Please ensure that you comply with the guidelines in this section to avoid these issues. We've included default formatting rules for emacs and vim to help make it easier.<br />
<br />
* No tabs should be used in any Lustre, LNET or ''libcfs'' files. The exceptions are ''libsysio'' (maintained by someone else), ''ldiskfs'' and kernel patches (also part of a non-Lustre Group project).<br />
<br />
* Blocks should be indented 8 spaces.<br />
<br />
* New files should contain the following along with the license boilerplate. This will cause vim and emacs to use spaces instead of tabs for indenting. If you use a different editor, it also needs to be set to use spaces for indenting Lustre code.<br />
<pre><br />
/* -*- mode: c; c-basic-offset: 8; indent-tabs-mode: nil; -*-<br />
* vim:expandtab:shiftwidth=8:tabstop=8:<br />
*/<br />
</pre><br />
<br />
* All lines should wrap at 80 characters. If it's getting too hard to wrap at 80 characters, you probably need to rearrange conditional order or break it up into more functions.<br />
<pre><br />
right:<br />
<br />
void func_helper(...)<br />
{<br />
do_sth2_1;<br />
<br />
if (cond3)<br />
do_sth_which_needs_a_very_long_line_to_read_clearly;<br />
<br />
do_sth2_2;<br />
}<br />
<br />
void func (...)<br />
{<br />
if (!cond1)<br />
return;<br />
<br />
do_sth1_1;<br />
<br />
if (cond 2)<br />
func_helper(...)<br />
<br />
do_sth1_2;<br />
}<br />
<br />
wrong:<br />
<br />
void func(...)<br />
{<br />
if (cond1) {<br />
do_sth1_1;<br />
if (cond2) {<br />
do_sth2_1;<br />
if (cond3) {<br />
do_sth_which_needs_a_very_long_line_to_read_clearly;<br />
}<br />
do_sth2_2;<br />
}<br />
do_sth1_2;<br />
}<br />
}<br />
<br />
</pre><br />
<br />
* Do not include spaces or tabs on blank lines or at the end of lines. Please ensure you remove all instances of these in any patches you submit to bugzilla. You can find them with grep or in vim using the following regexps:<br />
<pre><br />
/[ \t]$/<br />
</pre><br />
<br />
:Alternatively, if you use vim, you can put this line in your vimrc file, which will highlight whitespace at the end of lines and spaces followed by tabs in indentation (only works for C/C++ files):<br />
<pre><br />
let c_space_errors=1<br />
</pre><br />
<br />
:Or you can use this command, which will make tabs and whitespace at the end of lines visible for all files (but a bit more discretely):<br />
<pre><br />
set list listchars=tab:>\ ,trail:$<br />
</pre><br />
<br />
:In emacs, you can use (whitespace-mode) or (whitespace-visual-mode) depending on the version. You could also consider using (flyspell-prog-mode).<br />
<br />
=== C Language Features ===<br />
<br />
* Don't use ''inline'' unless you're doing something so performance critical that the function call overhead will make a difference -- in other words: almost never. It makes debugging harder and overuse can actually hurt performance by causing instruction cache overflow.<br />
<br />
* Use ''typedef'' carefully...<br />
** Do not create a new integer ''typedef'' without a good reason.<br />
** Always postfix ''typedef'' names with ''_t'' so that they can be identified clearly in the code.<br />
** ''Never'' ''typedef'' pointers. The ''*'' makes C pointer declarations obvious. Hiding it inside a ''typedef'' just obfuscates the code.<br />
<br />
* Do not embed assignments inside boolean expressions. Although this can make the code more concise, it doesn't necessarily make it more elegant and you increase the risk of confusing "=" with "==" or getting operator precedence wrong if you skimp on brackets. It's even easier to make mistakes when reading the code, so it's much safer simply to avoid it altogether.<br />
<pre><br />
right:<br />
ptr = malloc(size);<br />
if (ptr != NULL) {<br />
...<br />
<br />
wrong:<br />
if ((ptr = malloc(size)) != NULL) {<br />
...<br />
</pre><br />
<br />
* Conditional expressions read more clearly if only boolean expressions are implicit (i.e., non-boolean and pointer expressions compare explicitly with ''0'' and ''NULL'' respectively.)<br />
<pre><br />
right:<br />
if (!writing && /* not writing? */<br />
inode != NULL && /* valid inode? */<br />
ref_count == 0) /* no more references? */<br />
do_this();<br />
<br />
wrong:<br />
if (writing == 0 && /* not writing? */<br />
inode && /* valid inode? */<br />
!ref_count) /* no more references? */<br />
do_this();<br />
</pre><br />
<br />
* Use parentheses to help readability and reduce the chance of operator precedence errors.<br />
<pre><br />
right:<br />
if (a->a_field == 3 ||<br />
((b->b_field & BITMASK1) && (c->c_field & BITMASK2)))<br />
do this();<br />
<br />
wrong:<br />
if (a->a_field == 3 || b->b_field & BITMASK1 && c->c_field & BITMASK2)<br />
do this()<br />
</pre><br />
<br />
=== Layout ===<br />
<br />
* Code can be much more readable if the simpler actions are taken first in a set of tests. Re-ordering conditions like this also eliminates excessive nesting.<br />
<pre><br />
right:<br />
list_for_each_entry(...) {<br />
<br />
if (!condition1) {<br />
do_sth1;<br />
continue;<br />
}<br />
<br />
do_sth2_1;<br />
do_sth2_2;<br />
...<br />
do_sth2_N;<br />
<br />
if (!condition2)<br />
break;<br />
<br />
do_sth3_1;<br />
do_sth3_2;<br />
...<br />
do_sth3_N;<br />
}<br />
wrong:<br />
list_for_each_entry(...) {<br />
if (condition1) {<br />
do_sth2_1;<br />
do_sth2_2;<br />
...<br />
do_sth2_N;<br />
if (condition2) {<br />
do_sth3_1;<br />
do_sth3_2;<br />
...<br />
do_sth3_N;<br />
continue;<br />
} <br />
break;<br />
} else {<br />
do_sth1;<br />
}<br />
}<br />
</pre><br />
<br />
* Variable should be declared one per line, type and name, even if there are multiple variables of the same type. For maximum readability, the names should be aligned on the same column.<br />
<pre><br />
right:<br />
int len;<br />
int count;<br />
struct inode *inode;<br />
<br />
wrong:<br />
int len, count;<br />
struct inode *inode;<br />
</pre><br />
<br />
* Even for short conditionals, the operation should be on a separate line:<br />
<pre><br />
if (foo)<br />
bar();<br />
</pre><br />
<br />
* When you wrap a line containing parenthesis, start the next line after the parenthesis so that the expression or argument is visually bracketed.<br />
<pre><br />
right:<br />
variable = do_something_complicated(long_argument, longer_argument,<br />
longest_argument(sub_argument,<br />
foo_argument),<br />
last_argument);<br />
<br />
if (some_long_condition(arg1, arg2, arg3) < some_long_value &&<br />
another_long_condition(very_long_argument_name,<br />
another_long_argument_name) ><br />
second_long_value) {<br />
...<br />
<br />
wrong:<br />
<br />
variable = do_something_complicated(long_argument, longer_argument,<br />
longest_argument(sub_argument, foo_argument),<br />
last_argument);<br />
<br />
if (some_long_condition(arg1, arg2, arg3) < some_long_value &&<br />
another_long_condition(very_long_argument_name,<br />
another_long_argument_name) ><br />
second_long_value) {<br />
...<br />
</pre><br />
<br />
* If you're wrapping an expression, put the operator at the end of the line. If there are no parentheses to which to align the start of the next line, just indent 8 more spaces.<br />
<pre><br />
off = le32_to_cpu(fsd->fsd_client_start) +<br />
cl_idx * le16_to_cpu(fsd->fsd_client_size);<br />
</pre><br />
<br />
* Binary and ternary (but not unary) operators should be separated from their arguments by one space.<br />
<pre><br />
a++;<br />
b |= c;<br />
d = (f > g) ? 0 : 1;<br />
</pre><br />
<br />
* Function calls should be nestled against the parentheses, the parentheses should crowd the arguments, and one space should appear after commas:<br />
<pre><br />
right: <br />
do_foo(bar, baz);<br />
<br />
wrong:<br />
do_foo ( bar,baz );<br />
</pre><br />
<br />
* Put a space between ''if'', ''for'', ''while'' etc. and the following parenthesis. Put a space after each semicolon in a ''for'' statement.<br />
<pre><br />
for (a = 0; a < b; a++)<br />
if (a < b || a == c)<br />
while (1)<br />
</pre><br />
<br />
* Opening braces should be on the same line as the line that introduces the block, except for function calls. Bare closing braces (i.e. not ''else'' or ''while'' in do/while) get their own line. <br />
<pre><br />
int foo(void)<br />
{<br />
if (bar) {<br />
this();<br />
that();<br />
} else if (baz) {<br />
stuff();<br />
} else {<br />
other_stuff();<br />
}<br />
<br />
do {<br />
cow();<br />
} while (condition);<br />
}<br />
</pre><br />
<br />
* If one part of a compound ''if'' block has braces, all should.<br />
<pre><br />
right:<br />
if (foo) {<br />
bar();<br />
baz();<br />
} else {<br />
salmon();<br />
}<br />
<br />
wrong:<br />
if (foo) {<br />
bar();<br />
baz();<br />
} else<br />
moose();<br />
</pre><br />
<br />
* When you define a macro, protect callers by placing parentheses round every parameter reference in the body. Line up the backslashes of multi-line macros to help readability. Use a do/while (0) block with ''no'' trailing semicolon to ensure multi-statement macros are syntactically equivalent to procedure calls.<br />
<pre><br />
/* right */<br />
#define DO_STUFF(a) \<br />
do { \<br />
int b = (a) + MAGIC; \<br />
do_other_stuff(b); \<br />
} while (0)<br />
<br />
/* wrong */<br />
#define DO_STUFF(a) \<br />
do { \<br />
int b = a + MAGIC; \<br />
do_other_stuff(b); \<br />
} while (0);<br />
</pre><br />
<br />
* If you write conditionally compiled code in a procedure body, make sure you do not create unbalanced braces, quotes, etc. This really confuses editors that navigate expressions or use fonts to highlight language features. It can often be much cleaner to put the conditionally compiled code in its own helper function which, by good choice of name, documents itself too.<br />
<pre><br />
/* right */<br />
static inline int invalid_dentry(struct dentry *d)<br />
{<br />
#ifdef DCACHE_LUSTRE_INVALID<br />
return d->d_flags & DCACHE_LUSTRE_INVALID;<br />
#else<br />
return d_unhashed(d);<br />
#endif<br />
}<br />
<br />
int do_stuff(struct dentry *parent)<br />
{<br />
if (invalid_dentry(parent)) {<br />
...<br />
<br />
/* wrong */<br />
int do_stuff(struct dentry *parent)<br />
{<br />
#ifdef DCACHE_LUSTRE_INVALID<br />
if (parent->d_flags & DCACHE_LUSTRE_INVALID) {<br />
#else<br />
if (d_unhashed(parent)) {<br />
#endif<br />
...<br />
</pre><br />
<br />
* If you nest preprocessor commands, use spaces to visually delineate:<br />
<pre><br />
#ifdef __KERNEL__<br />
# include <goose><br />
# define MOOSE steak<br />
#else<br />
# include <mutton><br />
# define MOOSE prancing<br />
#endif<br />
</pre><br />
<br />
* For very long #ifdefs, include the conditional with each #endif to make it readable:<br />
<pre><br />
#ifdef __KERNEL__<br />
# if LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,0)<br />
/* lots<br />
of<br />
stuff */<br />
# endif /* KERNEL_VERSION(2,5,0) */<br />
#else /* !__KERNEL__ */<br />
# if HAVE_FEATURE<br />
/* more<br />
* stuff */<br />
# endif<br />
#endif /* __KERNEL__ */<br />
</pre><br />
<br />
* Comments should have the leading '/*' on the same line as the comment and the trailing '*/' at the end of the last comment line. Intermediate lines should start with a '*' aligned with the '*' on the first line:<br />
<pre><br />
/* This is a short comment */<br />
<br />
/* This is a multi-line comment. I wish the line would wrap already,<br />
* as I don't have much to write about. */<br />
</pre><br />
<br />
* Function declarations absolutely should NOT go into .c files, unless they are forward declarations for static functions that can't otherwise be moved before the caller. Instead, the declaration should go into the most "local" header available (preferably *_internal.h for a given piece of code).<br />
<br />
* Structure and constant declarations should not be declared in multiple places. Put the struct into the most "local" header possible. If it is something that is passed over the wire, it needs to go into lustre_idl.h and needs to be correctly swabbed when the RPC message is unpacked.<br />
<br />
* The types and printf/printk formats used by Lustre code are:<br />
<pre><br />
__u64 LPU64/LPX64/LPD64 (unsigned, hex, signed)<br />
size_t LPSZ (or cast to int and use %u / %d)<br />
__u32/int %u/%x/%d (unsigned, hex, signed)<br />
(unsigned) long long %llu/%llx/%lld<br />
loff_t %lld after a cast to long long (unfortunately)<br />
</pre><br />
<br />
* For Autoconf macros, follow the [http://www.gnu.org/software/autoconf/manual/html_node/Coding-Style.html style suggested in the autoconf manual].<br />
<br />
<pre><br />
AC_CACHE_CHECK([for EMX OS/2 environment], [ac_cv_emxos2],<br />
[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([], [return __EMX__;])],<br />
[ac_cv_emxos2=yes],<br />
[ac_cv_emxos2=no])])<br />
</pre><br />
:or_even<br />
<pre><br />
AC_CACHE_CHECK([for EMX OS/2 environment],<br />
[ac_cv_emxos2],<br />
[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([],<br />
[return __EMX__;])],<br />
[ac_cv_emxos2=yes],<br />
[ac_cv_emxos2=no])])<br />
</pre><br />
<br />
----</div>Eebhttp://wiki.old.lustre.org/index.php?title=Coding_Guidelines&diff=7362Coding Guidelines2009-09-22T10:55:57Z<p>Eeb: </p>
<hr />
<div><br />
== Beautiful Code == <br />
<br />
''A note from Eric Barton, our lead engineer:''<br />
<br />
More important than the physical layout of code (which is covered in detail below) is the idea that the code should be ''beautiful'' to read.<br />
<br />
What makes code beautiful to me? Fundamentally, it's readability and obviousness. The code must not have secrets but should flow easily, pleasurably and ''accurately'' off the page and into the mind of the reader.<br />
<br />
How do I think beautiful code is written? Like this...<br />
<br />
* The author must be confident and knowledgeable and proud of her work. She must understand what the code should do, the environment it must work in, all the combinations of inputs, all the valid outputs, all the possible races and all the reachable states. She must [http://en.wikipedia.org/wiki/Grok grok] it.<br />
<br />
* Names must be well chosen. The meaning a human reader attaches to a name can be orthogonal to what the compiler does with it, so it's just as easy to mislead as it is to inform. ''[http://en.wikipedia.org/wiki/Does_what_it_says_on_the_tin "Does exactly what it says on the tin"]'' is a popular UK English expression describing something that does ''exactly'' what it tells you it's going to do, no more and no less. For example, if I open a tin labeled "soap", I expect the contents to help me wash and maybe even smell nice. If it's no good at removing dirt, I'll be disappointed. If it removes the dirt but burns off a layer of skin with it, I'll be positively upset. The name of a procedure, a variable or a structure member should tell you something informative about the entity without misleading - just "what it says on the tin".<br />
<br />
* Names must be well chosen. Local, temporary variables can almost always remain relatively short and anonymous, while names in global scope must be unique. In general, the wider the context you expect to use the name in, the more unique and informative the name should be. Don't be scared of long names if they help to ''make_the_code_clearer'', but ''do_not_let_things_get_out_of_hand'' either - we don't write COBOL. Related names should be obvious, unambiguous and avoid naming conflicts with other unrelated names, e.g. by using a consistent prefix. This applies to all API procedures (if not all procedures period) within a given subsystem. Similarly, unique member names for global structures, using a prefix to identify the parent structure type, helps readability.<br />
<br />
* Names must be well chosen. Don't choose names that are easily confused - especially not if the compiler can't even tell the difference when you make a spelling mistake. ''i'' and ''j'' aren't the worst example - ''req_portal'' and ''rep_portal'' are much worse (and taken from our own code!!!).<br />
<br />
* Names must be well chosen. I can't emphasize this issue enough - I hope you get the point.<br />
<br />
* Assertions must be used intelligently. They combine the roles of ''active comment'' and ''software fuse''. As an ''active comment'' they tell you something about the program that you can trust more than a comment. And as a ''software fuse'', they provide fault isolation between subsystems by letting you know when and where invariant assumptions are violated. Overuse must be avoided - it hurts performance without helping readability - and any other use is just plain wrong. For example, assertions must '''never''' be used to validate data read, disk or the network. Network and disk hardware ''does'' fail and Lustre has to handle that - it can't just crash. Similarly for user input - checking data copied in from userspace with assertions just opens the door for a denial of service attack.<br />
<br />
* Formatting and indentation rules should be followed intelligently. The visual layout of the code on the page should lend itself to being read easily and accurately - it just looks clean and good.<br />
** Separate "ideas" should be separated clearly in the code layout using blank lines that group related statements and separate unrelated statements.<br />
** Procedures should not ramble on. You must be able to take in the meaning of a procedure without scrolling past page after page of code or parsing deeply nested conditionals and loops. The 80-character rule is there for a reason.<br />
** Declarations are easier to refer to while scanning the code if placed in a block locally to, but visually separate from, the code that uses them. Readability is further enhanced by limiting declarations to one per line and aligning types and names vertically.<br />
** Parameters in multi-line procedure calls should be aligned so that they are visually contained by their brackets.<br />
** Brackets should be used in expressions to make operator precedence clear.<br />
** Conditional boolean (''if (expr)''), scalar (''if (val != 0)'') and pointer (''if (ptr != NULL)'') expressions should be written consistently.<br />
** Formatting and indentation rules should not be followed slavishly. If you're faced with either breaking the 80-chars-per-line rule or the parameter indentation rule or creating an obscure helper function, then the 80-chars-per-line rule might have to suffer. The overriding consideration is how the code reads.<br />
<br />
I could go on, but I hope you get the idea. Just think about the poor reader when you're writing, and whether your code will convey its meaning naturally, quickly and accurately, without room for misinterpretation. <br />
<br />
I didn't mention ''clever'' as a feature of beautiful code because it's only one step from ''clever'' to ''tricky'' - consider...<br />
<br />
t = a; a = b; b = t; /* dumb swap */<br />
<br />
a ^= b; b ^= a; a ^= b; /* clever swap */<br />
<br />
You could feel quite pleased that the clever swap avoids the need for a local temporary variable - but is that such a big deal compared with how quickly, easily and accurately the reader will read it? This is a very minor example which can almost be excused because the "cleverness" is confined to a tiny part of the code. But when ''clever'' code gets spread out, it becomes much harder to modify without adding defects. You can only work on code without screwing up if you understand the code ''and'' the environment it works in completely. Or to put it more succinctly...<br />
<br />
:''Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it.'' - [http://en.wikipedia.org/wiki/Brian_Kernighan Brian W. Kernighan]<br />
<br />
IMHO, beautiful code helps code quality because it improves communication between the code author and the code reader. Since everyone maintaining and developing the code is a code reader as well as a code author, the quality of this communication can lead either to a virtuous circle of improving quality, or a vicious circle of degrading quality. You, dear reader, will determine which.<br />
<br />
----<br />
<br />
== Style and Formatting Guidlelines ==<br />
<br />
All of our rules for formatting, wrapping, parenthesis, brace placement, etc., are originally derived from the [http://www.kernel.org/doc/Documentation/CodingStyle Linux kernel rules], which are basically K&R style.<br />
<br />
=== Whitespace ===<br />
<br />
Whitespace gets its own section because unnecessary whitespace changes can cause spurious merge conflicts when code is landed and updated in a distributed development environment. Please ensure that you comply with the guidelines in this section to avoid these issues. We've included default formatting rules for emacs and vim to help make it easier.<br />
<br />
* No tabs should be used in any Lustre, LNET or ''libcfs'' files. The exceptions are ''libsysio'' (maintained by someone else), ''ldiskfs'' and kernel patches (also part of a non-Lustre Group project).<br />
<br />
* Blocks should be indented 8 spaces.<br />
<br />
* New files should contain the following along with the license boilerplate. This will cause vim and emacs to use spaces instead of tabs for indenting. If you use a different editor, it also needs to be set to use spaces for indenting Lustre code.<br />
<pre><br />
/* -*- mode: c; c-basic-offset: 8; indent-tabs-mode: nil; -*-<br />
* vim:expandtab:shiftwidth=8:tabstop=8:<br />
*/<br />
</pre><br />
<br />
* All lines should wrap at 80 characters. If it's getting too hard to wrap at 80 characters, you probably need to rearrange conditional order or break it up into more functions.<br />
<pre><br />
right:<br />
<br />
void func_helper(...)<br />
{<br />
do_sth2_1;<br />
<br />
if (cond3)<br />
do_sth_which_needs_a_very_long_line_to_read_clearly;<br />
<br />
do_sth2_2;<br />
}<br />
<br />
void func (...)<br />
{<br />
if (!cond1)<br />
return;<br />
<br />
do_sth1_1;<br />
<br />
if (cond 2)<br />
func_helper(...)<br />
<br />
do_sth1_2;<br />
}<br />
<br />
wrong:<br />
<br />
void func(...)<br />
{<br />
if (cond1) {<br />
do_sth1_1;<br />
if (cond2) {<br />
do_sth2_1;<br />
if (cond3) {<br />
do_sth_which_needs_a_very_long_line_to_read_clearly;<br />
}<br />
do_sth2_2;<br />
}<br />
do_sth1_2;<br />
}<br />
}<br />
<br />
</pre><br />
<br />
* Do not include spaces or tabs on blank lines or at the end of lines. Please ensure you remove all instances of these in any patches you submit to bugzilla. You can find them with grep or in vim using the following regexps:<br />
<pre><br />
/[ \t]$/<br />
</pre><br />
<br />
:Alternatively, if you use vim, you can put this line in your vimrc file, which will highlight whitespace at the end of lines and spaces followed by tabs in indentation (only works for C/C++ files):<br />
<pre><br />
let c_space_errors=1<br />
</pre><br />
<br />
:Or you can use this command, which will make tabs and whitespace at the end of lines visible for all files (but a bit more discretely):<br />
<pre><br />
set list listchars=tab:>\ ,trail:$<br />
</pre><br />
<br />
:In emacs, you can use (whitespace-mode) or (whitespace-visual-mode) depending on the version. You could also consider using (flyspell-prog-mode).<br />
<br />
=== C Language Features ===<br />
<br />
* Don't use ''inline'' unless you're doing something so performance critical that the function call overhead will make a difference -- in other words: almost never. It makes debugging harder and overuse can actually hurt performance by causing instruction cache overflow.<br />
<br />
* Use ''typedef'' carefully...<br />
** Do not create a new integer ''typedef'' without a good reason.<br />
** Always postfix ''typedef'' names with ''_t'' so that they can be identified clearly in the code.<br />
** ''Never'' ''typedef'' pointers. The ''*'' makes C pointer declarations obvious. Hiding it inside a ''typedef'' just obfuscates the code.<br />
<br />
* Do not embed assignments inside boolean expressions. Although this can make the code more concise, it doesn't necessarily make it more elegant and you increase the risk of confusing "=" with "==" or getting operator precedence wrong if you skimp on brackets. It's even easier to make mistakes when reading the code, so it's much safer simply to avoid it altogether.<br />
<pre><br />
right:<br />
ptr = malloc(size);<br />
if (ptr != NULL) {<br />
...<br />
<br />
wrong:<br />
if ((ptr = malloc(size)) != NULL) {<br />
...<br />
</pre><br />
<br />
* Conditional expressions read more clearly if only boolean expressions are implicit (i.e., non-boolean and pointer expressions compare explicitly with ''0'' and ''NULL'' respectively.)<br />
<pre><br />
right:<br />
if (!writing && /* not writing? */<br />
inode != NULL && /* valid inode? */<br />
ref_count == 0) /* no more references? */<br />
do_this();<br />
<br />
wrong:<br />
if (writing == 0 && /* not writing? */<br />
inode && /* valid inode? */<br />
!ref_count) /* no more references? */<br />
do_this();<br />
</pre><br />
<br />
* Use parentheses to help readability and reduce the chance of operator precedence errors.<br />
<pre><br />
right:<br />
if (a->a_field == 3 ||<br />
((b->b_field & BITMASK1) && (c->c_field & BITMASK2)))<br />
do this();<br />
<br />
wrong:<br />
if (a->a_field == 3 || b->b_field & BITMASK1 && c->c_field & BITMASK2)<br />
do this()<br />
</pre><br />
<br />
=== Layout ===<br />
<br />
* Code can be much more readable if the simpler actions are taken first in a set of tests. Re-ordering conditions like this also eliminates excessive nesting.<br />
<pre><br />
right:<br />
list_for_each_entry(...) {<br />
<br />
if (!condition1) {<br />
do_sth1;<br />
continue;<br />
}<br />
<br />
do_sth2_1;<br />
do_sth2_2;<br />
...<br />
do_sth2_N;<br />
<br />
if (!condition2)<br />
break;<br />
<br />
do_sth3_1;<br />
do_sth3_2;<br />
...<br />
do_sth3_N;<br />
}<br />
wrong:<br />
list_for_each_entry(...) {<br />
if (condition1) {<br />
do_sth2_1;<br />
do_sth2_2;<br />
...<br />
do_sth2_N;<br />
if (condition2) {<br />
do_sth3_1;<br />
do_sth3_2;<br />
...<br />
do_sth3_N;<br />
continue;<br />
} <br />
break;<br />
} else {<br />
do_sth1;<br />
}<br />
}<br />
</pre><br />
<br />
* Variable should be declared one per line, type and name, even if there are multiple variables of the same type. For maximum readability, the names should be aligned on the same column.<br />
<pre><br />
right:<br />
int len;<br />
int count;<br />
struct inode *inode;<br />
<br />
wrong:<br />
int len, count;<br />
struct inode *inode;<br />
</pre><br />
<br />
* Even for short conditionals, the operation should be on a separate line:<br />
<pre><br />
if (foo)<br />
bar();<br />
</pre><br />
<br />
* When you wrap a line containing parenthesis, start the next line after the parenthesis so that the expression or argument is visually bracketed.<br />
<pre><br />
right:<br />
variable = do_something_complicated(long_argument, longer_argument,<br />
longest_argument(sub_argument,<br />
foo_argument),<br />
last_argument);<br />
<br />
if (some_long_condition(arg1, arg2, arg3) < some_long_value &&<br />
another_long_condition(very_long_argument_name,<br />
another_long_argument_name) ><br />
second_long_value) {<br />
...<br />
<br />
wrong:<br />
<br />
variable = do_something_complicated(long_argument, longer_argument,<br />
longest_argument(sub_argument, foo_argument),<br />
last_argument);<br />
<br />
if (some_long_condition(arg1, arg2, arg3) < some_long_value &&<br />
another_long_condition(very_long_argument_name,<br />
another_long_argument_name) ><br />
second_long_value) {<br />
...<br />
</pre><br />
<br />
* If you're wrapping an expression, put the operator at the end of the line. If there are no parentheses to which to align the start of the next line, just indent 8 more spaces.<br />
<pre><br />
off = le32_to_cpu(fsd->fsd_client_start) +<br />
cl_idx * le16_to_cpu(fsd->fsd_client_size);<br />
</pre><br />
<br />
* Binary and ternary (but not unary) operators should be separated from their arguments by one space.<br />
<pre><br />
a++;<br />
b |= c;<br />
d = (f > g) ? 0 : 1;<br />
</pre><br />
<br />
* Function calls should be nestled against the parentheses, the parentheses should crowd the arguments, and one space should appear after commas:<br />
<pre><br />
right: <br />
do_foo(bar, baz);<br />
<br />
wrong:<br />
do_foo ( bar,baz );<br />
</pre><br />
<br />
* Put a space between ''if'', ''for'', ''while'' etc. and the following parenthesis. Put a space after each semicolon in a ''for'' statement.<br />
<pre><br />
for (a = 0; a < b; a++)<br />
if (a < b || a == c)<br />
while (1)<br />
</pre><br />
<br />
* Opening braces should be on the same line as the line that introduces the block, except for function calls. Bare closing braces (i.e. not ''else'' or ''while'' in do/while) get their own line. <br />
<pre><br />
int foo(void)<br />
{<br />
if (bar) {<br />
this();<br />
that();<br />
} else if (baz) {<br />
stuff();<br />
} else {<br />
other_stuff();<br />
}<br />
<br />
do {<br />
cow();<br />
} while (condition);<br />
}<br />
</pre><br />
<br />
* If one part of a compound ''if'' block has braces, all should.<br />
<pre><br />
right:<br />
if (foo) {<br />
bar();<br />
baz();<br />
} else {<br />
salmon();<br />
}<br />
<br />
wrong:<br />
if (foo) {<br />
bar();<br />
baz();<br />
} else<br />
moose();<br />
</pre><br />
<br />
* When you define a macro, protect callers by placing parentheses round every parameter reference in the body. Line up the backslashes of multi-line macros to help readability. Use a do/while (0) block with ''no'' trailing semicolon to ensure multi-statement macros are syntactically equivalent to procedure calls.<br />
<pre><br />
/* right */<br />
#define DO_STUFF(a) \<br />
do { \<br />
int b = (a) + MAGIC; \<br />
do_other_stuff(b); \<br />
} while (0)<br />
<br />
/* wrong */<br />
#define DO_STUFF(a) \<br />
do { \<br />
int b = a + MAGIC; \<br />
do_other_stuff(b); \<br />
} while (0);<br />
</pre><br />
<br />
* If you write conditionally compiled code in a procedure body, make sure you do not create unbalanced braces, quotes, etc. This really confuses editors that navigate expressions or use fonts to highlight language features. It can often be much cleaner to put the conditionally compiled code in its own helper function which, by good choice of name, documents itself too.<br />
<pre><br />
/* right */<br />
static inline int invalid_dentry(struct dentry *d)<br />
{<br />
#ifdef DCACHE_LUSTRE_INVALID<br />
return d->d_flags & DCACHE_LUSTRE_INVALID;<br />
#else<br />
return d_unhashed(d);<br />
#endif<br />
}<br />
<br />
int do_stuff(struct dentry *parent)<br />
{<br />
if (invalid_dentry(parent)) {<br />
...<br />
<br />
/* wrong */<br />
int do_stuff(struct dentry *parent)<br />
{<br />
#ifdef DCACHE_LUSTRE_INVALID<br />
if (parent->d_flags & DCACHE_LUSTRE_INVALID) {<br />
#else<br />
if (d_unhashed(parent)) {<br />
#endif<br />
...<br />
</pre><br />
<br />
* If you nest preprocessor commands, use spaces to visually delineate:<br />
<pre><br />
#ifdef __KERNEL__<br />
# include <goose><br />
# define MOOSE steak<br />
#else<br />
# include <mutton><br />
# define MOOSE prancing<br />
#endif<br />
</pre><br />
<br />
* For very long #ifdefs, include the conditional with each #endif to make it readable:<br />
<pre><br />
#ifdef __KERNEL__<br />
# if LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,0)<br />
/* lots<br />
of<br />
stuff */<br />
# endif /* KERNEL_VERSION(2,5,0) */<br />
#else /* !__KERNEL__ */<br />
# if HAVE_FEATURE<br />
/* more<br />
* stuff */<br />
# endif<br />
#endif /* __KERNEL__ */<br />
</pre><br />
<br />
* Comments should have the leading '/*' on the same line as the comment and the trailing '*/' at the end of the last comment line. Intermediate lines should start with a '*' aligned with the '*' on the first line:<br />
<pre><br />
/* This is a short comment */<br />
<br />
/* This is a multi-line comment. I wish the line would wrap already,<br />
* as I don't have much to write about. */<br />
</pre><br />
<br />
* Function declarations absolutely should NOT go into .c files, unless they are forward declarations for static functions that can't otherwise be moved before the caller. Instead, the declaration should go into the most "local" header available (preferably *_internal.h for a given piece of code).<br />
<br />
* Structure and constant declarations should not be declared in multiple places. Put the struct into the most "local" header possible. If it is something that is passed over the wire, it needs to go into lustre_idl.h and needs to be correctly swabbed when the RPC message is unpacked.<br />
<br />
* The types and printf/printk formats used by Lustre code are:<br />
<pre><br />
__u64 LPU64/LPX64/LPD64 (unsigned, hex, signed)<br />
size_t LPSZ (or cast to int and use %u / %d)<br />
__u32/int %u/%x/%d (unsigned, hex, signed)<br />
(unsigned) long long %llu/%llx/%lld<br />
loff_t %lld after a cast to long long (unfortunately)<br />
</pre><br />
<br />
* For Autoconf macros, follow the [http://www.gnu.org/software/autoconf/manual/html_node/Coding-Style.html style suggested in the autoconf manual].<br />
<br />
<pre><br />
AC_CACHE_CHECK([for EMX OS/2 environment], [ac_cv_emxos2],<br />
[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([], [return __EMX__;])],<br />
[ac_cv_emxos2=yes],<br />
[ac_cv_emxos2=no])])<br />
</pre><br />
:or_even<br />
<pre><br />
AC_CACHE_CHECK([for EMX OS/2 environment],<br />
[ac_cv_emxos2],<br />
[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([],<br />
[return __EMX__;])],<br />
[ac_cv_emxos2=yes],<br />
[ac_cv_emxos2=no])])<br />
</pre><br />
<br />
----</div>Eebhttp://wiki.old.lustre.org/index.php?title=Coding_Guidelines&diff=7296Coding Guidelines2009-09-21T13:02:09Z<p>Eeb: </p>
<hr />
<div><br />
<br />
== Beautiful Code == <br />
<br />
''A note from Eric Barton, our lead engineer:''<br />
<br />
More important than the physical layout of code (which is covered in detail below) is the idea that the code should be ''beautiful'' to read.<br />
<br />
What makes code beautiful to me? Fundamentally, it's readability and obviousness. The code must not have secrets but should flow easily, pleasurably and ''accurately'' off the page and into the mind of the reader.<br />
<br />
How do I think beautiful code is written? Like this...<br />
<br />
* The author must be confident and knowledgeable and proud of her work. She must understand what the code should do, the environment it must work in, all the combinations of inputs, all the valid outputs, all the possible races and all the reachable states. She must [http://en.wikipedia.org/wiki/Grok grok] it.<br />
<br />
* Names must be well chosen. The meaning a human reader attaches to a name can be orthogonal to what the compiler does with it, so it's just as easy to mislead as it is to inform. ''[http://en.wikipedia.org/wiki/Does_what_it_says_on_the_tin "Does exactly what it says on the tin"]'' is a popular UK English expression describing something that does ''exactly'' what it tells you it's going to do, no more and no less. For example, if I open a tin labeled "soap", I expect the contents to help me wash and maybe even smell nice. If it's no good at removing dirt, I'll be disappointed. If it removes the dirt but burns off a layer of skin with it, I'll be positively upset. The name of a procedure, a variable or a structure member should tell you something informative about the entity without misleading - just "what it says on the tin".<br />
<br />
* Names must be well chosen. Local, temporary variables can almost always remain relatively short and anonymous, while names in global scope must be unique. In general, the wider the context you expect to use the name in, the more unique and informative the name should be. Don't be scared of long names if they help to ''make_the_code_clearer'', but ''do_not_let_things_get_out_of_hand'' either - we don't write COBOL. Related names should be obvious, unambiguous and avoid naming conflicts with other unrelated names, e.g. by using a consistent prefix. This applies to all API procedures (if not all procedures period) within a given subsystem. Similarly, unique member names for global structures, using a prefix to identify the parent structure type, helps readability.<br />
<br />
* Names must be well chosen. Don't choose names that are easily confused - especially not if the compiler can't even tell the difference when you make a spelling mistake. ''i'' and ''j'' aren't the worst example - ''req_portal'' and ''rep_portal'' are much worse (and taken from our own code!!!).<br />
<br />
* Names must be well chosen. I can't emphasize this issue enough - I hope you get the point.<br />
<br />
* Assertions must be used intelligently. They combine the roles of ''active comment'' and ''software fuse''. As an ''active comment'' they tell you something about the program that you can trust more than a comment. And as a ''software fuse'', they provide fault isolation between subsystems by letting you know when and where invariant assumptions are violated. Overuse must be avoided - it hurts performance without helping readability - and any other use is just plain wrong. For example, assertions must '''never''' be used to validate data read from disk or the network. Network and disk hardware ''does'' fail and Lustre has to handle that - it can't just crash. <br />
<br />
* Formatting and indentation rules should be followed intelligently. The visual layout of the code on the page should lend itself to being read easily and accurately - it just looks clean and good.<br />
** Separate "ideas" should be separated clearly in the code layout using blank lines that group related statements and separate unrelated statements.<br />
** Procedures should not ramble on. You must be able to take in the meaning of a procedure without scrolling past page after page of code or parsing deeply nested conditionals and loops. The 80-character rule is there for a reason.<br />
** Declarations are easier to refer to while scanning the code if placed in a block locally to, but visually separate from, the code that uses them. Readability is further enhanced by limiting declarations to one per line and aligning types and names vertically.<br />
** Parameters in multi-line procedure calls should be aligned so that they are visually contained by their brackets.<br />
** Brackets should be used in expressions to make operator precedence clear.<br />
** Conditional boolean (''if (expr)''), scalar (''if (val != 0)'') and pointer (''if (ptr != NULL)'') expressions should be written consistently.<br />
** Formatting and indentation rules should not be followed slavishly. If you're faced with either breaking the 80-chars-per-line rule or the parameter indentation rule or creating an obscure helper function, then the 80-chars-per-line rule might have to suffer. The overriding consideration is how the code reads.<br />
<br />
I could go on, but I hope you get the idea. Just think about the poor reader when you're writing, and whether your code will convey its meaning naturally, quickly and accurately, without room for misinterpretation. <br />
<br />
I didn't mention ''clever'' as a feature of beautiful code because it's only one step from ''clever'' to ''tricky'' - consider...<br />
<br />
t = a; a = b; b = t; /* dumb swap */<br />
<br />
a ^= b; b ^= a; a ^= b; /* clever swap */<br />
<br />
You could feel quite pleased that the clever swap avoids the need for a local temporary variable - but is that such a big deal compared with how quickly, easily and accurately the reader will read it? This is a very minor example which can almost be excused because the "cleverness" is confined to a tiny part of the code. But when ''clever'' code gets spread out, it becomes much harder to modify without adding defects. You can only work on code without screwing up if you understand the code ''and'' the environment it works in completely. Or to put it more succinctly...<br />
<br />
:''Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it.'' - [http://en.wikipedia.org/wiki/Brian_Kernighan Brian W. Kernighan]<br />
<br />
IMHO, beautiful code helps code quality because it improves communication between the code author and the code reader. Since everyone maintaining and developing the code is a code reader as well as a code author, the quality of this communication can lead either to a virtuous circle of improving quality, or a vicious circle of degrading quality. You, dear reader, will determine which.<br />
<br />
----<br />
<br />
== Style and Formatting Guidlelines ==<br />
<br />
All of our rules for formatting, wrapping, parenthesis, brace placement, etc., are originally derived from the [http://www.kernel.org/doc/Documentation/CodingStyle Linux kernel rules], which are basically K&R style.<br />
<br />
=== Whitespace ===<br />
<br />
Whitespace gets its own section because unnecessary whitespace changes can cause spurious merge conflicts when code is landed and updated in a distributed development environment. Please ensure that you comply with the guidelines in this section to avoid these issues. We've included default formatting rules for emacs and vim to help make it easier.<br />
<br />
* No tabs should be used in any Lustre, LNET or ''libcfs'' files. The exceptions are ''libsysio'' (maintained by someone else), ''ldiskfs'' and kernel patches (also part of a non-Lustre Group project).<br />
<br />
* Blocks should be indented 8 spaces.<br />
<br />
* New files should contain the following along with the license boilerplate. This will cause vim and emacs to use spaces instead of tabs for indenting. If you use a different editor, it also needs to be set to use spaces for indenting Lustre code.<br />
<pre><br />
/* -*- mode: c; c-basic-offset: 8; indent-tabs-mode: nil; -*-<br />
* vim:expandtab:shiftwidth=8:tabstop=8:<br />
*/<br />
</pre><br />
<br />
* All lines should wrap at 80 characters. If it's getting too hard to wrap at 80 characters, you probably need to rearrange conditional order or break it up into more functions.<br />
<pre><br />
right:<br />
<br />
void func_helper(...)<br />
{<br />
do_sth2_1;<br />
<br />
if (cond3)<br />
do_sth_which_needs_a_very_long_line_to_read_clearly;<br />
<br />
do_sth2_2;<br />
}<br />
<br />
void func (...)<br />
{<br />
if (!cond1)<br />
return;<br />
<br />
do_sth1_1;<br />
<br />
if (cond 2)<br />
func_helper(...)<br />
<br />
do_sth1_2;<br />
}<br />
<br />
wrong:<br />
<br />
void func(...)<br />
{<br />
if (cond1) {<br />
do_sth1_1;<br />
if (cond2) {<br />
do_sth2_1;<br />
if (cond3) {<br />
do_sth_which_needs_a_very_long_line_to_read_clearly;<br />
}<br />
do_sth2_2;<br />
}<br />
do_sth1_2;<br />
}<br />
}<br />
<br />
</pre><br />
<br />
* Do not include spaces or tabs on blank lines or at the end of lines. Please ensure you remove all instances of these in any patches you submit to bugzilla. You can find them with grep or in vim using the following regexps:<br />
<pre><br />
/[ \t]$/<br />
</pre><br />
<br />
:Alternatively, if you use vim, you can put this line in your vimrc file, which will highlight whitespace at the end of lines and spaces followed by tabs in indentation (only works for C/C++ files):<br />
<pre><br />
let c_space_errors=1<br />
</pre><br />
<br />
:Or you can use this command, which will make tabs and whitespace at the end of lines visible for all files (but a bit more discretely):<br />
<pre><br />
set list listchars=tab:>\ ,trail:$<br />
</pre><br />
<br />
:In emacs, you can use (whitespace-mode) or (whitespace-visual-mode) depending on the version. You could also consider using (flyspell-prog-mode).<br />
<br />
=== C Language Features ===<br />
<br />
* Don't use ''inline'' unless you're doing something so performance critical that the function call overhead will make a difference -- in other words: almost never. It makes debugging harder and overuse can actually hurt performance by causing instruction cache overflow.<br />
<br />
* Use ''typedef'' carefully...<br />
** Do not create a new integer ''typedef'' without a good reason.<br />
** Always postfix ''typedef'' names with ''_t'' so that they can be identified clearly in the code.<br />
** ''Never'' ''typedef'' pointers. The ''*'' makes C pointer declarations obvious. Hiding it inside a ''typedef'' just obfuscates the code.<br />
<br />
* Do not embed assignments inside boolean expressions. Although this can make the code more concise, it doesn't necessarily make it more elegant and you increase the risk of confusing "=" with "==" or getting operator precedence wrong if you skimp on brackets. It's even easier to make mistakes when reading the code, so it's much safer simply to avoid it altogether.<br />
<pre><br />
right:<br />
ptr = malloc(size);<br />
if (ptr != NULL) {<br />
...<br />
<br />
wrong:<br />
if ((ptr = malloc(size)) != NULL) {<br />
...<br />
</pre><br />
<br />
* Conditional expressions read more clearly if only boolean expressions are implicit (i.e., non-boolean and pointer expressions compare explicitly with ''0'' and ''NULL'' respectively.)<br />
<pre><br />
right:<br />
if (!writing && /* not writing? */<br />
inode != NULL && /* valid inode? */<br />
ref_count == 0) /* no more references? */<br />
do_this();<br />
<br />
wrong:<br />
if (writing == 0 && /* not writing? */<br />
inode && /* valid inode? */<br />
!ref_count) /* no more references? */<br />
do_this();<br />
</pre><br />
<br />
* Use parentheses to help readability and reduce the chance of operator precedence errors.<br />
<pre><br />
right:<br />
if (a->a_field == 3 ||<br />
((b->b_field & BITMASK1) && (c->c_field & BITMASK2)))<br />
do this();<br />
<br />
wrong:<br />
if (a->a_field == 3 || b->b_field & BITMASK1 && c->c_field & BITMASK2)<br />
do this()<br />
</pre><br />
<br />
=== Layout ===<br />
<br />
* Code can be much more readable if the simpler actions are taken first in a set of tests. Re-ordering conditions like this also eliminates excessive nesting.<br />
<pre><br />
right:<br />
list_for_each_entry(...) {<br />
<br />
if (!condition1) {<br />
do_sth1;<br />
continue;<br />
}<br />
<br />
do_sth2_1;<br />
do_sth2_2;<br />
...<br />
do_sth2_N;<br />
<br />
if (!condition2)<br />
break;<br />
<br />
do_sth3_1;<br />
do_sth3_2;<br />
...<br />
do_sth3_N;<br />
}<br />
wrong:<br />
list_for_each_entry(...) {<br />
if (condition1) {<br />
do_sth2_1;<br />
do_sth2_2;<br />
...<br />
do_sth2_N;<br />
if (condition2) {<br />
do_sth3_1;<br />
do_sth3_2;<br />
...<br />
do_sth3_N;<br />
continue;<br />
} <br />
break;<br />
} else {<br />
do_sth1;<br />
}<br />
}<br />
</pre><br />
<br />
* Variable should be declared one per line, type and name, even if there are multiple variables of the same type. For maximum readability, the names should be aligned on the same column.<br />
<pre><br />
right:<br />
int len;<br />
int count;<br />
struct inode *inode;<br />
<br />
wrong:<br />
int len, count;<br />
struct inode *inode;<br />
</pre><br />
<br />
* Even for short conditionals, the operation should be on a separate line:<br />
<pre><br />
if (foo)<br />
bar();<br />
</pre><br />
<br />
* When you wrap a line containing parenthesis, start the next line after the parenthesis so that the expression or argument is visually bracketed.<br />
<pre><br />
right:<br />
variable = do_something_complicated(long_argument, longer_argument,<br />
longest_argument(sub_argument,<br />
foo_argument),<br />
last_argument);<br />
<br />
if (some_long_condition(arg1, arg2, arg3) < some_long_value &&<br />
another_long_condition(very_long_argument_name,<br />
another_long_argument_name) ><br />
second_long_value) {<br />
...<br />
<br />
wrong:<br />
<br />
variable = do_something_complicated(long_argument, longer_argument,<br />
longest_argument(sub_argument, foo_argument),<br />
last_argument);<br />
<br />
if (some_long_condition(arg1, arg2, arg3) < some_long_value &&<br />
another_long_condition(very_long_argument_name,<br />
another_long_argument_name) ><br />
second_long_value) {<br />
...<br />
</pre><br />
<br />
* If you're wrapping an expression, put the operator at the end of the line. If there are no parentheses to which to align the start of the next line, just indent 8 more spaces.<br />
<pre><br />
off = le32_to_cpu(fsd->fsd_client_start) +<br />
cl_idx * le16_to_cpu(fsd->fsd_client_size);<br />
</pre><br />
<br />
* Binary and ternary (but not unary) operators should be separated from their arguments by one space.<br />
<pre><br />
a++;<br />
b |= c;<br />
d = (f > g) ? 0 : 1;<br />
</pre><br />
<br />
* Function calls should be nestled against the parentheses, the parentheses should crowd the arguments, and one space should appear after commas:<br />
<pre><br />
right: <br />
do_foo(bar, baz);<br />
<br />
wrong:<br />
do_foo ( bar,baz );<br />
</pre><br />
<br />
* Put a space between ''if'', ''for'', ''while'' etc. and the following parenthesis. Put a space after each semicolon in a ''for'' statement.<br />
<pre><br />
for (a = 0; a < b; a++)<br />
if (a < b || a == c)<br />
while (1)<br />
</pre><br />
<br />
* Opening braces should be on the same line as the line that introduces the block, except for function calls. Bare closing braces (i.e. not ''else'' or ''while'' in do/while) get their own line. <br />
<pre><br />
int foo(void)<br />
{<br />
if (bar) {<br />
this();<br />
that();<br />
} else if (baz) {<br />
stuff();<br />
} else {<br />
other_stuff();<br />
}<br />
<br />
do {<br />
cow();<br />
} while (condition);<br />
}<br />
</pre><br />
<br />
* If one part of a compound ''if'' block has braces, all should.<br />
<pre><br />
right:<br />
if (foo) {<br />
bar();<br />
baz();<br />
} else {<br />
salmon();<br />
}<br />
<br />
wrong:<br />
if (foo) {<br />
bar();<br />
baz();<br />
} else<br />
moose();<br />
</pre><br />
<br />
* When you define a macro, protect callers by placing parentheses round every parameter reference in the body. Line up the backslashes of multi-line macros to help readability. Use a do/while (0) block with ''no'' trailing semicolon to ensure multi-statement macros are syntactically equivalent to procedure calls.<br />
<pre><br />
/* right */<br />
#define DO_STUFF(a) \<br />
do { \<br />
int b = (a) + MAGIC; \<br />
do_other_stuff(b); \<br />
} while (0)<br />
<br />
/* wrong */<br />
#define DO_STUFF(a) \<br />
do { \<br />
int b = a + MAGIC; \<br />
do_other_stuff(b); \<br />
} while (0);<br />
</pre><br />
<br />
* If you write conditionally compiled code in a procedure body, make sure you do not create unbalanced braces, quotes, etc. This really confuses editors that navigate expressions or use fonts to highlight language features. It can often be much cleaner to put the conditionally compiled code in its own helper function which, by good choice of name, documents itself too.<br />
<pre><br />
/* right */<br />
static inline int invalid_dentry(struct dentry *d)<br />
{<br />
#ifdef DCACHE_LUSTRE_INVALID<br />
return d->d_flags & DCACHE_LUSTRE_INVALID;<br />
#else<br />
return d_unhashed(d);<br />
#endif<br />
}<br />
<br />
int do_stuff(struct dentry *parent)<br />
{<br />
if (invalid_dentry(parent)) {<br />
...<br />
<br />
/* wrong */<br />
int do_stuff(struct dentry *parent)<br />
{<br />
#ifdef DCACHE_LUSTRE_INVALID<br />
if (parent->d_flags & DCACHE_LUSTRE_INVALID) {<br />
#else<br />
if (d_unhashed(parent)) {<br />
#endif<br />
...<br />
</pre><br />
<br />
* If you nest preprocessor commands, use spaces to visually delineate:<br />
<pre><br />
#ifdef __KERNEL__<br />
# include <goose><br />
# define MOOSE steak<br />
#else<br />
# include <mutton><br />
# define MOOSE prancing<br />
#endif<br />
</pre><br />
<br />
* For very long #ifdefs, include the conditional with each #endif to make it readable:<br />
<pre><br />
#ifdef __KERNEL__<br />
# if LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,0)<br />
/* lots<br />
of<br />
stuff */<br />
# endif /* KERNEL_VERSION(2,5,0) */<br />
#else /* !__KERNEL__ */<br />
# if HAVE_FEATURE<br />
/* more<br />
* stuff */<br />
# endif<br />
#endif /* __KERNEL__ */<br />
</pre><br />
<br />
* Comments should have the leading '/*' on the same line as the comment and the trailing '*/' at the end of the last comment line. Intermediate lines should start with a '*' aligned with the '*' on the first line:<br />
<pre><br />
/* This is a short comment */<br />
<br />
/* This is a multi-line comment. I wish the line would wrap already,<br />
* as I don't have much to write about. */<br />
</pre><br />
<br />
* Function declarations absolutely should NOT go into .c files, unless they are forward declarations for static functions that can't otherwise be moved before the caller. Instead, the declaration should go into the most "local" header available (preferably *_internal.h for a given piece of code).<br />
<br />
* Structure and constant declarations should not be declared in multiple places. Put the struct into the most "local" header possible. If it is something that is passed over the wire, it needs to go into lustre_idl.h and needs to be correctly swabbed when the RPC message is unpacked.<br />
<br />
* The types and printf/printk formats used by Lustre code are:<br />
<pre><br />
__u64 LPU64/LPX64/LPD64 (unsigned, hex, signed)<br />
size_t LPSZ (or cast to int and use %u / %d)<br />
__u32/int %u/%x/%d (unsigned, hex, signed)<br />
(unsigned) long long %llu/%llx/%lld<br />
loff_t %lld after a cast to long long (unfortunately)<br />
</pre><br />
<br />
* For Autoconf macros, follow the [http://www.gnu.org/software/autoconf/manual/html_node/Coding-Style.html style suggested in the autoconf manual].<br />
<br />
<pre><br />
AC_CACHE_CHECK([for EMX OS/2 environment], [ac_cv_emxos2],<br />
[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([], [return __EMX__;])],<br />
[ac_cv_emxos2=yes],<br />
[ac_cv_emxos2=no])])<br />
</pre><br />
:or_even<br />
<pre><br />
AC_CACHE_CHECK([for EMX OS/2 environment],<br />
[ac_cv_emxos2],<br />
[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([],<br />
[return __EMX__;])],<br />
[ac_cv_emxos2=yes],<br />
[ac_cv_emxos2=no])])<br />
</pre><br />
<br />
----</div>Eebhttp://wiki.old.lustre.org/index.php?title=Documenting_Code&diff=6993Documenting Code2009-09-09T22:02:24Z<p>Eeb: </p>
<hr />
<div>Lustre code documentation exists to help engineers working on the code to read and modify it correctly. It assumes the reader has a good overall grasp of the [[Learn:Lustre_Internals|Lustre architecture and internals]]) and it provides reference information on the application programming interfacess (APIs) and significant internal features of each [[Subsystem Map|Lustre subsystem]]. It consists of stylized comments embedded in the source code. This helps to keep the documentation consistent as the code is developed and it can be processed by [http://www.doxygen.org doxygen] into online browsable (HTML) documentation.<br />
<br />
== Requirements ==<br />
<br />
The minimum requirement for Lustre code documentation is to describe subsystem APIs - the datatypes, procedures and globals subsystem exports to the rest of Lustre - and significant internal datatypes. These should be described as follows:<br />
<br />
* Datatypes (structs, typedefs, enums)<br />
** What it is for<br />
** Structure members<br />
** Usage constraints<br />
* Procedures<br />
** What it does<br />
** Parameters<br />
** Return values<br />
** Usage constraints<br />
** Subtle implementation details<br />
* Globals<br />
** What it is for<br />
** Usage constraints<br />
<br />
The ''most important'' information is "Usage constraints" and "Subtle implementation details". "Usage constraints" means restrictions on how and when you call a procedure or operate on a datastructure. These include concurrency control, reference counting, permitted caller context etc. etc. "Subtle implementation details" means anything done in the code that might not be transparently obvious - e.g. code that ensures the last thread in a pool of workers is held in reserve for deadlock avoidance. Good choices of descriptive names can allow the other documentation (e.g. what the procedure does, what a parameter means) to be quite brief or even omitted. But usage constraints and implementation subtleties must always be spelled out, e.g. by describing an object's entire lifecycle from creation through to destruction, so that the next engineer to maintain or use the code does it safely and correctly. <br />
<br />
Each time you make a change to the Lustre code or inspect a patch, you must review the changes to ensure:<br />
<br />
* Sufficient documentation exists.<br />
* The documentation is accurate and up to date.<br />
<br />
== Examples ==<br />
<br />
Doxygen comments start with [http://www.doxygen.org/docblocks.html '''/**'''] (like in [http://en.wikipedia.org/wiki/Javadoc javadoc]). Doxygen commands are placed in doxygen comments to control how the doxygen formats the output. They start with a backslash ('''\''') or at-sign ('''@'''), but we typically use the backslash and keep the at-sign for group blocks (see below). Don't use doxygen commands unnecessarily. The main purpose of code documentation is to be right there in the code for you to read when you're working on it, so it's most important that the comments should read like real C comments and not formatting gibberish.<br />
<br />
===Procedures and Globals===<br />
Document procedures and globals in the .c files, rather than in headers.<br />
<br />
<pre><br />
/**<br />
* Owns a page by IO.<br />
*<br />
* Waits until \a pg is in cl_page_state::CPS_CACHED state, and then switch it<br />
* into cl_page_state::CPS_OWNED state.<br />
*<br />
* \param io IO context which wants to own the page<br />
* \param pg page to be owned<br />
*<br />
* \pre !cl_page_is_owned(pg, io)<br />
* \post result == 0 iff cl_page_is_owned(pg, io)<br />
*<br />
* \retval 0 success<br />
*<br />
* \retval -ve failure, e.g., page was destroyed (and landed in<br />
* cl_page_state::CPS_FREEING instead of cl_page_state::CPS_CACHED).<br />
*<br />
* \see cl_page_disown()<br />
* \see cl_page_operations::cpo_own()<br />
*/<br />
int cl_page_own(const struct lu_env *env, struct cl_io *io, struct cl_page *pg)<br />
</pre><br />
<br />
Notes:<br />
* Start with a brief description, which continues to the first '.' (period or full stop).<br />
* Follow the brief description with a detailed description.<br />
* Descriptions are written in the third person singular, e.g. "<this function> does this and that", "<this datatype> represents such and such a concept".<br />
* To refer to a function argument, use the [http://www.doxygen.org/commands.html#cmda '''\a argname'''] syntax.<br />
* To refer to another function, use the [http://www.doxygen.org/autolink.html '''funcname()'''] syntax. This will produce a cross-reference.<br />
* To refer to a field or an enum value use the [http://www.doxygen.org/autolink.html '''SCOPE::NAME'''] syntax.<br />
* Describe possible return values with [http://www.doxygen.org/commands.html#cmdretval '''\retval'''].<br />
* Mention all concurrency control restrictions here (such as locks that the function expects to be held, or holds on exit).<br />
* If possible, specify a (weakest) pre-condition and (strongest) post-condition for the function. If conditions cannot be expressed as a C language expression, provide an informal description.<br />
* Enumerate related functions and datatypes in the [http://www.doxygen.org/commands.html#cmdsee '''\see'''] section. Note, that doxygen will automatically cross-reference all places where a given function is called (but not through a function pointer) and all functions that it calls, so there is no need to enumerate all this.<br />
<br />
===Datatypes===<br />
Document datatypes where they are declared.<br />
<br />
<pre><br />
/**<br />
* "Compound" object, consisting of multiple layers.<br />
*<br />
* Compound object with given fid is unique with given lu_site.<br />
*<br />
* Note, that object does *not* necessary correspond to the real object in the<br />
* persistent storage: object is an anchor for locking and method calling, so<br />
* it is created for things like not-yet-existing child created by mkdir or<br />
* create calls. lu_object_operations::loo_exists() can be used to check<br />
* whether object is backed by persistent storage entity.<br />
*/<br />
struct lu_object_header {<br />
/**<br />
* Object flags from enum lu_object_header_flags. Set and checked<br />
* atomically.<br />
*/<br />
unsigned long loh_flags;<br />
/**<br />
* Object reference count. Protected by lu_site::ls_guard.<br />
*/<br />
atomic_t loh_ref;<br />
/**<br />
* Fid, uniquely identifying this object.<br />
*/<br />
struct lu_fid loh_fid;<br />
/**<br />
* Common object attributes, cached for efficiency. From enum<br />
* lu_object_header_attr.<br />
*/<br />
__u32 loh_attr;<br />
/**<br />
* Linkage into per-site hash table. Protected by lu_site::ls_guard.<br />
*/<br />
struct hlist_node loh_hash;<br />
/**<br />
* Linkage into per-site LRU list. Protected by lu_site::ls_guard.<br />
*/<br />
struct list_head loh_lru;<br />
/**<br />
* Linkage into list of layers. Never modified once set (except lately<br />
* during object destruction). No locking is necessary.<br />
*/<br />
struct list_head loh_layers;<br />
};<br />
</pre><br />
<br />
Describe datatype invariants (preferably formally).<br />
<br />
<pre><br />
/**<br />
* Fields are protected by the lock on cfs_page_t, except for atomics and<br />
* immutables.<br />
*<br />
* \invariant Datatype invariants are in cl_page_invariant(). Basically:<br />
* cl_page::cp_parent and cl_page::cp_child are a well-formed double-linked<br />
* list, consistent with the parent/child pointers in the cl_page::cp_obj and<br />
* cl_page::cp_owner (when set).<br />
*/<br />
struct cl_page {<br />
/** Reference counter. */<br />
atomic_t cp_ref;<br />
</pre><br />
<br />
Describe concurrency control mechanisms for structure fields.<br />
<br />
<pre><br />
/** An object this page is a part of. Immutable after creation. */<br />
struct cl_object *cp_obj;<br />
/** Logical page index within the object. Immutable after creation. */<br />
pgoff_t cp_index;<br />
/** List of slices. Immutable after creation. */<br />
struct list_head cp_layers;<br />
...<br />
};<br />
</pre><br />
<br />
Specify when fields are valid.<br />
<br />
<pre><br />
/**<br />
* Owning IO in cl_page_state::CPS_OWNED state. Sub-page can be owned<br />
* by sub-io.<br />
*/<br />
struct cl_io *cp_owner;<br />
/**<br />
* Owning IO request in cl_page_state::CPS_PAGEOUT and<br />
* cl_page_state::CPS_PAGEIN states. This field is maintained only in<br />
* the top-level pages.<br />
*/<br />
struct cl_req *cp_req;<br />
</pre><br />
<br />
You can use [http://www.doxygen.org/grouping.html#memgroup '''@{'''...'''@}'''] syntax to define a subset of fields or enum values, which should be grouped together.<br />
<br />
<pre><br />
struct cl_object_header {<br />
/** Standard lu_object_header. cl_object::co_lu::lo_header points<br />
* here. */<br />
struct lu_object_header coh_lu;<br />
/** \name locks<br />
* \todo XXX move locks below to the separate cache-lines, they are<br />
* mostly useless otherwise.<br />
*/<br />
/** @{ */<br />
/** Lock protecting page tree. */<br />
spinlock_t coh_page_guard;<br />
/** Lock protecting lock list. */<br />
spinlock_t coh_lock_guard;<br />
/** @} locks */<br />
</pre><br />
<br />
By default, a documenting comment goes immediately before the entity being commented. If it is necessary to place this comment separately (e.g. to streamline comments in the header file), use the following syntax.<br />
<br />
<pre><br />
/** \struct cl_page<br />
* Layered client page.<br />
*<br />
* cl_page: represents a portion of a file, cached in the memory. All pages<br />
* of the given file are of the same size, and are kept in the radix tree<br />
</pre><br />
<br />
===Subsystem Overview===<br />
<br />
To document a subsystem, add the following comment to the header file which contains the definitions of its key datatypes. This will group all the documentation in the '''@{'''...'''@}''' block.<br />
<br />
<pre><br />
/** \defgroup component_name Component Name<br />
*<br />
* overall module documentation<br />
* ...<br />
*<br />
* @{ <br />
*/<br />
datatype definitions...<br />
exported functions...<br />
/** @} component_name */<br />
</pre><br />
<br />
"component_name" is a single word name that identifies a group to doxygen. "Component Name" is the printable title of the group. It extends to the end of the line. See [http://doxygen.org/commands.html#cmddefgroup \defgroup] for more details.<br />
<br />
To separate a logical part of a larger component, add the following somewhere within the component's \defgroup:<br />
<br />
<pre><br />
/**<br />
* \name Printable Title of sub-component<br />
*<br />
* Description of a sub-component<br />
*/<br />
/** @{ */<br />
datatype definitions...<br />
exported functions...<br />
/** @} */<br />
</pre><br />
<br />
If an exported function prototype in a header is located within some group, the appropriate function definition in a .c file is automatically assigned to the same group.<br />
<br />
A set of comments that is not lexically a part of a group can be included into it with the \addtogroup command. It works just like "\defgroup", but the printable group title is optional. See [http://doxygen.org/commands.html#cmdaddtogroup] for full details.<br />
<br />
<pre><br />
/** \addtogroup cl_object<br />
* @{ */<br />
/**<br />
* "Data attributes" of cl_object. Data attributes can be updated<br />
* independently for a sub-object, and top-object's attributes are calculated<br />
* from sub-objects' ones.<br />
*/<br />
struct cl_attr {<br />
/** Object size, in bytes */<br />
loff_t cat_size;<br />
...<br />
};<br />
...<br />
/** @} cl_object */<br />
</pre><br />
<br />
<br />
== Running Doxygen ==<br />
You need to install the "graphviz" package before you can run doxygen.<br />
<br />
Doxygen uses a ''configuration file'' to control how it builds documentation. See [http://www.doxygen.org/config.html Doxygen Configuration] for all the details.<br />
<br />
Lustre comes with two configuration files:<br />
* '''build/doxyfile.ref''' produces a ''short'' form of the documentation set, suitable as a reference. Output is placed into the doxygen.ref/ directory.<br />
* '''build/doxyfile.api''' produces a full documentation set, more suitable for a learning code structure. In addition to the short form, this set includes call-graphs and source code excerpts. Output is placed into the doxygen.api/ directory.<br />
<br />
If the version of doxygen you are running is newer than the one last used to generate the configuration files, run the following commands to upgrade:<br />
<pre><br />
doxygen -s -u build/doxyfile.api<br />
doxygen -s -u build/doxyfile.ref<br />
</pre><br />
<br />
To build all the documentation, in the top-level lustre directory run:<br />
<pre><br />
doxygen build/doxyfile.api<br />
doxygen build/doxyfile.ref<br />
</pre><br />
<br />
There are also phony Makefile targets '''doxygen-api''' and '''doxygen-ref''' to run these commands and '''doxygen''' to run both.<br />
<br />
Note that doxygen currently gives many warnings about undocumented entities. These should abate as we improve the code documentation.<br />
<br />
== Publishing Documention ==<br />
<br />
The build/publish_doxygen script publishes a local version of the documentation at "http://wiki.lustre.org/doxygen":<br />
<br />
<pre><br />
build/publish_doxygen [-b branchname] [-l additional-label] [-d] [-u user] [-p port]<br />
</pre><br />
<br />
The script tries to guess the branchname by looking into CVS/Tag. The user and port are used to ssh into shell.lustre.sun.com. User defaults to your $USER environment variable and port defaults to 922. -d instructs the script to use the current date as a label. <br />
<br />
Documentation is uploaded into...<br />
<br />
<pre><br />
user@shell.lustre.sun.com:/home/www/doxygen/$branch$label<br />
</pre><br />
where $label is a concatenation of all labels given on the command line in order. The parent directory is rsync-ed to wiki.lustre.org regularly and the documentation can be browsed at...<br />
<br />
<pre><br />
http://wiki.lustre.org:/doxygen<br />
</pre><br />
<br />
When adding a new branch/label, you have to edit ''index.html'' in the doxygen directory on shell.lustre.sun.com.<br />
<br />
== Doxygen References ==<br />
<br />
[http://www.doxygen.org/ Doxygen Home]<br />
<br />
[http://www.doxygen.org/manual.html/ Doxygen Manual]<br />
<br />
[http://www.doxygen.org/commands.html/ Doxygen Special Commands]</div>Eebhttp://wiki.old.lustre.org/index.php?title=Coding_Guidelines&diff=6947Coding Guidelines2009-09-09T10:42:07Z<p>Eeb: </p>
<hr />
<div>== Beautiful Code ==<br />
<br />
''A note from Eric Barton, our lead engineer:''<br />
<br />
More important than the physical layout of code (which is covered in detail below) is the idea that the code should be ''beautiful'' to read.<br />
<br />
What makes code beautiful to me? Fundamentally, it's readability and obviousness. The code must not have secrets but should flow easily, pleasurably and ''accurately'' off the page and into the mind of the reader.<br />
<br />
How do I think beautiful code is written? Like this...<br />
<br />
* The author must be confident and knowledgeable and proud of her work. She must understand what the code should do, the environment it must work in, all the combinations of inputs, all the valid outputs, all the possible races and all the reachable states. She must [http://en.wikipedia.org/wiki/Grok grok] it.<br />
<br />
* Names must be well chosen. The meaning a human reader attaches to a name can be orthogonal to what the compiler does with it so it's just as easy to mislead as it is to inform. ''[http://en.wikipedia.org/wiki/Does_what_it_says_on_the_tin "Does exactly what it says on the tin"]'' is a popular UK English expression describing something that does ''exactly'' what it tells you it's going to do, no more and no less. For example, if I open a tin labelled "soap", I expect the contents to help me wash and maybe even smell nice. If it's no good at removing dirt, I'll be disappointed. If it removes the dirt but burns off a layer of skin with it, I'll be positively upset. The name of a procedure, a variable or a structure member should tell you something informative about the entity without misleading - just "what it says on the tin".<br />
<br />
* Names must be well chosen. Local, temporary variables can almost always remain relatively short and anonymous, while names in global scope must be unique. In general, the wider the context you expect to use the name in, the more unique and informative the name should be. Don't be scared of long names if they help to '''make_the_code_clearer''', but '''do_not_let_things_get_out_of_hand''' either - we don't write COBOL. Related names should be obvious, unambiguous and avoid naming conflicts with other unrelated names, e.g. by using a consistent prefix. This applies to all API procedures (if not all procedures period) within a given subsystem. Similarly, unique member names for global structures, using a prefix to identify the parent structure type, helps readability.<br />
<br />
* Names must be well chosen. Don't choose names that are easily confused - especially not if the compiler can't even tell the difference when you make a spelling mistake. '''i''' and '''j''' aren't the worst example - '''req_portal''' and '''rep_portal''' are much worse (and taken from our own code!!!).<br />
<br />
* Names must be well chosen. I can't emphasize this issue enough - I hope you get the point.<br />
<br />
* Assertions must be used intelligently. They can be abused - obviously, when over-used, they hurt performance and can even convey the impression the author didn't know what she was doing and added them to help her ''learn'' the code. But when assertions are chosen carefully and used properly, they combine the roles of ''active comment'' and ''software fuse''. As an ''active comment'' they tell you something about the program which you can trust more than a comment. And as a ''software fuse'', they provide fault isolation between subsystems by letting you know when and where invariant assumptions are violated.<br />
<br />
* Formatting and indentation rules should be followed intelligently. The visual layout of the code on the page should lend itself to being read easily and accurately - it just looks clean and good.<br />
** Separate "ideas" should be separated clearly in the code layout using blank lines that group related statements and separate unrelated statements.<br />
** Procedures should not ramble on. You must be able to take in the meaning of a procedure without scrolling past page after page of code or parsing deeply nested conditionals and loops. The 80-character rule is there for a reason.<br />
** Declarations are easier to refer to while scanning the code if placed in a block locally to, but visually separate from, the code that uses them. Readability is further enhanced by limiting declarations to one per line and aligning types and names vertically.<br />
** Parameters in multi-line procedure calls should be aligned so that they are visually contained by their brackets.<br />
** Brackets should be used in expressions to make operator precedence clear.<br />
** Conditional boolean ('''if (expr)'''), scalar ('''if (val != 0)''') and pointer ('''if (ptr != NULL)''') expressions should be written consistently.<br />
** Formatting and indentation rules should not be followed slavishly. If you're faced with either breaking the 80-chars-per-line rule or the parameter indentation rule or creating an obscure helper function, then the 80-chars-per-line rule might have to suffer. The overriding consideration is how the code reads.<br />
<br />
I could go on, but I hope you get the idea. Just think about the poor reader when you're writing, and whether your code will convey its meaning naturally, quickly and accurately, without room for misinterpretation. I didn't mention ''clever'' as a feature of beautiful code because it's only one step from ''clever'' to ''tricky'' - consider...<br />
<br />
t = a; a = b; b = t; /* dumb swap */<br />
<br />
a ^= b; b ^= a; a ^= b; /* clever swap */<br />
<br />
You could feel quite pleased that the clever swap avoids the need for a local temporary variable - but is that such a big deal compared with how quickly, easily and accurately the reader will read it? This is a very minor example which can almost be excused because the "cleverness" is confined to a tiny part of the code. But when ''clever'' code gets spread out it becomes much harder to modify without adding defects. You can only work on code without screwing up if you understand the code ''and'' the environment it works in completely. Or to put it more succinctly...<br />
<br />
''Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it.'' - [http://en.wikipedia.org/wiki/Brian_Kernighan Brian W. Kernighan]<br />
<br />
IMHO, beautiful code helps code quality because it improves communication between the code author and the code reader. Since everyone maintaining and developing the code is a code reader as well as a code author, the quality of this communication can lead either to a virtuous circle of improving quality, or a vicious circle of degrading quality. You, dear reader, will determine which.<br />
<br />
== Style and Formatting Guidlelines ==<br />
<br />
All of our rules for formatting, wrapping, parenthesis, brace placement, etc., are originally derived from the [http://www.kernel.org/doc/Documentation/CodingStyle Linux kernel rules], which are basically K&R.<br />
<br />
=== Whitespace ===<br />
<br />
Whitespace gets its own section because unnecessary whitespace changes can cause spurious merge conflicts when landing and updating in a distributed development environment. Please ensure that you comply with the guidelines in this section to avoid these issues - we've included default formatting rules for emacs and vim to help make it easier.<br />
<br />
* There should be no tabs in any Lustre, LNET or libcfs files. The exceptions are libsysio (maintained by someone else), ldiskfs and kernel patches (also part of a non-Lustre Group project).<br />
<br />
* Blocks should be indented 8 spaces.<br />
<br />
* New files should contain the following along with the license boilerplate. This will cause vim and emacs to use spaces instead of tabs for indenting. If you use a different editor, it also needs to be set to use spaces for indenting Lustre code.<br />
<pre><br />
/* -*- mode: c; c-basic-offset: 8; indent-tabs-mode: nil; -*-<br />
* vim:expandtab:shiftwidth=8:tabstop=8:<br />
*/<br />
</pre><br />
<br />
* All lines should wrap at 80 characters. If it's getting too hard to wrap at 80 characters, you probably need to break it up into more functions.<br />
<br />
* Do not have spaces or tabs on blank lines or at the end of lines. Please ensure you remove all instances of these in any patches you submit to bugzilla. You can find them with grep or in vim using the following regexps...<br />
<pre><br />
/[ \t]$/<br />
</pre><br />
<br />
:Alternatively, if you use vim you can put this line in your vimrc file, which will highlight whitespace at the end of lines and spaces followed by tabs in indentation (only works for C/C++ files):<br />
<pre><br />
let c_space_errors=1<br />
</pre><br />
<br />
:Or you can use this command, which will make tabs and whitespace at the end of lines visible for all files (but a bit more discretely):<br />
<pre><br />
set list listchars=tab:>\ ,trail:$<br />
</pre><br />
<br />
:In emacs, you can use (whitespace-mode) or (whitespace-visual-mode) depending on the version. You could also consider using (flyspell-prog-mode).<br />
<br />
=== C Language Features ===<br />
<br />
* Don't use '''inline''' unless you're doing something so performance critical that the function call overhead will make a difference -- in other words: almost never. It makes debugging harder and overuse can actually hurt performance by causing instruction cache overflow.<br />
<br />
* Use '''typedef''' carefully...<br />
** Do not create a new integer typedef without a really good reason.<br />
** Always postfix typedef names with '_t' so that they can be identified clearly in the code.<br />
** ''Never'' typedef pointers. The '*' makes C pointer declarations obvious - hiding it inside a typedef just obfuscates the code.<br />
<br />
* Do not embed assignments inside boolean expressions. Although this can make the code more concise, it doesn't necessarily make it more elegant and you increase the risk of confusing '=' with '==' or getting operator precedence wrong if you skimp on brackets. It's even easier to make mistakes when reading the code so it's much safer simply to avoid it altogether.<br />
<pre><br />
right:<br />
ptr = malloc(size);<br />
if (ptr != NULL) {<br />
...<br />
<br />
wrong:<br />
if ((ptr = malloc(size)) != NULL) {<br />
...<br />
</pre><br />
<br />
* Conditional expressions read more clearly if only boolean expressions are implicit - i.e. non-boolean and pointer expressions compare explicitly with 0 and NULL respectively.<br />
<pre><br />
right:<br />
if (!writing && /* not writing? */<br />
inode != NULL && /* valid inode? */<br />
ref_count == 0) /* no more references? */<br />
do_this();<br />
<br />
wrong:<br />
if (writing == 0 && /* not writing? */<br />
inode && /* valid inode? */<br />
!ref_count) /* no more references? */<br />
do_this();<br />
</pre><br />
<br />
* Use parentheses to help readability and reduce the chance of operator precedence errors.<br />
<pre><br />
right:<br />
if (a->a_field == 3 ||<br />
((b->b_field & BITMASK1) && (c->c_field & BITMASK2)))<br />
do this();<br />
<br />
wrong:<br />
if (a->a_field == 3 || b->b_field & BITMASK1 && c->c_field & BITMASK2)<br />
do this()<br />
</pre><br />
<br />
=== Layout ===<br />
<br />
* Variable should be declared one per line, type and name, even if there are multiple variables of the same type. For maximum readability, the names should be aligned on the same column.<br />
<pre><br />
right:<br />
int len;<br />
int count;<br />
struct inode *inode;<br />
<br />
wrong:<br />
int len, count;<br />
struct inode *inode;<br />
</pre><br />
<br />
* Even for short conditionals, the operation should be on a separate line:<br />
<pre><br />
if (foo)<br />
bar();<br />
</pre><br />
<br />
* When you wrap a line containing parenthesis, start the next line after the parenthesis so that the expression or argument is visually bracketted.<br />
<pre><br />
right:<br />
variable = do_something_complicated(long_argument, longer_argument,<br />
longest_argument(sub_argument,<br />
foo_argument),<br />
last_argument);<br />
<br />
if (some_long_condition(arg1, arg2, arg3) < some_long_value &&<br />
another_long_condition(very_long_argument_name,<br />
another_long_argument_name) ><br />
second_long_value) {<br />
...<br />
<br />
wrong:<br />
<br />
variable = do_something_complicated(long_argument, longer_argument,<br />
longest_argument(sub_argument, foo_argument),<br />
last_argument);<br />
<br />
if (some_long_condition(arg1, arg2, arg3) < some_long_value &&<br />
another_long_condition(very_long_argument_name,<br />
another_long_argument_name) ><br />
second_long_value) {<br />
...<br />
</pre><br />
<br />
* If you're wrapping an expression, put the operator at the end of the line. If there are no parentheses to align the start of the next line to, just indent 8 more spaces.<br />
<pre><br />
off = le32_to_cpu(fsd->fsd_client_start) +<br />
cl_idx * le16_to_cpu(fsd->fsd_client_size);<br />
</pre><br />
<br />
* Binary and ternary (but not unary) operators should be separated from their arguments by one space.<br />
<pre><br />
a++;<br />
b |= c;<br />
d = (f > g) ? 0 : 1;<br />
</pre><br />
<br />
* Function calls should be nestled against the parentheses, the parentheses should crowd the arguments, and one space should appear after commas:<br />
<pre><br />
right: <br />
do_foo(bar, baz);<br />
<br />
wrong:<br />
do_foo ( bar,baz );<br />
</pre><br />
<br />
* Put a space between '''if''', '''for''', '''while''' etc. and the following parenthesis. Put a space after each semicolon in a '''for''' statement.<br />
<pre><br />
for (a = 0; a < b; a++)<br />
if (a < b || a == c)<br />
while (1)<br />
</pre><br />
<br />
* Opening braces should be on the same line as the line that introduces the block, except for function calls. Bare closing braces (i.e. not '''else''' or '''while''' in do/while) get their own line. <br />
<pre><br />
int foo(void)<br />
{<br />
if (bar) {<br />
this();<br />
that();<br />
} else if (baz) {<br />
stuff();<br />
} else {<br />
other_stuff();<br />
}<br />
<br />
do {<br />
cow();<br />
} while (condition);<br />
}<br />
</pre><br />
<br />
* If one part of a compound '''if''' block has braces, all should.<br />
<pre><br />
right:<br />
if (foo) {<br />
bar();<br />
baz();<br />
} else {<br />
salmon();<br />
}<br />
<br />
wrong:<br />
if (foo) {<br />
bar();<br />
baz();<br />
} else<br />
moose();<br />
</pre><br />
<br />
* When you defining a macro, protect callers by parenthesising all parameter usage in the body. Line up the backslashes of multi-line macros to help readability. Use a do/while (0) block with ''no'' trailing semicolon to ensure multi-statement macros are syntactically equivalent to procedure calls.<br />
<pre><br />
/* right */<br />
#define DO_STUFF(a) \<br />
do { \<br />
int b = (a) + MAGIC; \<br />
do_other_stuff(b); \<br />
} while (0)<br />
<br />
/* wrong */<br />
#define DO_STUFF(a) \<br />
do { \<br />
int b = a + MAGIC; \<br />
do_other_stuff(b); \<br />
} while (0);<br />
</pre><br />
<br />
* If you write conditionally compiled code in a procedure body, ensure you do not create unbalanced braces, quotes etc. This really confuses editors that navigate expressions or can fontify language features. It can often be much cleaner to put the conditionally compiled code in its own helper function which, by good choice of name, documents itself too.<br />
<pre><br />
/* right */<br />
static inline int invalid_dentry(struct dentry *d)<br />
{<br />
#ifdef DCACHE_LUSTRE_INVALID<br />
return d->d_flags & DCACHE_LUSTRE_INVALID;<br />
#else<br />
return d_unhashed(d);<br />
#endif<br />
}<br />
<br />
int do_stuff(struct dentry *parent)<br />
{<br />
if (invalid_dentry(parent)) {<br />
...<br />
<br />
/* wrong */<br />
int do_stuff(struct dentry *parent)<br />
{<br />
#ifdef DCACHE_LUSTRE_INVALID<br />
if (parent->d_flags & DCACHE_LUSTRE_INVALID) {<br />
#else<br />
if (d_unhashed(parent)) {<br />
#endif<br />
...<br />
</pre><br />
<br />
* If you nest preprocessor commands, use spaces to visually delineate:<br />
<pre><br />
#ifdef __KERNEL__<br />
# include <goose><br />
# define MOOSE steak<br />
#else<br />
# include <mutton><br />
# define MOOSE prancing<br />
#endif<br />
</pre><br />
<br />
* For very long #ifdefs, include the conditional with each #endif to make it readable:<br />
<pre><br />
#ifdef __KERNEL__<br />
# if LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,0)<br />
/* lots<br />
of<br />
stuff */<br />
# endif /* KERNEL_VERSION(2,5,0) */<br />
#else /* !__KERNEL__ */<br />
# if HAVE_FEATURE<br />
/* more<br />
* stuff */<br />
# endif<br />
#endif /* __KERNEL__ */<br />
</pre><br />
<br />
* Comments should have the leading '/*' on the same line as the comment, and the trailing '*/' at the end of the last comment line. Intermediate lines should start with a '*' aligned with the first line's '*':<br />
<pre><br />
/* This is a short comment */<br />
<br />
/* This is a multi-line comment. I wish the line would wrap already,<br />
* as I don't have much to write about. */<br />
</pre><br />
<br />
* Function declarations absolutely should NOT go into .c files, unless they are forward declarations for static functions that can't otherwise be moved before the caller. Instead, the declaration should go into the most "local" header available (preferably *_internal.h for a given piece of code).<br />
<br />
* Structure and constant declarations should not be declared in multiple places. Put the struct into the most "local" header possible. If it is something that is passed over the wire, it needs to go into lustre_idl.h and needs to be correctly swabbed when the RPC message is unpacked.<br />
<br />
* The types and printf/printk formats used by Lustre code are:<br />
<pre><br />
__u64 LPU64/LPX64/LPD64 (unsigned, hex, signed)<br />
size_t LPSZ (or cast to int and use %u / %d)<br />
__u32/int %u/%x/%d (unsigned, hex, signed)<br />
(unsigned) long long %llu/%llx/%lld<br />
loff_t %lld after a cast to long long (unfortunately)<br />
</pre><br />
<br />
* For Autoconf macros, follow the [http://www.gnu.org/software/autoconf/manual/html_node/Coding-Style.html style suggested in the autoconf manual].<br />
<br />
<pre><br />
AC_CACHE_CHECK([for EMX OS/2 environment], [ac_cv_emxos2],<br />
[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([], [return __EMX__;])],<br />
[ac_cv_emxos2=yes],<br />
[ac_cv_emxos2=no])])<br />
</pre><br />
:or_even<br />
<pre><br />
AC_CACHE_CHECK([for EMX OS/2 environment],<br />
[ac_cv_emxos2],<br />
[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([],<br />
[return __EMX__;])],<br />
[ac_cv_emxos2=yes],<br />
[ac_cv_emxos2=no])])<br />
</pre><br />
<br />
----</div>Eebhttp://wiki.old.lustre.org/index.php?title=Coding_Guidelines&diff=6859Coding Guidelines2009-09-04T15:00:25Z<p>Eeb: </p>
<hr />
<div>== Beautiful Code ==<br />
<br />
''A note from Eric Barton, our lead engineer:''<br />
<br />
More important than the physical layout of code (which is covered in detail below) is the idea that the code should be ''beautiful'' to read.<br />
<br />
What makes code beautiful to me? Fundamentally, it's readability and obviousness. The code must not have secrets but should flow easily, pleasurably and ''accurately'' off the page and into the mind of the reader.<br />
<br />
How do I think beautiful code is written? Like this...<br />
<br />
* The author must be confident and knowledgeable and proud of her work. She must understand what the code should do, the environment it must work in, all the combinations of inputs, all the valid outputs, all the possible races and all the reachable states. She must [http://en.wikipedia.org/wiki/Grok grok] it.<br />
<br />
* Names must be well chosen. The meaning a human reader attaches to a name can be orthogonal to what the compiler does with it so it's just as easy to mislead as it is to inform. ''[http://en.wikipedia.org/wiki/Does_what_it_says_on_the_tin "Does exactly what it says on the tin"]'' is a popular UK English expression describing something that does ''exactly'' what it tells you it's going to do, no more and no less. For example, if I open a tin labelled "soap", I expect the contents to help me wash and maybe even smell nice. If it's no good at removing dirt, I'll be disappointed. If it removes the dirt but burns off a layer of skin with it, I'll be positively upset. The name of a procedure, a variable or a structure member should tell you something informative about the entity without misleading - just "what it says on the tin".<br />
<br />
* Names must be well chosen. Local, temporary variables can almost always remain relatively short and anonymous, while names in global scope must be unique. In general, the wider the context you expect to use the name in, the more unique and informative the name should be. Don't be scared of long names if they help to '''make_the_code_clearer''', but '''do_not_let_things_get_out_of_hand''' either - we don't write COBOL. Related names should be obvious, unambiguous and avoid naming conflicts with other unrelated names, e.g. by using a consistent prefix. This applies to all API procedures (if not all procedures period) within a given subsystem. Similarly, unique member names for global structures, using a prefix to identify the parent structure type, helps readability.<br />
<br />
* Names must be well chosen. Don't choose names that are easily confused - especially not if the compiler can't even tell the difference when you make a spelling mistake. '''i''' and '''j''' aren't the worst example - '''req_portal''' and '''rep_portal''' are much worse (and taken from our own code!!!).<br />
<br />
* Names must be well chosen. I can't emphasize this issue enough - I hope you get the point.<br />
<br />
* Assertions must be used intelligently. They can be abused - obviously, when over-used, they hurt performance and can even convey the impression the author didn't know what she was doing and added them to help her ''learn'' the code. But when assertions are chosen carefully and used properly, they combine the roles of ''active comment'' and ''software fuse''. As an ''active comment'' they tell you something about the program which you can trust more than a comment. And as a ''software fuse'', they provide fault isolation between subsystems by letting you know when and where invariant assumptions are violated.<br />
<br />
* Formatting and indentation rules should be followed intelligently. The visual layout of the code on the page should lend itself to being read easily and accurately - it just looks clean and good.<br />
** Separate "ideas" should be separated clearly in the code layout using blank lines that group related statements and separate unrelated statements.<br />
** Procedures should not ramble on. You must be able to take in the meaning of a procedure without scrolling past page after page of code or parsing deeply nested conditionals and loops. The 80-character rule is there for a reason.<br />
** Declarations are easier to refer to while scanning the code if placed in a block locally to, but visually separate from, the code that uses them. Readability is further enhanced by limiting declarations to one per line and aligning types and names vertically.<br />
** Parameters in multi-line procedure calls should be aligned so that they are visually contained by their brackets.<br />
** Brackets should be used in expressions to make operator precedence clear.<br />
** Conditional boolean ('''if (expr)'''), scalar ('''if (val != 0)''') and pointer ('''if (ptr != NULL)''') expressions should be written consistently.<br />
** Formatting and indentation rules should not be followed slavishly. If you're faced with either breaking the 80-chars-per-line rule or the parameter indentation rule or creating an obscure helper function, then the 80-chars-per-line rule might have to suffer. The overriding consideration is how the code reads.<br />
<br />
I could go on, but I hope you get the idea. Just think about the poor reader when you're writing, and whether your code will convey its meaning naturally, quickly and accurately, without room for misinterpretation. I didn't mention ''clever'' as a feature of beautiful code because it's only one step from ''clever'' to ''tricky'' - consider...<br />
<br />
t = a; a = b; b = t; /* dumb swap */<br />
<br />
a ^= b; b ^= a; a ^= b; /* clever swap */<br />
<br />
You could feel quite pleased that the clever swap avoids the need for a local temporary variable - but is that such a big deal compared with how quickly, easily and accurately the reader will read it? This is a very minor example which can almost be excused because the "cleverness" is confined to a tiny part of the code. But when ''clever'' code gets spread out it becomes much harder to modify without adding defects. You can only work on code without screwing up if you understand the code ''and'' the environment it works in completely. Or to put it more succinctly...<br />
<br />
''Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it.'' - [http://en.wikipedia.org/wiki/Brian_Kernighan Brian W. Kernighan]<br />
<br />
IMHO, beautiful code helps code quality because it improves communication between the code author and the code reader. Since everyone maintaining and developing the code is a code reader as well as a code author, the quality of this communication can lead either to a virtuous circle of improving quality, or a vicious circle of degrading quality. You, dear reader, will determine which.<br />
<br />
== Coding Style ==<br />
<br />
All of our rules for formatting, wrapping, parenthesis, brace placement, etc., are originally derived from the [http://www.kernel.org/doc/Documentation/CodingStyle Linux kernel rules], which are basically K&R.<br />
<br />
=== Formatting Guidelines ===<br />
<br />
'''Note:''' ''All Lustre developers and contributors should follow these guidelines strictly to avoid spurious whitespace differences, which create problems later for code merges. Please ensure the default formatting rules in whichever editor you use comply with these guidelines.''<br />
<br />
* There should be no tabs in any Lustre, LNET or libcfs files. The exceptions are libsysio (maintained by someone else), ldiskfs and kernel patches (also part of a non-Lustre Group project).<br />
<br />
* Blocks should be indented 8 spaces.<br />
<br />
* New files should contain the following along with the license boilerplate. This will cause vim and emacs to use spaces instead of tabs for indenting. If you use a different editor, it also needs to be set to use spaces for indenting Lustre code.<br />
<pre><br />
/* -*- mode: c; c-basic-offset: 8; indent-tabs-mode: nil; -*-<br />
* vim:expandtab:shiftwidth=8:tabstop=8:<br />
*/<br />
</pre><br />
<br />
* All lines should wrap at 80 characters. If it's getting too hard to wrap at 80 characters, you probably need to break it up into more functions.<br />
<br />
* Do not have spaces or tabs on blank lines or at the end of lines. Please ensure you remove all instances of these in any patches you submit to bugzilla. You can find them with grep or in vim using the following regexps...<br />
<pre><br />
/[ \t]$/<br />
</pre><br />
<br />
:Alternatively, if you use vim you can put this line in your vimrc file, which will highlight whitespace at the end of lines and spaces followed by tabs in indentation (only works for C/C++ files):<br />
<pre><br />
let c_space_errors=1<br />
</pre><br />
<br />
:Or you can use this command, which will make tabs and whitespace at the end of lines visible for all files (but a bit more discretely):<br />
<pre><br />
set list listchars=tab:>\ ,trail:$<br />
</pre><br />
<br />
:In emacs, you can use (whitespace-mode) or (whitespace-visual-mode) depending on the version. You should also consider using (flyspell-prog-mode).<br />
<br />
=== Language Features ===<br />
<br />
* Don't use "inline" unless you're doing something so performance critical that the function call overhead will make a difference -- in other words: almost never. It makes debugging harder.<br />
<br />
* Use ''typedef'' carefully...<br />
** Do not create a new integer typedef without a really good reason.<br />
** Always postfix typedef names with '''_t''' so that they can be identified clearly in the code.<br />
** Never typedef pointers. The '''*''' makes C pointer declarations obvious - hiding it obfuscates the code.<br />
<br />
* Variable should be declared one per line, type and name, even if there are multiple variables of the same type. For maximum readability, the names should be aligned on the same column.<br />
<pre><br />
right:<br />
<br />
int len;<br />
int count;<br />
struct inode *inode;<br />
</pre><br />
<pre><br />
wrong:<br />
<br />
int len, count;<br />
struct inode *inode;<br />
</pre><br />
<br />
* Even for short conditionals, the operation should be on a separate line:<br />
<pre><br />
if (foo)<br />
bar();<br />
</pre><br />
<br />
* When you wrap, the next line should start after the parenthesis:<br />
<pre><br />
right:<br />
<br />
variable = do_something_complicated(long_argument, longer_argument,<br />
longest_argument(sub_argument,<br />
foo_argument),<br />
last_argument);<br />
<br />
if (some_long_condition(arg1, arg2, arg3) < some_long_value &&<br />
another_long_condition(very_long_argument_name,<br />
another_long_argument_name) ><br />
second_long_value) {<br />
}<br />
</pre><br />
<pre><br />
wrong:<br />
<br />
variable = do_something_complicated(long_argument, longer_argument,<br />
longest_argument(sub_argument, foo_argument),<br />
last_argument);<br />
<br />
if (some_long_condition(arg1, arg2, arg3) < some_long_value &&<br />
another_long_condition(very_long_argument_name,<br />
another_long_argument_name) ><br />
second_long_value) {<br />
}<br />
</pre><br />
* If you're wrapping, put the operators at the end of the line, and if there are no parentheses, indent 8 more:<br />
<pre><br />
off = le32_to_cpu(fsd->fsd_client_start) +<br />
cl_idx * le16_to_cpu(fsd->fsd_client_size);<br />
</pre><br />
* Binary and ternary (but not unary) operators should be separated from their arguments by one space.<br />
<pre><br />
a++;<br />
b |= c;<br />
d = f > g ? 0 : 1;<br />
</pre><br />
* Function calls should be nestled against the parentheses, the parentheses should crowd the arguments, and one space should appear after commas:<br />
<pre><br />
right: do_foo(bar, baz);<br />
wrong: do_foo ( bar,baz );<br />
</pre><br />
* All ''if'', ''for'', ''while'', etc. expressions should be separated by a space from the parenthesis, one space after the semicolons:<br />
<pre><br />
for (a = 0; a < b; a++)<br />
if (a < b || a == c)<br />
while (1)<br />
</pre><br />
* Opening braces should be on the same line as the line that introduces the block, except for function calls. Closing braces get their own line, except for "else".<br />
<pre><br />
int foo(void)<br />
{<br />
if (bar) {<br />
this();<br />
that();<br />
} else if (baz) {<br />
;<br />
} else {<br />
;<br />
}<br />
do {<br />
cow();<br />
} while (0);<br />
}<br />
</pre><br />
* If one part of a compound ''if'' block has braces, all should.<br />
<pre><br />
right:<br />
<br />
if (foo) {<br />
bar();<br />
baz();<br />
} else {<br />
salmon();<br />
}<br />
</pre><br />
<pre><br />
wrong:<br />
<br />
if (foo) {<br />
bar();<br />
baz();<br />
} else<br />
moose();<br />
</pre><br />
* When you make a macro, protect those who might call it by using do/while and parentheses. Line up your backslashes to help readability and note that there is '''no''' trailing semicolon after the 'while (0)'. Think about it...<br />
<pre><br />
right:<br />
<br />
#define DO_STUFF(a) \<br />
do { \<br />
int b = (a) + MAGIC; \<br />
do_other_stuff(b); \<br />
} while (0)<br />
</pre><br />
<pre><br />
wrong:<br />
<br />
#define DO_STUFF(a) \<br />
{ \<br />
int b = a + MAGIC; \<br />
do_other_stuff(b); \<br />
}<br />
</pre><br />
* If you write conditionally compiled code in a procedure body, ensure you do not create unbalanced braces, quotes etc. This really confuses editors that navigate expressions or can fontify language features. It can often be much cleaner to put the conditionally compiled code in its own helper function which, by good choice of name, documents itself too.<br />
<pre><br />
wrong:<br />
<br />
#ifdef DCACHE_LUSTRE_INVALID<br />
if (parent->d_flags & DCACHE_LUSTRE_INVALID) {<br />
#else<br />
if (d_unhashed(parent)) {<br />
#endif<br />
</pre><br />
<pre><br />
right:<br />
<br />
static inline int invalid_dentry(struct dentry *d)<br />
{<br />
#ifdef DCACHE_LUSTRE_INVALID<br />
return d->d_flags & DCACHE_LUSTRE_INVALID;<br />
#else<br />
return d_unhashed(d);<br />
#endif<br />
}<br />
<br />
...<br />
<br />
if (invalid_dentry(parent)) {<br />
</pre><br />
* If you nest preprocessor commands, use spaces to visually delineate:<br />
<pre><br />
#ifdef __KERNEL__<br />
# include <goose><br />
# define MOOSE steak<br />
#else<br />
# include <mutton><br />
# define MOOSE prancing<br />
#endif<br />
</pre><br />
* For very long #ifdefs, include the conditional with each #endif to make it readable:<br />
<pre><br />
#ifdef __KERNEL__<br />
# if LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,0)<br />
/* lots<br />
of<br />
stuff */<br />
# endif /* KERNEL_VERSION(2,5,0) */<br />
#else /* !__KERNEL__ */<br />
# if HAVE_FEATURE<br />
/* more<br />
* stuff */<br />
# endif<br />
#endif /* __KERNEL__ */<br />
</pre><br />
* Comments should have the leading '''/*''' on the same line as the comment, and the trailing '''*/''' at the end of the last comment line. Intermediate lines should start with a '''*''' aligned with the first line's '''*''':<br />
<pre><br />
/* This is a short comment */<br />
<br />
/* This is a multi-line comment. I wish the line would wrap already,<br />
* as I don't have much to write about. */<br />
</pre><br />
* Conditional expressions read more clearly if boolean expressions are implicit, but non-boolean and pointer expressions compare explicitly with 0 and NULL respectively - e.g...<br />
<pre><br />
right:<br />
<br />
if (inode != NULL && /* valid inode? */<br />
!writing && /* not writing? */<br />
ref_count == 0) /* no more references? */<br />
do_this();<br />
</pre><br />
<pre><br />
wrong:<br />
<br />
if (inode && /* valid inode? */<br />
writing == 0 && /* not writing? */<br />
!ref_count) /* no more references? */<br />
do_this();<br />
</pre><br />
* Parentheses and line breaks help make conditional expressions more readable.<br />
<pre><br />
right:<br />
<br />
if (a->a_field == 3 ||<br />
((b->b_field & BITMASK1) && (c->c_field & BITMASK2)))<br />
do this();<br />
</pre><br />
<pre><br />
wrong:<br />
<br />
if (a->a_field == 3 || b->b_field & BITMASK1 && c->c_field & BITMASK2)<br />
do this()<br />
</pre><br />
* Function declarations absolutely should NOT go into .c files, unless they are forward declarations for static functions that can't otherwise be moved before the caller. Instead, the declaration should go into the most "local" header available (preferably *_internal.h for a given piece of code).<br />
* Structure and constant declarations should not be declared in multiple places. Put the struct into the most "local" header possible. If it is something that is passed over the wire, it needs to go into lustre_idl.h and needs to be correctly swabbed when the RPC message is unpacked.<br />
* The types and printf/printk formats used by Lustre code are:<br />
<pre><br />
__u64 LPU64/LPX64/LPD64 (unsigned, hex, signed)<br />
size_t LPSZ (or cast to int and use %u / %d)<br />
__u32/int %u/%x/%d (unsigned, hex, signed)<br />
(unsigned) long long %llu/%llx/%lld<br />
loff_t %lld after a cast to long long (unfortunately)<br />
</pre><br />
<br />
* For Autoconf macros, follow the [http://www.gnu.org/software/autoconf/manual/html_node/Coding-Style.html style suggested in the autoconf manual].<br />
<br />
<pre><br />
AC_CACHE_CHECK([for EMX OS/2 environment], [ac_cv_emxos2],<br />
[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([], [return __EMX__;])],<br />
[ac_cv_emxos2=yes],<br />
[ac_cv_emxos2=no])])<br />
</pre><br />
:or even<br />
<pre><br />
AC_CACHE_CHECK([for EMX OS/2 environment],<br />
[ac_cv_emxos2],<br />
[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([],<br />
[return __EMX__;])],<br />
[ac_cv_emxos2=yes],<br />
[ac_cv_emxos2=no])])<br />
</pre><br />
<br />
----</div>Eebhttp://wiki.old.lustre.org/index.php?title=Coding_Guidelines&diff=6808Coding Guidelines2009-09-03T02:48:11Z<p>Eeb: Lustre coding guidelines</p>
<hr />
<div>== Beautiful Code ==<br />
<br />
''A note from Eric Barton, our lead engineer:''<br />
<br />
More important than the physical layout of code (which is covered in detail below) is the idea that the code should be ''beautiful'' to read.<br />
<br />
What makes code beautiful to me? Fundamentally, it's readability and obviousness. The code must not have secrets but should flow easily, pleasurably and ''accurately'' off the page and into the mind of the reader.<br />
<br />
How do I think beautiful code is written? Like this...<br />
<br />
* The author must be confident and knowledgeable and proud of her work. She must understand what the code should do, the environment it must work in, all the combinations of inputs, all the valid outputs, all the possible races and all the reachable states. She must [http://en.wikipedia.org/wiki/Grok grok] it.<br />
<br />
* Names must be well chosen. The meaning a human reader attaches to a name can be orthogonal to what the compiler does with it so it's just as easy to mislead as it is to inform. ''[http://en.wikipedia.org/wiki/Does_what_it_says_on_the_tin "Does exactly what it says on the tin"]'' is a popular UK English expression describing something that does ''exactly'' what it tells you it's going to do, no more and no less. For example, if I open a tin labelled "soap", I expect the contents to help me wash and maybe even smell nice. If it's no good at removing dirt I'll be disappointed. If it removes the dirt but burns off a layer of skin with it, I'll be positively upset. The name of a procedure, a variable or a structure member should tell you something informative about the entity without misleading - just "what it says on the tin".<br />
<br />
* Names must be well chosen. Local, temporary variables can almost always remain relatively short and anonymous, while names in global scope must be unique. In general the wider the context you expect to use the name in, the more unique and informative the name should be. Don't be scared of long names if they help to '''make_the_code_clearer''', but '''do_not_let_things_get_out_of_hand''' either - we don't write COBOL. Related names should be obvious, unambiguous and avoid naming conflicts with other unrelated names, e.g. by using a consistent prefix. This applies to all API procedures (if not all procedures period) withing a given subsystem. Similarly unique member names for global structures, using a prefix to identify the parent structure type helps readability.<br />
<br />
* Names must be well chosen. Don't choose names that are easily confused - especially not if the compiler can't even tell the difference when you make a spelling mistake. '''i''' and '''j''' aren't the worst example - '''req_portal''' and '''rep_portal''' are much worse (and taken from our own code!!!).<br />
<br />
* Names must be well chosen. I can't emphasise this issue enough - I hope you get the point.<br />
<br />
* Assertions must be used intelligently. They can be abused - obviously when over-used they hurt performance and can even convey the impression the author didn't know what she was doing and added them to help her ''learn'' the code. But when assertions are chosen carefully and used properly, they combine the roles of ''active comment'' and ''software fuse''. As an ''active comment'' they tell you something about the program which you can trust more than a comment. And as a ''software fuse'', they provide fault isolation between subsystems by letting you know when and where invariant assumptions are violated.<br />
<br />
* Formatting and indentation rules should be followed intelligently. The visual layout of the code on the page should lend itself to being read easily and accurately - it just looks clean and good.<br />
** Separate "ideas" should be separated clearly in the code layout using blank lines that group related statements and separate unrelated statements.<br />
** Procedures should not ramble on. You must be able to take in the meaning of a procedure without scrolling past page after page of code or parsing deeply nested conditionals and loops. The 80-character rule is there for a reason.<br />
** Declarations are easier to refer to while scanning the code if placed in a block locally to, but visually separate from the code that uses them. Readability is further enhanced by limiting declarations to one per line and aligning types and names vertically.<br />
** Parameters in multiline procedure calls should be aligned so that they are visually contained by their brackets.<br />
** Brackets should be used in expressions to make operator precedence clear.<br />
** Conditional boolean ('''if (expr)'''), scalar ('''if (val != 0)''') and pointer ('''if (ptr != NULL)''') expressions should be written consistently.<br />
** Formatting and indentation rules should not be followed slavishly. If you're faced with either breaking the 80-chars-per-line rule or the parameter indentation rule or creating an obscure helper function, then the 80-chars-per-line rule might have to suffer. The overriding consideration is how the code reads.<br />
<br />
I could go on, but I hope you get the idea. Just think about the poor reader when you're writing, and whether your code will convey its meaning naturally, quickly and accurately, without room for misinterpretation. I didn't mention ''clever'' as a feature of beautiful code because it's only one step from ''clever'' to ''tricky'' - consider...<br />
<br />
t = a; a = b; b = t; /* dumb swap */<br />
<br />
a ^= b; b ^= a; a ^= b; /* clever swap */<br />
<br />
You could feel quite pleased that the clever swap avoids the need for a local temporary variable - but is that such a big deal compared with how quickly, easily and accurately the reader will read it? This is a very minor example which can almost be excused because the "cleverness" is confined to a tiny part of the code. But when ''clever'' code gets spread out it becomes much harder to modify without adding defects. You can only work on code without screwing up if you understand the code ''and'' the environment it works in completely. Or to put it more succinctly...<br />
<br />
''Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it.'' - [http://en.wikipedia.org/wiki/Brian_Kernighan Brian W. Kernighan]<br />
<br />
IMHO, beautiful code helps code quality because it improves communication between the code author and the code reader. Since everyone maintaining and developing the code is a code reader as well as a code author, the quality of this communication can lead either to a virtuous circle of improving quality, or a vicious circle of degrading quality. You, dear reader, will determine which.<br />
<br />
== Coding Style ==<br />
<br />
All of our formatting, wrapping, parenthesis, brace placement, etc. rules are originally derived from the [http://www.kernel.org/doc/Documentation/CodingStyle Linux kernel rules], which are basically K&R.<br />
<br />
=== Formatting Guidelines ===<br />
<br />
'''Note:''' ''All Lustre developers and contributors should follow these guidelines strictly to avoid spurious whitespace differences which create problems later for code merges. Please ensure the default formatting rules in whichever editor you use comply with them.''<br />
<br />
* There should be no tabs in any Lustre, LNET or libcfs files. The exceptions are libsysio (maintained by someone else), ldiskfs and kernel patches (also part of a non-Lustre Group project).<br />
<br />
* Blocks should be indented 8 spaces.<br />
<br />
* New files should contain the following along with the license boilerplate. This will cause vim and emacs to use spaces instead of tabs for indenting. If you use a different editor, it also needs to be set to use spaces for indening Lustre code.<br />
<pre style="background:lightgrey;"><br />
/* -*- mode: c; c-basic-offset: 8; indent-tabs-mode: nil; -*-<br />
* vim:expandtab:shiftwidth=8:tabstop=8:<br />
*/<br />
</pre><br />
<br />
* All lines should wrap at 80 characters. If it's getting too hard to wrap there, you probably need to break it up into more functions.<br />
<br />
* Do not have spaces or tabs on blank lines or at the end of lines. Please ensure you remove all instances of these in any patches you submit to bugzilla. You can find them with grep or in vim using the following regexps...<br />
<pre style="background:lightgrey;"><br />
/[ \t]$/<br />
</pre><br />
<br />
:Alternatively, if you use vim you can put this line in your vimrc file, which will highlight whitespace at the end of lines and spaces followed by tabs in indentation (only works for C/C++ files):<br />
<pre style="background:lightgrey;"><br />
let c_space_errors=1<br />
</pre><br />
<br />
:Or you can use this command, which will make tabs and whitespace at the end of lines visible for all files (but a bit more discretely):<br />
<pre style="background:lightgrey;"><br />
set list listchars=tab:>\ ,trail:$<br />
</pre><br />
<br />
:In emacs, you can use (whitespace-mode) or (whitespace-visual-mode) depending on version. You should also consider using (flyspell-prog-mode).<br />
<br />
=== Language Features ===<br />
<br />
* Don't use "inline" unless you're doing something so performance critical that the function call overhead will make a difference -- in other words: almost never. It makes debugging harder.<br />
<br />
* Use ''typedef'' carefully...<br />
** Do not create a new integer typedef without a really good reason.<br />
** Always postfix typedef names with '''_t''' so that they can be identified clearly in the code.<br />
** Never typedef pointers. The '''*''' makes C pointer declarations obvious - hiding it obfuscates the code.<br />
<br />
* Variable should be declared one per line, type and name, even if there are multiple variables of the same type. For maximum readability, the names should be aligned on the same column.<br />
<pre style="background:lightgrey;"><br />
right:<br />
<br />
int len;<br />
int count;<br />
struct inode *inode;<br />
</pre><br />
<pre style="background:lightgrey;"><br />
wrong:<br />
<br />
int len, count;<br />
struct inode *inode;<br />
</pre><br />
<br />
* Even for short conditionals, the operation should be on a separate line:<br />
<pre style="background:lightgrey;"><br />
if (foo)<br />
bar();<br />
</pre><br />
<br />
* When you wrap, the next line should start after the parenthesis:<br />
<pre style="background:lightgrey;"><br />
right:<br />
<br />
variable = do_something_complicated(long_argument, longer_argument,<br />
longest_argument(sub_argument,<br />
foo_argument),<br />
last_argument);<br />
<br />
if (some_long_condition(arg1, arg2, arg3) < some_long_value &&<br />
another_long_condition(very_long_argument_name,<br />
another_long_argument_name) ><br />
second_long_value) {<br />
}<br />
</pre><br />
<pre style="background:lightgrey;"><br />
wrong:<br />
<br />
variable = do_something_complicated(long_argument, longer_argument,<br />
longest_argument(sub_argument, foo_argument),<br />
last_argument);<br />
<br />
if (some_long_condition(arg1, arg2, arg3) < some_long_value &&<br />
another_long_condition(very_long_argument_name,<br />
another_long_argument_name) ><br />
second_long_value) {<br />
}<br />
</pre><br />
* If you're wrapping put the operators at the end of the line, and if there are no parentheses indent 8 more:<br />
<pre style="background:lightgrey;"><br />
off = le32_to_cpu(fsd->fsd_client_start) +<br />
cl_idx * le16_to_cpu(fsd->fsd_client_size);<br />
</pre><br />
* Binary and ternary (but not unary) operators should be separated from their arguments by one space.<br />
<pre style="background:lightgrey;"><br />
a++;<br />
b |= c;<br />
d = f > g ? 0 : 1;<br />
</pre><br />
* Function calls should be nestled against the parentheses, the parentheses should crowd the arguments, and one space after commas:<br />
<pre style="background:lightgrey;"><br />
right: do_foo(bar, baz);<br />
wrong: do_foo ( bar,baz );<br />
</pre><br />
* All ''if'', ''for'', ''while'', etc. expressions should be separated by a space from the parenthesis, one space after the semicolons:<br />
<pre style="background:lightgrey;"><br />
for (a = 0; a < b; a++)<br />
if (a < b || a == c)<br />
while (1)<br />
</pre><br />
* Opening braces should be on the same line as the line that introduces the block, except for function calls. Closing braces get their own line, except for "else".<br />
<pre style="background:lightgrey;"><br />
int foo(void)<br />
{<br />
if (bar) {<br />
this();<br />
that();<br />
} else if (baz) {<br />
;<br />
} else {<br />
;<br />
}<br />
do {<br />
cow();<br />
} while (0);<br />
}<br />
</pre><br />
* If one part of a compound ''if'' block has braces, all should.<br />
<pre style="background:lightgrey;"><br />
right:<br />
<br />
if (foo) {<br />
bar();<br />
baz();<br />
} else {<br />
salmon();<br />
}<br />
</pre><br />
<pre style="background:lightgrey;"><br />
wrong:<br />
<br />
if (foo) {<br />
bar();<br />
baz();<br />
} else<br />
moose();<br />
</pre><br />
* When you make a macro, protect those who might call it by using do/while and parentheses; line up your backslashes:<br />
<pre style="background:lightgrey;"><br />
right:<br />
<br />
#define DO_STUFF(a) \<br />
do { \<br />
int b = (a) + MAGIC; \<br />
do_other_stuff(b); \<br />
} while (0)<br />
</pre><br />
<pre style="background:lightgrey;"><br />
wrong:<br />
<br />
#define DO_STUFF(a) \<br />
{ \<br />
int b = a + MAGIC; \<br />
do_other_stuff(b); \<br />
}<br />
</pre><br />
* If you nest preprocessor commands, use spaces to visually delineate:<br />
<pre style="background:lightgrey;"><br />
#ifdef __KERNEL__<br />
# include <goose><br />
# define MOOSE steak<br />
#else<br />
# include <mutton><br />
# define MOOSE prancing<br />
#endif<br />
</pre><br />
* For very long #ifdefs include the conditional with each #endif to make it readable:<br />
<pre style="background:lightgrey;"><br />
#ifdef __KERNEL__<br />
# if LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,0)<br />
/* lots<br />
of<br />
stuff */<br />
# endif /* KERNEL_VERSION(2,5,0) */<br />
#else /* !__KERNEL__ */<br />
# if HAVE_FEATURE<br />
/* more<br />
* stuff */<br />
# endif<br />
#endif /* __KERNEL__ */<br />
</pre><br />
* Comments should have the leading '''/*''' on the same line as the comment, and the trailing '''*/''' at the end of the last comment line. Intermediate lines should start with a '''*''' aligned with the first line's '''*''':<br />
<pre style="background:lightgrey;"><br />
/* This is a short comment */<br />
<br />
/* This is a multi-line comment. I wish the line would wrap already,<br />
* as I don't have much to write about. */<br />
</pre><br />
* Conditional expressions read more clearly if boolean expressions are implicit, but non-boolean and pointer expressions compare explicitly with 0 and NULL respectively - e.g...<br />
<pre style="background:lightgrey;"><br />
right:<br />
<br />
if (inode != NULL && /* valid inode? */<br />
!writing && /* not writing? */<br />
ref_count == 0) /* no more references? */<br />
do_this();<br />
</pre><br />
<pre style="background:lightgrey;"><br />
wrong:<br />
<br />
if (inode && /* valid inode? */<br />
writing == 0 && /* not writing? */<br />
!ref_count) /* no more references? */<br />
do_this();<br />
</pre><br />
* Parentheses and line breaks help make conditional expressions more readable.<br />
<pre style="background:lightgrey;"><br />
right:<br />
<br />
if (a->a_field == 3 ||<br />
((b->b_field & BITMASK1) && (c->c_field & BITMASK2)))<br />
do this();<br />
</pre><br />
<pre style="background:lightgrey;"><br />
wrong:<br />
<br />
if (a->a_field == 3 || b->b_field & BITMASK1 && c->c_field & BITMASK2)<br />
do this()<br />
</pre><br />
* Function declarations absolutely should NOT go into .c files, unless they are forward declarations for static functions that can't otherwise be moved before the caller. Instead, the declaration should go into the most "local" header available (preferrably *_internal.h for a given piece of code).<br />
* Structure and constant declarations should not be declared in multiple places. Put the struct into the most "local" header possible. If it is something that is passed over the wire it needs to go into lustre_idl.h, and needs to be correctly swabbed when the RPC message is unpacked.<br />
* The types and printf/printk formats used by Lustre code are:<br />
<pre style="background:lightgrey;"><br />
__u64 LPU64/LPX64/LPD64 (unsigned, hex, signed)<br />
size_t LPSZ (or cast to int and use %u / %d)<br />
__u32/int %u/%x/%d (unsigned, hex, signed)<br />
(unsigned) long long %llu/%llx/%lld<br />
loff_t %lld after a cast to long long (unfortunately)<br />
</pre><br />
<br />
* For Autoconf macros, follow the [http://www.gnu.org/software/autoconf/manual/html_node/Coding-Style.html style suggested in the autoconf manual].<br />
<br />
<pre style="background:lightgrey;"><br />
AC_CACHE_CHECK([for EMX OS/2 environment], [ac_cv_emxos2],<br />
[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([], [return __EMX__;])],<br />
[ac_cv_emxos2=yes],<br />
[ac_cv_emxos2=no])])<br />
</pre><br />
:or even<br />
<pre style="background:lightgrey;"><br />
AC_CACHE_CHECK([for EMX OS/2 environment],<br />
[ac_cv_emxos2],<br />
[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([],<br />
[return __EMX__;])],<br />
[ac_cv_emxos2=yes],<br />
[ac_cv_emxos2=no])])<br />
</pre><br />
<br />
----</div>Eebhttp://wiki.old.lustre.org/index.php?title=Coding_Guidelines&diff=6533Coding Guidelines2009-08-18T13:12:08Z<p>Eeb: /* Great detail */</p>
<hr />
<div>''All Lustre developers should follow the guidelines in this page very strictly to avoid problems during code merges later on. Please make the required changes to the default formatting rules in the editor you use to comply to the guidelines below.<br />
<br />
== Beautiful Code ==<br />
<br />
More important than the physical layout of code (which is covered in detail below) is the thought that code is ''beautiful'' to read. What makes code "beautiful" to me? Fundamentally, I'm talking about readability and obviousness - the code must not have secrets.<br />
<br />
* Separate "ideas" are clearly separated in the program layout - e.g. blank lines group related statements and separate unrelated statements.<br />
<br />
* Declarations are easy to read. The declarations should always be separated from the functional code. Declarations should be made as locally as possible to the code they are being used. I prefer one variable per line, with the names aligned vertically - it makes checking types as you read quicker and IMHO it's a pleasure to the eye.<br />
<br />
* If it's a choice between breaking the 80-chars-per-line rule and code that doesn't look good (e.g. breaks indentation rules or doesn't visually contain procedure call parameters between the brackets or spreads over too many lines), then break the 80-chars-per-line rule. We _don't_ want stupidly long lines, but we all have enough screen resolution these days to not have to wrap lines at 80 chars.<br />
<br />
* Names are well chosen. ''"Does exactly what it says on the tin"'' is an English (UK) expression describing something that tells you what it's going to do and then does _exactly_ that. For example, when I open a box labelled "soap", I expect it to help me wash and maybe even smell nice. I'll get upset if it's no good at removing dirt, and really upset if it makes me break out in a rash. The name of a procedure or variable or structure member should tell you something about the entity, without giving you misleading information - just "what it says on the tin".<br />
<br />
* Names are well chosen. This time I mean that you don't choose names that easily become a different but valid (to the compiler) name if you make a spelling mistake. '''i''' and '''j''' aren't the worst example - '''req_portal''' and '''rep_portal''' are much worse (and taken from our own code!!!).<br />
<br />
* Names are well chosen. Don't be scared of long names, but_do_not_go_to_extremes_either - we don't write COBOL.<br />
<br />
* Names are well chosen. I can't emphasise this issue enough - I hope you get the point.<br />
<br />
* Intelligent use of assertions. Assertions can be abused. Obviously, when over-used they hurt performance. And they can also make you think that the code author didn't know what she was doing and added them to help her ''learn'' the code. But when assertions are used properly, they combine the roles of ''active comment'' and ''software fuse''. As an ''active comment'' they tell you something about the program which you can trust more than a comment. As a ''software fuse'', they provide fault isolation between subsystem by letting you know when and where invariant assumptions are violated.<br />
<br />
I could go on, but I hope you get the idea. Notice that I didn't mention ''clever'' as a desirable attribute. It's only one step from<br />
''clever'' to ''tricky'' - consider...<br />
<br />
t = a; a = b; b = t; /* dumb swap */<br />
<br />
a ^= b; b ^= a; a ^= b; /* clever swap */<br />
<br />
...which is a very minor example. It can almost be excused, because the "cleverness" is confined to a tiny part of the code. But if the ''clever'' code had sub-structure, that sub-structure would be very hard to work on. You can't be sure you're not screwing something up until you understand completely the environment in which you're working.<br />
<br />
''Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it.'' - [http://en.wikipedia.org/wiki/Brian_Kernighan Brian W. Kernighan]<br />
<br />
IMHO, beautiful code helps code quality because it improves communication between the code author and the code reader. Since code modifiers are also code readers, the quality of communication can lead either to a virtuous circle of improving quality, or a vicious circle of degrading quality.<br />
<br />
<br />
<br />
== Formatting Guidelines ==<br />
<br />
* There should be no tabs in any Lustre or LNET files. The exceptions are libsysio (maintained by someone else), ldiskfs and kernel patches (also part of a non-Lustre Group project).<br />
<br />
* Blocks should be indented by 8 spaces.<br />
<br />
* Variables should be declared one per line, even if there are multiple variables of the same type.<br />
<br />
* New files should contain the following along with the license boilerplate. This will cause vim and emacs to use spaces instead of tabs for indenting. If you use a different editor, it also needs to be set to use spaces for indening Lustre code.<br />
<pre style="background:lightgrey;"><br />
/* -*- mode: c; c-basic-offset: 8; indent-tabs-mode: nil; -*-<br />
* vim:expandtab:shiftwidth=8:tabstop=8:<br />
*/<br />
</pre><br />
<br />
* All lines should wrap at 80 characters. If it's getting too hard to wrap there, you probably need to break it up into more functions. In some cases, it is acceptable to remove a few spaces between function arguments to avoid overflowing onto the next line.<br />
<br />
* Do not have spaces or tabs on blank lines or at the end of lines. Find these with some regexps in your patch (grep, or in vim) before attaching it to bugzilla:<br />
<pre style="background:lightgrey;"><br />
/[ \t]$/<br />
</pre><br />
<br />
Alternatively, if you use vim you can put this line in your vimrc file, which will highlight whitespace at the end of lines and spaces followed by tabs in indentation (only works for C/C++ files):<br />
<pre style="background:lightgrey;"><br />
let c_space_errors=1<br />
</pre><br />
<br />
Or you can use this command, which will make tabs and whitespace at the end of lines visible for all files (but a bit more discretely):<br />
<pre style="background:lightgrey;"><br />
set list listchars=tab:>\ ,trail:$<br />
</pre><br />
<br />
In emacs, you can use (whitespace-mode) or (whitespace-visual-mode) depending on version. You should also consider using (flyspell-prog-mode).<br />
<br />
* Don't use "inline" unless you're doing something so performance critical that the function call overhead will make a difference -- in other words: almost never. It makes debugging harder.<br />
<br />
All of our wrapping, parenthesis, brace placement, etc. rules are basically Linux kernel rules, which are basically K&R. For those of you in need of a refresher, great detail is provided below.<br />
<br />
* For Autoconf macros, follow the [http://www.gnu.org/software/autoconf/manual/html_node/Coding-Style.html style suggested in the autoconf manual].<br />
<br />
<pre style="background:lightgrey;"><br />
AC_CACHE_CHECK([for EMX OS/2 environment], [ac_cv_emxos2],<br />
[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([], [return __EMX__;])],<br />
[ac_cv_emxos2=yes],<br />
[ac_cv_emxos2=no])])<br />
</pre><br />
or even<br />
<pre style="background:lightgrey;"><br />
AC_CACHE_CHECK([for EMX OS/2 environment],<br />
[ac_cv_emxos2],<br />
[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([],<br />
[return __EMX__;])],<br />
[ac_cv_emxos2=yes],<br />
[ac_cv_emxos2=no])])<br />
</pre><br />
<br />
== Great detail ==<br />
<br />
* Each variable declaration should be on a separate line, with the names aligned to the same column:<br />
<pre style="background:lightgrey;"><br />
int len;<br />
int count;<br />
struct inode *inode;<br />
</pre><br />
<br />
* Even for short conditionals, the operation should be on a separate line:<br />
<pre style="background:lightgrey;"><br />
if (foo)<br />
bar();<br />
</pre><br />
<br />
* When you wrap, the next line should start after the parenthesis:<br />
<pre style="background:lightgrey;"><br />
right:<br />
<br />
variable = do_something_complicated(long_argument, longer_argument,<br />
longest_argument(sub_argument,<br />
foo_argument),<br />
last_argument);<br />
<br />
if (some_long_condition(arg1, arg2, arg3) < some_long_value &&<br />
another_long_condition(very_long_argument_name,<br />
another_long_argument_name) ><br />
second_long_value) {<br />
}<br />
</pre> <br />
<pre style="background:lightgrey;"><br />
wrong:<br />
<br />
variable = do_something_complicated(long_argument, longer_argument,<br />
longest_argument(sub_argument, foo_argument),<br />
last_argument);<br />
<br />
if (some_long_condition(arg1, arg2, arg3) < some_long_value &&<br />
another_long_condition(very_long_argument_name,<br />
another_long_argument_name) ><br />
second_long_value) {<br />
}<br />
</pre><br />
* If you're wrapping put the operators at the end of the line, and if there are no parentheses indent 8 more:<br />
<pre style="background:lightgrey;"><br />
off = le32_to_cpu(fsd->fsd_client_start) +<br />
cl_idx * le16_to_cpu(fsd->fsd_client_size);<br />
</pre><br />
* Binary and ternary (but not unary) operators should be separated from their arguments by one space.<br />
<pre style="background:lightgrey;"><br />
a++;<br />
b |= c;<br />
d = f > g ? 0 : 1;<br />
</pre><br />
* Function calls should be nestled against the parentheses, the parentheses should crowd the arguments, and one space after commas:<br />
<pre style="background:lightgrey;"><br />
right: do_foo(bar, baz);<br />
wrong: do_foo ( bar,baz );<br />
</pre><br />
* All ''if'', ''for'', ''while'', etc. expressions should be separated by a space from the parenthesis, one space after the semicolons:<br />
<pre style="background:lightgrey;"><br />
for (a = 0; a < b; a++)<br />
if (a < b || a == c)<br />
while (1)<br />
</pre><br />
* Opening braces should be on the same line as the line that introduces the block, except for function calls. Closing braces get their own line, except for "else".<br />
<pre style="background:lightgrey;"><br />
int foo(void)<br />
{<br />
if (bar) {<br />
this();<br />
that();<br />
} else if (baz) {<br />
;<br />
} else {<br />
;<br />
}<br />
do {<br />
cow();<br />
} while (0);<br />
}<br />
</pre><br />
* If one part of a compound ''if'' block has braces, all should.<br />
<pre style="background:lightgrey;"><br />
right:<br />
<br />
if (foo) {<br />
bar();<br />
baz();<br />
} else {<br />
salmon();<br />
}<br />
</pre><br />
<pre style="background:lightgrey;"> <br />
wrong:<br />
<br />
if (foo) {<br />
bar();<br />
baz();<br />
} else<br />
moose();<br />
</pre><br />
* When you make a macro, protect those who might call it by using do/while and parentheses; line up your backslashes:<br />
<pre style="background:lightgrey;"><br />
right:<br />
<br />
#define DO_STUFF(a) \<br />
do { \<br />
int b = (a) + MAGIC; \<br />
do_other_stuff(b); \<br />
} while (0)<br />
</pre><br />
<pre style="background:lightgrey;"><br />
wrong:<br />
<br />
#define DO_STUFF(a) \<br />
{ \<br />
int b = a + MAGIC; \<br />
do_other_stuff(b); \<br />
}<br />
</pre><br />
* If you nest preprocessor commands, use spaces to visually delineate:<br />
<pre style="background:lightgrey;"><br />
#ifdef __KERNEL__<br />
# include <goose><br />
# define MOOSE steak<br />
#else<br />
# include <mutton><br />
# define MOOSE prancing<br />
#endif<br />
</pre><br />
* For very long #ifdefs include the conditional with each #endif to make it readable:<br />
<pre style="background:lightgrey;"><br />
#ifdef __KERNEL__<br />
# if LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,0)<br />
/* lots<br />
of<br />
stuff */<br />
# endif /* KERNEL_VERSION(2,5,0) */<br />
#else /* !__KERNEL__ */<br />
# if HAVE_FEATURE<br />
/* more<br />
* stuff */<br />
# endif<br />
#endif /* __KERNEL__ */ <br />
</pre><br />
* Comments should have the leading '''/*''' on the same line as the comment, and the trailing '''*/''' at the end of the last comment line. Intermediate lines should start with a '''*''' aligned with the first line's '''*''':<br />
<pre style="background:lightgrey;"> <br />
/* This is a short comment */<br />
<br />
/* This is a multi-line comment. I wish the line would wrap already,<br />
* as I don't have much to write about. */<br />
</pre><br />
* Conditional expressions read more clearly if boolean expressions are implicit, but non-boolean and pointer expressions compare explicitly with 0 and NULL respectively - e.g...<br />
<pre style="background:lightgrey;"><br />
right:<br />
<br />
if (inode != NULL && /* valid inode? */<br />
!writing && /* not writing? */<br />
ref_count == 0) /* no more references? */<br />
do_this();<br />
</pre><br />
<pre style="background:lightgrey;"><br />
wrong:<br />
<br />
if (inode && /* valid inode? */<br />
writing == 0 && /* not writing? */<br />
!ref_count) /* no more references? */<br />
do_this();<br />
</pre><br />
* Parentheses and line breaks help make conditional expressions more readable.<br />
<pre style="background:lightgrey;"><br />
right:<br />
<br />
if (a->a_field == 3 ||<br />
((b->b_field & BITMASK1) && (c->c_field & BITMASK2)))<br />
do this();<br />
</pre><br />
<pre style="background:lightgrey;"><br />
wrong:<br />
<br />
if (a->a_field == 3 || b->b_field & BITMASK1 && c->c_field & BITMASK2)<br />
do this()<br />
</pre><br />
* Function declarations absolutely should NOT go into .c files, unless they are forward declarations for static functions that can't otherwise be moved before the caller. Instead, the declaration should go into the most "local" header available (preferrably *_internal.h for a given piece of code).<br />
* Structure and constant declarations should not be declared in multiple places. Put the struct into the most "local" header possible. If it is something that is passed over the wire it needs to go into lustre_idl.h, and needs to be correctly swabbed when the RPC message is unpacked.<br />
* The types and printf/printk formats used by Lustre code are:<br />
<pre style="background:lightgrey;"><br />
__u64 LPU64/LPX64/LPD64 (unsigned, hex, signed)<br />
size_t LPSZ (or cast to int and use %u / %d)<br />
__u32/int %u/%x/%d (unsigned, hex, signed)<br />
(unsigned) long long %llu/%llx/%lld<br />
loff_t %lld after a cast to long long (unfortunately)<br />
</pre><br />
----<br />
''</div>Eebhttp://wiki.old.lustre.org/index.php?title=Documenting_Code&diff=5103Documenting Code2008-11-12T14:01:49Z<p>Eeb: </p>
<hr />
<div>== Introduction ==<br />
In addition to the architecture and design documentation, ''interface'' documentation is required for each Lustre subsystem to provide reference information on how to use each subsystem correctly. This documentation is embedded in the source code as stylised comments using [http://en.wikipedia.org/wiki/Doxygen doxygen] to ensure it stays up to date as the source is developed and maintained.<br />
<br />
The minimum requirement is to document the subsystem API, including every datatype, procedure and global designed to be used externally as follows...<br />
<br />
* Procedures<br />
** What does it do<br />
** How to use it / how not to abuse it<br />
** What does it return<br />
** Description of parameters and their valid values<br />
* Globals<br />
** What it is for<br />
** How to use it / how not to abuse it<br />
* Datatypes (structs, typedefs, enums)<br />
** What it is for<br />
** How to use it / how not to abuse it<br />
** Description of each struct member<br />
<br />
"How to use it / how not to abuse it" requires particular care - it should include all usage constraints e.g. concurrency controls, reference counting, permitted caller context etc. In some cases a description of the whole object life-cycle from creation through to destruction is required to ensure safe usage.<br />
<br />
Additional overview documentation for the subsystem is encouraged but is not a requirement.<br />
<br />
== Examples ==<br />
<br />
Doxygen comments start with [http://www.doxygen.org/docblocks.html '''/**'''] (like in [http://en.wikipedia.org/wiki/Javadoc javadoc]).<br />
<br />
===Procedures and Globals===<br />
Document procedures and globals in the .c files, rather than headers.<br />
<br />
<pre style="background:lightgrey;"><br />
/**<br />
* Owns a page by IO.<br />
*<br />
* Waits until \a pg is in cl_page_state::CPS_CACHED state, and then switch it<br />
* into cl_page_state::CPS_OWNED state.<br />
*<br />
* \param io IO context which wants to own the page<br />
* \param pg page to be owned<br />
*<br />
* \pre !cl_page_is_owned(pg, io)<br />
* \post result == 0 iff cl_page_is_owned(pg, io)<br />
*<br />
* \retval 0 success<br />
*<br />
* \retval -ve failure, e.g., page was destroyed (and landed in<br />
* cl_page_state::CPS_FREEING instead of cl_page_state::CPS_CACHED).<br />
*<br />
* \see cl_page_disown()<br />
* \see cl_page_operations::cpo_own()<br />
*/<br />
int cl_page_own(const struct lu_env *env, struct cl_io *io, struct cl_page *pg)<br />
</pre><br />
<br />
Note that:<br />
* It opens with a brief description which runs up to the first '.' (full stop).<br />
* The brief description is followed by the detailed description.<br />
* Descriptions are written in the third person singular - e.g. "<this function> does this and that", "<this datatype> represents such and such a concept".<br />
* To refer to a function argument use the [http://www.doxygen.org/commands.html#cmda '''\a argname'''] syntax.<br />
* To refer to another function use the [http://www.doxygen.org/autolink.html '''funcname()'''] syntax. This will produce a cross-reference.<br />
* To refer to a field or an enum value use the [http://www.doxygen.org/autolink.html '''SCOPE::NAME'''] syntax.<br />
* Describe possible return values with [http://www.doxygen.org/commands.html#cmdretval '''\retval'''].<br />
* Mention all concurrency control restrictions (such as locks that function expects to he held, or holds on exit) here.<br />
* If possible, specify a (weakest) precondition and (strongest) postcondition for the function. If conditions cannot be expressed as a C language expression, provide informal description.<br />
* enumerate related functions and datatypes in the [http://www.doxygen.org/commands.html#cmdsee '''\see'''] section. Note, that doxygen will automatically cross-reference all places where a given function is called (but not through function pointer), and all functions that it calls so there is no need to enumerate all this.<br />
<br />
===Datatype===<br />
Document datatypes where they are declared.<br />
<br />
<pre style="background:lightgrey;"><br />
/**<br />
* "Compound" object, consisting of multiple layers.<br />
*<br />
* Compound object with given fid is unique with given lu_site.<br />
*<br />
* Note, that object does *not* necessary correspond to the real object in the<br />
* persistent storage: object is an anchor for locking and method calling, so<br />
* it is created for things like not-yet-existing child created by mkdir or<br />
* create calls. lu_object_operations::loo_exists() can be used to check<br />
* whether object is backed by persistent storage entity.<br />
*/<br />
struct lu_object_header {<br />
/**<br />
* Object flags from enum lu_object_header_flags. Set and checked<br />
* atomically.<br />
*/<br />
unsigned long loh_flags;<br />
/**<br />
* Object reference count. Protected by lu_site::ls_guard.<br />
*/<br />
atomic_t loh_ref;<br />
/**<br />
* Fid, uniquely identifying this object.<br />
*/<br />
struct lu_fid loh_fid;<br />
/**<br />
* Common object attributes, cached for efficiency. From enum<br />
* lu_object_header_attr.<br />
*/<br />
__u32 loh_attr;<br />
/**<br />
* Linkage into per-site hash table. Protected by lu_site::ls_guard.<br />
*/<br />
struct hlist_node loh_hash;<br />
/**<br />
* Linkage into per-site LRU list. Protected by lu_site::ls_guard.<br />
*/<br />
struct list_head loh_lru;<br />
/**<br />
* Linkage into list of layers. Never modified once set (except lately<br />
* during object destruction). No locking is necessary.<br />
*/<br />
struct list_head loh_layers;<br />
};<br />
</pre><br />
<br />
Describe datatype invariants (preferably formally).<br />
<br />
<pre style="background:lightgrey;"><br />
/**<br />
* Fields are protected by the lock on cfs_page_t, except for atomics and<br />
* immutables.<br />
*<br />
* \invariant Datatype invariants are in cl_page_invariant(). Basically:<br />
* cl_page::cp_parent and cl_page::cp_child are a well-formed double-linked<br />
* list, consistent with the parent/child pointers in the cl_page::cp_obj and<br />
* cl_page::cp_owner (when set).<br />
*/<br />
struct cl_page {<br />
/** Reference counter. */<br />
atomic_t cp_ref;<br />
</pre><br />
<br />
Describe concurrency control mechanisms for structure fields.<br />
<br />
<pre style="background:lightgrey;"><br />
/** An object this page is a part of. Immutable after creation. */<br />
struct cl_object *cp_obj;<br />
/** Logical page index within the object. Immutable after creation. */<br />
pgoff_t cp_index;<br />
/** List of slices. Immutable after creation. */<br />
struct list_head cp_layers;<br />
...<br />
};<br />
</pre><br />
<br />
Specify when fields are valid.<br />
<br />
<pre style="background:lightgrey;"><br />
/**<br />
* Owning IO in cl_page_state::CPS_OWNED state. Sub-page can be owned<br />
* by sub-io.<br />
*/<br />
struct cl_io *cp_owner;<br />
/**<br />
* Owning IO request in cl_page_state::CPS_PAGEOUT and<br />
* cl_page_state::CPS_PAGEIN states. This field is maintained only in<br />
* the top-level pages.<br />
*/<br />
struct cl_req *cp_req;<br />
</pre><br />
<br />
You can use [http://www.doxygen.org/grouping.html#memgroup '''@{'''...'''@}'''] syntax to define a subset of fields or enum values which should be grouped together.<br />
<br />
<pre style="background:lightgrey;"><br />
struct cl_object_header {<br />
/** Standard lu_object_header. cl_object::co_lu::lo_header points<br />
* here. */<br />
struct lu_object_header coh_lu;<br />
/** \name locks<br />
* \todo XXX move locks below to the separate cache-lines, they are<br />
* mostly useless otherwise.<br />
*/<br />
/** @{ */<br />
/** Lock protecting page tree. */<br />
spinlock_t coh_page_guard;<br />
/** Lock protecting lock list. */<br />
spinlock_t coh_lock_guard;<br />
/** @} locks */<br />
</pre><br />
<br />
By default a documenting comment goes immediately before the entity being commented. If it's necessary to place this comment separately (e.g. to streamline comments in the header file) use following syntax.<br />
<br />
<pre style="background:lightgrey;"><br />
/** \struct cl_page<br />
* Layered client page.<br />
*<br />
* cl_page: represents a portion of a file, cached in the memory. All pages<br />
* of the given file are of the same size, and are kept in the radix tree<br />
</pre><br />
<br />
===Subsystem Overview===<br />
<br />
To document a subsystem, add the following comment to the header file which contains the definitions of its key datatypes.<br />
<br />
<pre style="background:lightgrey;"><br />
/** \defgroup component_name component_name<br />
*<br />
* overall module documentation<br />
* ...<br />
*<br />
* @{ <br />
*/<br />
datatype definitions...<br />
exported functions...<br />
/** @} component_name */<br />
</pre><br />
<br />
To separate a logical part of a larger component, add the following somewhere within the component's \defgroup:<br />
<br />
<pre style="background:lightgrey;"><br />
/**<br />
* \name subcomponent_name subcomponent_name<br />
*<br />
* Description of a sub-component<br />
*/<br />
/** @{ */<br />
datatype definitions...<br />
exported functions...<br />
/** @} subcomponent_name */<br />
</pre><br />
<br />
If an exported function prototype in a header is located within some group, the appropriate function definition in a .c file is automatically assigned to the same group.<br />
<br />
A set of comments which is not lexically a part of a group, can be included into it with \addtogroup command:<br />
<br />
<pre style="background:lightgrey;"><br />
/** \addtogroup cl_object cl_object<br />
* @{ */<br />
/**<br />
* "Data attributes" of cl_object. Data attributes can be updated<br />
* independently for a sub-object, and top-object's attributes are calculated<br />
* from sub-objects' ones.<br />
*/<br />
struct cl_attr {<br />
/** Object size, in bytes */<br />
loff_t cat_size;<br />
...<br />
};<br />
...<br />
/** @} cl_object */<br />
</pre><br />
<br />
== Running Doxygen ==<br />
Doxygen uses a ''template file'' to control documentation build. Lustre comes with two templates:<br />
* build/doxyfile.ref: produces a ''short'' form of documentation set, suitable as a reference. Output is placed into apidoc.ref/ directory.<br />
* build/doxyfile.api: produces full documentation set, more suitable for learning code structure. In addition to apidoc.ref/ version this includes call-graphs, source code excerpts, and non-html forms of documentation (rtf, latex, troff, and rtf). Output is placed into apidoc.api/ directory.<br />
<br />
To build documentation, run<br />
<pre style="background:lightgrey;"><br />
doxygen build/$TEMPLATE<br />
</pre><br />
in the top-level lustre directory.<br />
<br />
== Publishing ==<br />
<br />
build/apidoc.publish scripts publishes your local version of documentation on the http://wiki.lustre.org/apidoc:<br />
<br />
<pre><br />
build/apidoc.publish [-b branchname] [-l additional-label] [-d] [-u user]<br />
</pre><br />
<br />
build/apidoc.publish tries to guess branchname by looking into CVS/Tag. -d instructs script to use current date as a label. Documentation will be uploaded into<br />
<br />
<pre><br />
user@shell.lustre.sun.com:/home/www/apidoc/$branch$label<br />
</pre><br />
where $label is a concatenation of all labels given on the command line in order.<br />
<br />
== Doxygen References ==<br />
<br />
[http://www.doxygen.org/ Doxygen Home]<br />
<br />
[http://www.doxygen.org/manual.html Manual]<br />
<br />
[http://www.doxygen.org/commands.html Special Comment Tags]</div>Eebhttp://wiki.old.lustre.org/index.php?title=Documenting_Code&diff=5102Documenting Code2008-11-12T13:51:01Z<p>Eeb: </p>
<hr />
<div>== Introduction ==<br />
In addition to the architecture and design documentation, ''interface'' documentation is required for each Lustre subsystem to provide reference information on how to use each subsystem correctly. This documentation is embedded in the source code as stylised comments using [http://en.wikipedia.org/wiki/Doxygen doxygen] to ensure it stays up to date as the source is developed and maintained.<br />
<br />
The minimum requirement is to document the subsystem API, including every datatype, procedure and global designed to be used externally.<br />
<br />
* Procedures<br />
** What does it do<br />
** How to use it / how not to abuse it<br />
** What does it return<br />
** Description of parameters and their valid values<br />
* Globals<br />
** What it is for<br />
** How to use it / how not to abuse it<br />
* Datatypes (structs, typedefs, enums)<br />
** What it is for<br />
** How to use it / how not to abuse it<br />
** Description of each struct member<br />
<br />
Additional overview documentation for the subsystem is encouraged but is not a requirement.<br />
<br />
== Examples ==<br />
<br />
Doxygen comments start with [http://www.doxygen.org/docblocks.html '''/**'''] (like in [http://en.wikipedia.org/wiki/Javadoc javadoc]).<br />
<br />
===Procedures and Globals===<br />
Document procedures and globals in the .c files, rather than headers.<br />
<br />
<pre style="background:lightgrey;"><br />
/**<br />
* Owns a page by IO.<br />
*<br />
* Waits until \a pg is in cl_page_state::CPS_CACHED state, and then switch it<br />
* into cl_page_state::CPS_OWNED state.<br />
*<br />
* \param io IO context which wants to own the page<br />
* \param pg page to be owned<br />
*<br />
* \pre !cl_page_is_owned(pg, io)<br />
* \post result == 0 iff cl_page_is_owned(pg, io)<br />
*<br />
* \retval 0 success<br />
*<br />
* \retval -ve failure, e.g., page was destroyed (and landed in<br />
* cl_page_state::CPS_FREEING instead of cl_page_state::CPS_CACHED).<br />
*<br />
* \see cl_page_disown()<br />
* \see cl_page_operations::cpo_own()<br />
*/<br />
int cl_page_own(const struct lu_env *env, struct cl_io *io, struct cl_page *pg)<br />
</pre><br />
<br />
Note that:<br />
* It opens with a brief description which runs up to the first '.' (full stop).<br />
* The brief description is followed by the detailed description.<br />
* Descriptions are written in the third person singular - e.g. "<this function> does this and that", "<this datatype> represents such and such a concept".<br />
* To refer to a function argument use the [http://www.doxygen.org/commands.html#cmda '''\a argname'''] syntax.<br />
* To refer to another function use the [http://www.doxygen.org/autolink.html '''funcname()'''] syntax. This will produce a cross-reference.<br />
* To refer to a field or an enum value use the [http://www.doxygen.org/autolink.html '''SCOPE::NAME'''] syntax.<br />
* Describe possible return values with [http://www.doxygen.org/commands.html#cmdretval '''\retval'''].<br />
* Mention all concurrency control restrictions (such as locks that function expects to he held, or holds on exit) here.<br />
* If possible, specify a (weakest) precondition and (strongest) postcondition for the function. If conditions cannot be expressed as a C language expression, provide informal description.<br />
* enumerate related functions and datatypes in the [http://www.doxygen.org/commands.html#cmdsee '''\see'''] section. Note, that doxygen will automatically cross-reference all places where a given function is called (but not through function pointer), and all functions that it calls so there is no need to enumerate all this.<br />
<br />
===Datatype===<br />
Document datatypes where they are declared.<br />
<br />
<pre style="background:lightgrey;"><br />
/**<br />
* "Compound" object, consisting of multiple layers.<br />
*<br />
* Compound object with given fid is unique with given lu_site.<br />
*<br />
* Note, that object does *not* necessary correspond to the real object in the<br />
* persistent storage: object is an anchor for locking and method calling, so<br />
* it is created for things like not-yet-existing child created by mkdir or<br />
* create calls. lu_object_operations::loo_exists() can be used to check<br />
* whether object is backed by persistent storage entity.<br />
*/<br />
struct lu_object_header {<br />
/**<br />
* Object flags from enum lu_object_header_flags. Set and checked<br />
* atomically.<br />
*/<br />
unsigned long loh_flags;<br />
/**<br />
* Object reference count. Protected by lu_site::ls_guard.<br />
*/<br />
atomic_t loh_ref;<br />
/**<br />
* Fid, uniquely identifying this object.<br />
*/<br />
struct lu_fid loh_fid;<br />
/**<br />
* Common object attributes, cached for efficiency. From enum<br />
* lu_object_header_attr.<br />
*/<br />
__u32 loh_attr;<br />
/**<br />
* Linkage into per-site hash table. Protected by lu_site::ls_guard.<br />
*/<br />
struct hlist_node loh_hash;<br />
/**<br />
* Linkage into per-site LRU list. Protected by lu_site::ls_guard.<br />
*/<br />
struct list_head loh_lru;<br />
/**<br />
* Linkage into list of layers. Never modified once set (except lately<br />
* during object destruction). No locking is necessary.<br />
*/<br />
struct list_head loh_layers;<br />
};<br />
</pre><br />
<br />
Describe datatype invariants (preferably formally).<br />
<br />
<pre style="background:lightgrey;"><br />
/**<br />
* Fields are protected by the lock on cfs_page_t, except for atomics and<br />
* immutables.<br />
*<br />
* \invariant Datatype invariants are in cl_page_invariant(). Basically:<br />
* cl_page::cp_parent and cl_page::cp_child are a well-formed double-linked<br />
* list, consistent with the parent/child pointers in the cl_page::cp_obj and<br />
* cl_page::cp_owner (when set).<br />
*/<br />
struct cl_page {<br />
/** Reference counter. */<br />
atomic_t cp_ref;<br />
</pre><br />
<br />
Describe concurrency control mechanisms for structure fields.<br />
<br />
<pre style="background:lightgrey;"><br />
/** An object this page is a part of. Immutable after creation. */<br />
struct cl_object *cp_obj;<br />
/** Logical page index within the object. Immutable after creation. */<br />
pgoff_t cp_index;<br />
/** List of slices. Immutable after creation. */<br />
struct list_head cp_layers;<br />
...<br />
};<br />
</pre><br />
<br />
Specify when fields are valid.<br />
<br />
<pre style="background:lightgrey;"><br />
/**<br />
* Owning IO in cl_page_state::CPS_OWNED state. Sub-page can be owned<br />
* by sub-io.<br />
*/<br />
struct cl_io *cp_owner;<br />
/**<br />
* Owning IO request in cl_page_state::CPS_PAGEOUT and<br />
* cl_page_state::CPS_PAGEIN states. This field is maintained only in<br />
* the top-level pages.<br />
*/<br />
struct cl_req *cp_req;<br />
</pre><br />
<br />
You can use [http://www.doxygen.org/grouping.html#memgroup '''@{'''...'''@}'''] syntax to define a subset of fields or enum values which should be grouped together.<br />
<br />
<pre style="background:lightgrey;"><br />
struct cl_object_header {<br />
/** Standard lu_object_header. cl_object::co_lu::lo_header points<br />
* here. */<br />
struct lu_object_header coh_lu;<br />
/** \name locks<br />
* \todo XXX move locks below to the separate cache-lines, they are<br />
* mostly useless otherwise.<br />
*/<br />
/** @{ */<br />
/** Lock protecting page tree. */<br />
spinlock_t coh_page_guard;<br />
/** Lock protecting lock list. */<br />
spinlock_t coh_lock_guard;<br />
/** @} locks */<br />
</pre><br />
<br />
By default a documenting comment goes immediately before the entity being commented. If it's necessary to place this comment separately (e.g. to streamline comments in the header file) use following syntax.<br />
<br />
<pre style="background:lightgrey;"><br />
/** \struct cl_page<br />
* Layered client page.<br />
*<br />
* cl_page: represents a portion of a file, cached in the memory. All pages<br />
* of the given file are of the same size, and are kept in the radix tree<br />
</pre><br />
<br />
===Subsystem Overview===<br />
<br />
To document a subsystem, add the following comment to the header file which contains the definitions of its key datatypes.<br />
<br />
<pre style="background:lightgrey;"><br />
/** \defgroup component_name component_name<br />
*<br />
* overall module documentation<br />
* ...<br />
*<br />
* @{ <br />
*/<br />
datatype definitions...<br />
exported functions...<br />
/** @} component_name */<br />
</pre><br />
<br />
To separate a logical part of a larger component, add the following somewhere within the component's \defgroup:<br />
<br />
<pre style="background:lightgrey;"><br />
/**<br />
* \name subcomponent_name subcomponent_name<br />
*<br />
* Description of a sub-component<br />
*/<br />
/** @{ */<br />
datatype definitions...<br />
exported functions...<br />
/** @} subcomponent_name */<br />
</pre><br />
<br />
If an exported function prototype in a header is located within some group, the appropriate function definition in a .c file is automatically assigned to the same group.<br />
<br />
A set of comments which is not lexically a part of a group, can be included into it with \addtogroup command:<br />
<br />
<pre style="background:lightgrey;"><br />
/** \addtogroup cl_object cl_object<br />
* @{ */<br />
/**<br />
* "Data attributes" of cl_object. Data attributes can be updated<br />
* independently for a sub-object, and top-object's attributes are calculated<br />
* from sub-objects' ones.<br />
*/<br />
struct cl_attr {<br />
/** Object size, in bytes */<br />
loff_t cat_size;<br />
...<br />
};<br />
...<br />
/** @} cl_object */<br />
</pre><br />
<br />
== Running Doxygen ==<br />
Doxygen uses a ''template file'' to control documentation build. Lustre comes with two templates:<br />
* build/doxyfile.ref: produces a ''short'' form of documentation set, suitable as a reference. Output is placed into apidoc.ref/ directory.<br />
* build/doxyfile.api: produces full documentation set, more suitable for learning code structure. In addition to apidoc.ref/ version this includes call-graphs, source code excerpts, and non-html forms of documentation (rtf, latex, troff, and rtf). Output is placed into apidoc.api/ directory.<br />
<br />
To build documentation, run<br />
<pre style="background:lightgrey;"><br />
doxygen build/$TEMPLATE<br />
</pre><br />
in the top-level lustre directory.<br />
<br />
== Publishing ==<br />
<br />
build/apidoc.publish scripts publishes your local version of documentation on the http://wiki.lustre.org/apidoc:<br />
<br />
<pre><br />
build/apidoc.publish [-b branchname] [-l additional-label] [-d] [-u user]<br />
</pre><br />
<br />
build/apidoc.publish tries to guess branchname by looking into CVS/Tag. -d instructs script to use current date as a label. Documentation will be uploaded into<br />
<br />
<pre><br />
user@shell.lustre.sun.com:/home/www/apidoc/$branch$label<br />
</pre><br />
where $label is a concatenation of all labels given on the command line in order.<br />
<br />
== Doxygen References ==<br />
<br />
[http://www.doxygen.org/ Doxygen Home]<br />
<br />
[http://www.doxygen.org/manual.html Manual]<br />
<br />
[http://www.doxygen.org/commands.html Special Comment Tags]</div>Eebhttp://wiki.old.lustre.org/index.php?title=Coding_Guidelines&diff=4275Coding Guidelines2008-02-23T02:03:52Z<p>Eeb: </p>
<hr />
<div>''All Lustre developers should follow the guidelines in this page very strictly to avoid problems during code merges later on. Please make the required changes to the default formatting rules in the editor you use to comply to the guidelines below.<br />
<br />
== Beautiful Code ==<br />
<br />
More important than the physical layout of code (which is covered in detail below) is the thought that code is ''beautiful'' to read. What makes code "beautiful" to me? Fundamentally, I'm talking about readability and obviousness - the code must not have secrets.<br />
<br />
* Separate "ideas" are clearly separated in the program layout - e.g. blank lines group related statements and separate unrelated statements.<br />
<br />
* Declarations are easy to read. The declarations should always be separated from the functional code. Declarations should be made as locally as possible to the code they are being used. I prefer one variable per line, with the names aligned vertically - it makes checking types as you read quicker and IMHO it's a pleasure to the eye.<br />
<br />
* If it's a choice between breaking the 80-chars-per-line rule and code that doesn't look good (e.g. breaks indentation rules or doesn't visually contain procedure call parameters between the brackets or spreads over too many lines), then break the 80-chars-per-line rule. We _don't_ want stupidly long lines, but we all have enough screen resolution these days to not have to wrap lines at 80 chars.<br />
<br />
* Names are well chosen. ''"Does exactly what it says on the tin"'' is an English (UK) expression describing something that tells you what it's going to do and then does _exactly_ that. For example, when I open a box labelled "soap", I expect it to help me wash and maybe even smell nice. I'll get upset if it's no good at removing dirt, and really upset if it makes me break out in a rash. The name of a procedure or variable or structure member should tell you something about the entity, without giving you misleading information - just "what it says on the tin".<br />
<br />
* Names are well chosen. This time I mean that you don't choose names that easily become a different but valid (to the compiler) name if you make a spelling mistake. '''i''' and '''j''' aren't the worst example - '''req_portal''' and '''rep_portal''' are much worse (and taken from our own code!!!).<br />
<br />
* Names are well chosen. Don't be scared of long names, but_do_not_go_to_extremes_either - we don't write COBOL.<br />
<br />
* Names are well chosen. I can't emphasise this issue enough - I hope you get the point.<br />
<br />
* Intelligent use of assertions. Assertions can be abused. Obviously, when over-used they hurt performance. And they can also make you think that the code author didn't know what she was doing and added them to help her ''learn'' the code. But when assertions are used properly, they combine the roles of ''active comment'' and ''software fuse''. As an ''active comment'' they tell you something about the program which you can trust more than a comment. As a ''software fuse'', they provide fault isolation between subsystem by letting you know when and where invariant assumptions are violated.<br />
<br />
I could go on, but I hope you get the idea. Notice that I didn't mention ''clever'' as a desirable attribute. It's only one step from<br />
''clever'' to ''tricky'' - consider...<br />
<br />
t = a; a = b; b = t; /* dumb swap */<br />
<br />
a ^= b; b ^= a; a ^= b; /* clever swap */<br />
<br />
...which is a very minor example. It can almost be excused, because the "cleverness" is confined to a tiny part of the code. But if the ''clever'' code had sub-structure, that sub-structure would be very hard to work on. You can't be sure you're not screwing something up until you understand completely the environment in which you're working.<br />
<br />
''Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it.'' - [http://en.wikipedia.org/wiki/Brian_Kernighan Brian W. Kernighan]<br />
<br />
IMHO, beautiful code helps code quality because it improves communication between the code author and the code reader. Since code modifiers are also code readers, the quality of communication can lead either to a virtuous circle of improving quality, or a vicious circle of degrading quality.<br />
<br />
<br />
<br />
== Formatting Guidelines ==<br />
<br />
1. There should be no tabs in any Lustre or LNET files. The exceptions are libsysio (maintained by someone else), ldiskfs and kernel patches (also part of a non-CFS project).<br />
<br />
2. Blocks should be indented by 8 spaces.<br />
<br />
3. New files should contain the following along with the license boilerplate. This will cause vim and emacs to use spaces instead of tabs for indenting. If you use a different editor, it also needs to be set to use spaces for indening Lustre code.<br />
<pre style="background:lightgrey;"><br />
/* -*- mode: c; c-basic-offset: 8; indent-tabs-mode: nil; -*-<br />
* vim:expandtab:shiftwidth=8:tabstop=8:<br />
*/<br />
</pre><br />
4. All lines should wrap at 80 characters. If it's getting too hard to wrap there, you probably need to break it up into more functions. In some cases, it is acceptable to remove a few spaces between function arguments to avoid overflowing onto the next line.<br />
<br />
5. Don't have spaces or tabs on blank lines or at the end of lines. Find these with some regexps in your patch (grep, or in vim) before attaching it to bugzilla:<br />
<br />
/[ \t]$/<br />
<br />
6. Don't use "inline" unless you're doing something so performance critical that the function call overhead will make a difference -- in other words: never. It makes debugging harder.<br />
<br />
All of our wrapping, parenthesis, brace placement, etc. rules are basically Linux kernel rules, which are basically K&R. For those of you in need of a refresher, great detail is provided below.<br />
<br />
7. For Autoconf macros, follow the [http://www.gnu.org/software/autoconf/manual/html_node/Coding-Style.html style suggested in the autoconf manual].<br />
<br />
<pre><br />
AC_CACHE_CHECK([for EMX OS/2 environment], [ac_cv_emxos2],<br />
[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([], [return __EMX__;])],<br />
[ac_cv_emxos2=yes],<br />
[ac_cv_emxos2=no])])<br />
</pre><br />
or even<br />
<pre><br />
AC_CACHE_CHECK([for EMX OS/2 environment],<br />
[ac_cv_emxos2],<br />
[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([],<br />
[return __EMX__;])],<br />
[ac_cv_emxos2=yes],<br />
[ac_cv_emxos2=no])])<br />
</pre><br />
<br />
== Great detail ==<br />
<br />
a. When you wrap, the next line should start after the parenthesis:<br />
<pre style="background:lightgrey;"><br />
right:<br />
<br />
variable = do_something_complicated(long_argument, longer_argument,<br />
longest_argument(sub_argument,<br />
foo_argument),<br />
last_argument);<br />
<br />
if (some_long_condition(arg1, arg2, arg3) < some_long_value &&<br />
another_long_condition(very_long_argument_name,<br />
another_long_argument_name) ><br />
second_long_value) {<br />
}<br />
<br />
<br />
wrong:<br />
<br />
variable = do_something_complicated(long_argument, longer_argument,<br />
longest_argument(sub_argument, foo_argument),<br />
last_argument);<br />
<br />
if (some_long_condition(arg1, arg2, arg3) < some_long_value &&<br />
another_long_condition(very_long_argument_name,<br />
another_long_argument_name) ><br />
second_long_value) {<br />
}<br />
</pre><br />
b. If you're wrapping put the operators at the end of the line, and if there are no parentheses indent 8 more:<br />
<pre style="background:lightgrey;"><br />
off = le32_to_cpu(fsd->fsd_client_start) +<br />
cl_idx * le16_to_cpu(fsd->fsd_client_size);<br />
</pre><br />
c. Binary and ternary (but not unary) operators should be separated from their arguments by one space.<br />
<pre style="background:lightgrey;"><br />
a++;<br />
b |= c;<br />
d = f > g ? 0 : 1;<br />
</pre><br />
d. Function calls should be nestled against the parentheses, the parentheses should crowd the arguments, and one space after commas:<br />
<pre style="background:lightgrey;"><br />
right: do_foo(bar, baz);<br />
wrong: do_foo ( bar,baz );<br />
</pre><br />
<br />
e. All ''if'', ''for'', ''while'', etc. expressions should be separated by a space from the parenthesis, one space after the semicolons:<br />
<pre style="background:lightgrey;"><br />
for (a = 0; a < b; a++)<br />
if (a < b || a == c)<br />
while (1)<br />
</pre><br />
f. Opening braces should be on the same line as the line that introduces the block, except for function calls. Closing braces get their own line, except for "else".<br />
<pre style="background:lightgrey;"><br />
int foo(void)<br />
{<br />
if (bar) {<br />
this();<br />
that();<br />
} else if (baz) {<br />
;<br />
} else {<br />
;<br />
}<br />
do {<br />
cow();<br />
} while (0);<br />
}<br />
</pre><br />
g. If one part of a compound ''if'' block has braces, all should.<br />
<pre style="background:lightgrey;"><br />
right:<br />
<br />
if (foo) {<br />
bar();<br />
baz();<br />
} else {<br />
salmon();<br />
}<br />
<br />
wrong:<br />
<br />
if (foo) {<br />
bar();<br />
baz();<br />
} else<br />
moose();<br />
</pre><br />
h. When you make a macro, protect those who might call it by using do/while and parentheses; line up your backslashes:<br />
<pre style="background:lightgrey;"><br />
right:<br />
<br />
#define DO_STUFF(a) \<br />
do { \<br />
int b = (a) + MAGIC; \<br />
do_other_stuff(b); \<br />
} while (0)<br />
<br />
wrong:<br />
<br />
#define DO_STUFF(a) \<br />
{ \<br />
int b = a + MAGIC; \<br />
do_other_stuff(b); \<br />
}<br />
</pre><br />
i. If you nest preprocessor commands, use spaces to visually delineate:<br />
<pre style="background:lightgrey;"><br />
#ifdef __KERNEL__<br />
# include <goose><br />
# define MOOSE steak<br />
#else<br />
# include <mutton><br />
# define MOOSE prancing<br />
#endif<br />
</pre><br />
j. For very long #ifdefs include the conditional with each #endif to make it readable:<br />
<pre style="background:lightgrey;"><br />
#ifdef __KERNEL__<br />
# if LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,0)<br />
/* lots<br />
of<br />
stuff */<br />
# endif /* KERNEL_VERSION(2,5,0) */<br />
#else /* !__KERNEL__ */<br />
# if HAVE_FEATURE<br />
/* more<br />
* stuff */<br />
# endif<br />
#endif /* __KERNEL__ */ <br />
</pre><br />
k. Comments should have the leading '''/*''' on the same line as the comment, and the trailing '''*/''' at the end of the last comment line. Intermediate lines should start with a '''*''' aligned with the first line's '''*''':<br />
<pre style="background:lightgrey;"> <br />
/* This is a short comment */<br />
<br />
/* This is a multi-line comment. I wish the line would wrap already,<br />
* as I don't have much to write about. */<br />
</pre><br />
l. Function declarations absolutely should NOT go into .c files, unless they are forward declarations for static functions that can't otherwise be moved before the caller. Instead, the declaration should go into the most "local" header available (preferrably *_internal.h for a given piece of code).<br />
<br />
m. Structure and constant declarations should not be declared in multiple places. Put the struct into the most "local" header possible. If it is something that is passed over the wire it needs to go into lustre_idl.h, and needs to be correctly swabbed when the RPC message is unpacked.<br />
<br />
n. The types and printf/printk formats used by Lustre code are:<br />
<pre style="background:lightgrey;"><br />
__u64 LPU64/LPX64/LPD64 (unsigned, hex, signed)<br />
size_t LPSZ (or cast to int and use %u / %d)<br />
__u32/int %u/%x/%d (unsigned, hex, signed)<br />
(unsigned) long long %llu/%llx/%lld<br />
loff_t %lld after a cast to long long (unfortunately)<br />
</pre><br />
----<br />
*'''[http://wiki.lustre.org/index.php?title=Front_Page FrontPage]'''<br />
''</div>Eebhttp://wiki.old.lustre.org/index.php?title=Coding_Guidelines&diff=4274Coding Guidelines2008-02-23T02:00:36Z<p>Eeb: </p>
<hr />
<div>''All Lustre developers should follow the guidelines in this page very strictly to avoid problems during code merges later on. Please make the required changes to the default formatting rules in the editor you use to comply to the guidelines below.<br />
<br />
== Beautiful Code ==<br />
<br />
More important than the physical layout of code (which is covered in detail below) is the thought that code is ''beautiful'' to read. What makes code "beautiful" to me? Fundamentally, I'm talking about readability and obviousness - the code must not have secrets.<br />
<br />
* Separate "ideas" are clearly separated in the program layout - e.g. blank lines group related statements and separate unrelated statements.<br />
<br />
* Declarations are easy to read. The declarations should always be separated from the functional code. Declarations should be made as locally as possible to the code they are being used.<br />
<br />
* If it's a choice between breaking the 80-chars-per-line rule and code that doesn't look good (e.g. breaks indentation rules or doesn't visually contain procedure call parameters between the brackets or spreads over too many lines), then break the 80-chars-per-line rule. We _don't_ want stupidly long lines, but we all have enough screen resolution these days to not have to wrap lines at 80 chars.<br />
<br />
* Names are well chosen. ''"Does exactly what it says on the tin"'' is an English (UK) expression describing something that tells you what it's going to do and then does _exactly_ that. For example, when I open a box labelled "soap", I expect it to help me wash and maybe even smell nice. I'll get upset if it's no good at removing dirt, and really upset if it makes me break out in a rash. The name of a procedure or variable or structure member should tell you something about the entity, without giving you misleading information - just "what it says on the tin".<br />
<br />
* Names are well chosen. This time I mean that you don't choose names that easily become a different but valid (to the compiler) name if you make a spelling mistake. '''i''' and '''j''' aren't the worst example - '''req_portal''' and '''rep_portal''' are much worse (and taken from our own code!!!).<br />
<br />
* Names are well chosen. Don't be scared of long names, but_do_not_go_to_extremes_either - we don't write COBOL.<br />
<br />
* Names are well chosen. I can't emphasise this issue enough - I hope you get the point.<br />
<br />
* Intelligent use of assertions. Assertions can be abused. Obviously, when over-used they hurt performance. And they can also make you think that the code author didn't know what she was doing and added them to help her ''learn'' the code. But when assertions are used properly, they combine the roles of ''active comment'' and ''software fuse''. As an ''active comment'' they tell you something about the program which you can trust more than a comment. As a ''software fuse'', they provide fault isolation between subsystem by letting you know when and where invariant assumptions are violated.<br />
<br />
I could go on, but I hope you get the idea. Notice that I didn't mention ''clever'' as a desirable attribute. It's only one step from<br />
''clever'' to ''tricky'' - consider...<br />
<br />
t = a; a = b; b = t; /* dumb swap */<br />
<br />
a ^= b; b ^= a; a ^= b; /* clever swap */<br />
<br />
...which is a very minor example. It can almost be excused, because the "cleverness" is confined to a tiny part of the code. But if the ''clever'' code had sub-structure, that sub-structure would be very hard to work on. You can't be sure you're not screwing something up until you understand completely the environment in which you're working.<br />
<br />
''Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it.'' - [http://en.wikipedia.org/wiki/Brian_Kernighan Brian W. Kernighan]<br />
<br />
IMHO, beautiful code helps code quality because it improves communication between the code author and the code reader. Since code modifiers are also code readers, the quality of communication can lead either to a virtuous circle of improving quality, or a vicious circle of degrading quality.<br />
<br />
<br />
<br />
== Formatting Guidelines ==<br />
<br />
1. There should be no tabs in any Lustre or LNET files. The exceptions are libsysio (maintained by someone else), ldiskfs and kernel patches (also part of a non-CFS project).<br />
<br />
2. Blocks should be indented by 8 spaces.<br />
<br />
3. New files should contain the following along with the license boilerplate. This will cause vim and emacs to use spaces instead of tabs for indenting. If you use a different editor, it also needs to be set to use spaces for indening Lustre code.<br />
<pre style="background:lightgrey;"><br />
/* -*- mode: c; c-basic-offset: 8; indent-tabs-mode: nil; -*-<br />
* vim:expandtab:shiftwidth=8:tabstop=8:<br />
*/<br />
</pre><br />
4. All lines should wrap at 80 characters. If it's getting too hard to wrap there, you probably need to break it up into more functions. In some cases, it is acceptable to remove a few spaces between function arguments to avoid overflowing onto the next line.<br />
<br />
5. Don't have spaces or tabs on blank lines or at the end of lines. Find these with some regexps in your patch (grep, or in vim) before attaching it to bugzilla:<br />
<br />
/[ \t]$/<br />
<br />
6. Don't use "inline" unless you're doing something so performance critical that the function call overhead will make a difference -- in other words: never. It makes debugging harder.<br />
<br />
All of our wrapping, parenthesis, brace placement, etc. rules are basically Linux kernel rules, which are basically K&R. For those of you in need of a refresher, great detail is provided below.<br />
<br />
7. For Autoconf macros, follow the [http://www.gnu.org/software/autoconf/manual/html_node/Coding-Style.html style suggested in the autoconf manual].<br />
<br />
<pre><br />
AC_CACHE_CHECK([for EMX OS/2 environment], [ac_cv_emxos2],<br />
[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([], [return __EMX__;])],<br />
[ac_cv_emxos2=yes],<br />
[ac_cv_emxos2=no])])<br />
</pre><br />
or even<br />
<pre><br />
AC_CACHE_CHECK([for EMX OS/2 environment],<br />
[ac_cv_emxos2],<br />
[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([],<br />
[return __EMX__;])],<br />
[ac_cv_emxos2=yes],<br />
[ac_cv_emxos2=no])])<br />
</pre><br />
<br />
== Great detail ==<br />
<br />
a. When you wrap, the next line should start after the parenthesis:<br />
<pre style="background:lightgrey;"><br />
right:<br />
<br />
variable = do_something_complicated(long_argument, longer_argument,<br />
longest_argument(sub_argument,<br />
foo_argument),<br />
last_argument);<br />
<br />
if (some_long_condition(arg1, arg2, arg3) < some_long_value &&<br />
another_long_condition(very_long_argument_name,<br />
another_long_argument_name) ><br />
second_long_value) {<br />
}<br />
<br />
<br />
wrong:<br />
<br />
variable = do_something_complicated(long_argument, longer_argument,<br />
longest_argument(sub_argument, foo_argument),<br />
last_argument);<br />
<br />
if (some_long_condition(arg1, arg2, arg3) < some_long_value &&<br />
another_long_condition(very_long_argument_name,<br />
another_long_argument_name) ><br />
second_long_value) {<br />
}<br />
</pre><br />
b. If you're wrapping put the operators at the end of the line, and if there are no parentheses indent 8 more:<br />
<pre style="background:lightgrey;"><br />
off = le32_to_cpu(fsd->fsd_client_start) +<br />
cl_idx * le16_to_cpu(fsd->fsd_client_size);<br />
</pre><br />
c. Binary and ternary (but not unary) operators should be separated from their arguments by one space.<br />
<pre style="background:lightgrey;"><br />
a++;<br />
b |= c;<br />
d = f > g ? 0 : 1;<br />
</pre><br />
d. Function calls should be nestled against the parentheses, the parentheses should crowd the arguments, and one space after commas:<br />
<pre style="background:lightgrey;"><br />
right: do_foo(bar, baz);<br />
wrong: do_foo ( bar,baz );<br />
</pre><br />
<br />
e. All ''if'', ''for'', ''while'', etc. expressions should be separated by a space from the parenthesis, one space after the semicolons:<br />
<pre style="background:lightgrey;"><br />
for (a = 0; a < b; a++)<br />
if (a < b || a == c)<br />
while (1)<br />
</pre><br />
f. Opening braces should be on the same line as the line that introduces the block, except for function calls. Closing braces get their own line, except for "else".<br />
<pre style="background:lightgrey;"><br />
int foo(void)<br />
{<br />
if (bar) {<br />
this();<br />
that();<br />
} else if (baz) {<br />
;<br />
} else {<br />
;<br />
}<br />
do {<br />
cow();<br />
} while (0);<br />
}<br />
</pre><br />
g. If one part of a compound ''if'' block has braces, all should.<br />
<pre style="background:lightgrey;"><br />
right:<br />
<br />
if (foo) {<br />
bar();<br />
baz();<br />
} else {<br />
salmon();<br />
}<br />
<br />
wrong:<br />
<br />
if (foo) {<br />
bar();<br />
baz();<br />
} else<br />
moose();<br />
</pre><br />
h. When you make a macro, protect those who might call it by using do/while and parentheses; line up your backslashes:<br />
<pre style="background:lightgrey;"><br />
right:<br />
<br />
#define DO_STUFF(a) \<br />
do { \<br />
int b = (a) + MAGIC; \<br />
do_other_stuff(b); \<br />
} while (0)<br />
<br />
wrong:<br />
<br />
#define DO_STUFF(a) \<br />
{ \<br />
int b = a + MAGIC; \<br />
do_other_stuff(b); \<br />
}<br />
</pre><br />
i. If you nest preprocessor commands, use spaces to visually delineate:<br />
<pre style="background:lightgrey;"><br />
#ifdef __KERNEL__<br />
# include <goose><br />
# define MOOSE steak<br />
#else<br />
# include <mutton><br />
# define MOOSE prancing<br />
#endif<br />
</pre><br />
j. For very long #ifdefs include the conditional with each #endif to make it readable:<br />
<pre style="background:lightgrey;"><br />
#ifdef __KERNEL__<br />
# if LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,0)<br />
/* lots<br />
of<br />
stuff */<br />
# endif /* KERNEL_VERSION(2,5,0) */<br />
#else /* !__KERNEL__ */<br />
# if HAVE_FEATURE<br />
/* more<br />
* stuff */<br />
# endif<br />
#endif /* __KERNEL__ */ <br />
</pre><br />
k. Comments should have the leading '''/*''' on the same line as the comment, and the trailing '''*/''' at the end of the last comment line. Intermediate lines should start with a '''*''' aligned with the first line's '''*''':<br />
<pre style="background:lightgrey;"> <br />
/* This is a short comment */<br />
<br />
/* This is a multi-line comment. I wish the line would wrap already,<br />
* as I don't have much to write about. */<br />
</pre><br />
l. Function declarations absolutely should NOT go into .c files, unless they are forward declarations for static functions that can't otherwise be moved before the caller. Instead, the declaration should go into the most "local" header available (preferrably *_internal.h for a given piece of code).<br />
<br />
m. Structure and constant declarations should not be declared in multiple places. Put the struct into the most "local" header possible. If it is something that is passed over the wire it needs to go into lustre_idl.h, and needs to be correctly swabbed when the RPC message is unpacked.<br />
<br />
n. The types and printf/printk formats used by Lustre code are:<br />
<pre style="background:lightgrey;"><br />
__u64 LPU64/LPX64/LPD64 (unsigned, hex, signed)<br />
size_t LPSZ (or cast to int and use %u / %d)<br />
__u32/int %u/%x/%d (unsigned, hex, signed)<br />
(unsigned) long long %llu/%llx/%lld<br />
loff_t %lld after a cast to long long (unfortunately)<br />
</pre><br />
----<br />
*'''[http://wiki.lustre.org/index.php?title=Front_Page FrontPage]'''<br />
''</div>Eebhttp://wiki.old.lustre.org/index.php?title=Coding_Guidelines&diff=4273Coding Guidelines2008-02-23T01:56:10Z<p>Eeb: </p>
<hr />
<div>''All Lustre developers should follow the guidelines in this page very strictly to avoid problems during code merges later on. Please make the required changes to the default formatting rules in the editor you use to comply to the guidelines below.<br />
<br />
== Beautiful Code ==<br />
<br />
More important than the physical layout of code (which is covered in detail below) is the thought that code is ''beautiful'' to read. What makes code "beautiful" to me? Fundamentally, I'm talking about readability and obviousness - the code must not have secrets.<br />
<br />
* Separate "ideas" are clearly separated in the program layout - e.g. blank lines group related statements and separate unrelated statements.<br />
<br />
* Declarations are easy to read. The declarations should always be separated from the functional code. Declarations should be made as locally as possible to the code they are being used.<br />
<br />
* If it's a choice between breaking the 80-chars-per-line rule and code that doesn't look good (e.g. breaks indentation rules or doesn't visually contain procedure call parameters between the brackets or spreads over too many lines), then break the 80-chars-per-line rule. We _don't_ want stupidly long lines, but we all have enough screen resolution these days to not have to wrap lines at 80 chars.<br />
<br />
* Names are well chosen. ''"Does exactly what it says on the tin"'' is an English (UK) expression describing something that tells you what it's going to do and then does _exactly_ that. For example, when I open a box labelled "soap", I expect it to help me wash and maybe even smell nice. I'll get upset if it's no good at removing dirt, and really upset if it makes me break out in a rash. The name of a procedure or variable or structure member should tell you something about the entity, without giving you misleading information - just "what it says on the tin".<br />
<br />
* Names are well chosen. This time I mean that you don't choose names that easily become a different but valid (to the compiler) name if you make a spelling mistake. '''i''' and '''j''' aren't the worst example - '''req_portal''' and '''rep_portal''' are much worse (and taken from our own code!!!).<br />
<br />
* Names are well chosen. Don't be scared of long names, but_do_not_go_to_extremes_either. If long <br />
<br />
* Names are well chosen. I can't emphasise this issue enough - I hope you get the point.<br />
<br />
* Intelligent use of assertions. Assertions can be abused. Obviously, when over-used they hurt performance. And they can also make you think that the code author didn't know what she was doing and added them to help her ''learn'' the code. But when assertions are used properly, they combine the roles of ''active comment'' and ''software fuse''. As an ''active comment'' they tell you something about the program which you can trust more than a comment. As a ''software fuse'', they provide fault isolation between subsystem by letting you know when and where invariant assumptions are violated.<br />
<br />
I could go on, but I hope you get the idea. Notice that I didn't mention ''clever'' as a desirable attribute. It's only one step from<br />
''clever'' to ''tricky'' - consider...<br />
<br />
t = a; a = b; b = t; /* dumb swap */<br />
<br />
a ^= b; b ^= a; a ^= b; /* clever swap */<br />
<br />
...which is a very minor example. It can almost be excused, because the "cleverness" is confined to a tiny part of the code. But if the ''clever'' code had sub-structure, that sub-structure would be very hard to work on. You can't be sure you're not screwing something up until you understand completely the environment in which you're working.<br />
<br />
''Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it.'' - [http://en.wikipedia.org/wiki/Brian_Kernighan Brian W. Kernighan]<br />
<br />
IMHO, beautiful code helps code quality because it improves communication between the code author and the code reader. Since code modifiers are also code readers, the quality of communication can lead either to a virtuous circle of improving quality, or a vicious circle of degrading quality.<br />
<br />
<br />
<br />
== Formatting Guidelines ==<br />
<br />
1. There should be no tabs in any Lustre or LNET files. The exceptions are libsysio (maintained by someone else), ldiskfs and kernel patches (also part of a non-CFS project).<br />
<br />
2. Blocks should be indented by 8 spaces.<br />
<br />
3. New files should contain the following along with the license boilerplate. This will cause vim and emacs to use spaces instead of tabs for indenting. If you use a different editor, it also needs to be set to use spaces for indening Lustre code.<br />
<pre style="background:lightgrey;"><br />
/* -*- mode: c; c-basic-offset: 8; indent-tabs-mode: nil; -*-<br />
* vim:expandtab:shiftwidth=8:tabstop=8:<br />
*/<br />
</pre><br />
4. All lines should wrap at 80 characters. If it's getting too hard to wrap there, you probably need to break it up into more functions. In some cases, it is acceptable to remove a few spaces between function arguments to avoid overflowing onto the next line.<br />
<br />
5. Don't have spaces or tabs on blank lines or at the end of lines. Find these with some regexps in your patch (grep, or in vim) before attaching it to bugzilla:<br />
<br />
/[ \t]$/<br />
<br />
6. Don't use "inline" unless you're doing something so performance critical that the function call overhead will make a difference -- in other words: never. It makes debugging harder.<br />
<br />
All of our wrapping, parenthesis, brace placement, etc. rules are basically Linux kernel rules, which are basically K&R. For those of you in need of a refresher, great detail is provided below.<br />
<br />
7. For Autoconf macros, follow the [http://www.gnu.org/software/autoconf/manual/html_node/Coding-Style.html style suggested in the autoconf manual].<br />
<br />
<pre><br />
AC_CACHE_CHECK([for EMX OS/2 environment], [ac_cv_emxos2],<br />
[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([], [return __EMX__;])],<br />
[ac_cv_emxos2=yes],<br />
[ac_cv_emxos2=no])])<br />
</pre><br />
or even<br />
<pre><br />
AC_CACHE_CHECK([for EMX OS/2 environment],<br />
[ac_cv_emxos2],<br />
[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([],<br />
[return __EMX__;])],<br />
[ac_cv_emxos2=yes],<br />
[ac_cv_emxos2=no])])<br />
</pre><br />
<br />
== Great detail ==<br />
<br />
a. When you wrap, the next line should start after the parenthesis:<br />
<pre style="background:lightgrey;"><br />
right:<br />
<br />
variable = do_something_complicated(long_argument, longer_argument,<br />
longest_argument(sub_argument,<br />
foo_argument),<br />
last_argument);<br />
<br />
if (some_long_condition(arg1, arg2, arg3) < some_long_value &&<br />
another_long_condition(very_long_argument_name,<br />
another_long_argument_name) ><br />
second_long_value) {<br />
}<br />
<br />
<br />
wrong:<br />
<br />
variable = do_something_complicated(long_argument, longer_argument,<br />
longest_argument(sub_argument, foo_argument),<br />
last_argument);<br />
<br />
if (some_long_condition(arg1, arg2, arg3) < some_long_value &&<br />
another_long_condition(very_long_argument_name,<br />
another_long_argument_name) ><br />
second_long_value) {<br />
}<br />
</pre><br />
b. If you're wrapping put the operators at the end of the line, and if there are no parentheses indent 8 more:<br />
<pre style="background:lightgrey;"><br />
off = le32_to_cpu(fsd->fsd_client_start) +<br />
cl_idx * le16_to_cpu(fsd->fsd_client_size);<br />
</pre><br />
c. Binary and ternary (but not unary) operators should be separated from their arguments by one space.<br />
<pre style="background:lightgrey;"><br />
a++;<br />
b |= c;<br />
d = f > g ? 0 : 1;<br />
</pre><br />
d. Function calls should be nestled against the parentheses, the parentheses should crowd the arguments, and one space after commas:<br />
<pre style="background:lightgrey;"><br />
right: do_foo(bar, baz);<br />
wrong: do_foo ( bar,baz );<br />
</pre><br />
<br />
e. All ''if'', ''for'', ''while'', etc. expressions should be separated by a space from the parenthesis, one space after the semicolons:<br />
<pre style="background:lightgrey;"><br />
for (a = 0; a < b; a++)<br />
if (a < b || a == c)<br />
while (1)<br />
</pre><br />
f. Opening braces should be on the same line as the line that introduces the block, except for function calls. Closing braces get their own line, except for "else".<br />
<pre style="background:lightgrey;"><br />
int foo(void)<br />
{<br />
if (bar) {<br />
this();<br />
that();<br />
} else if (baz) {<br />
;<br />
} else {<br />
;<br />
}<br />
do {<br />
cow();<br />
} while (0);<br />
}<br />
</pre><br />
g. If one part of a compound ''if'' block has braces, all should.<br />
<pre style="background:lightgrey;"><br />
right:<br />
<br />
if (foo) {<br />
bar();<br />
baz();<br />
} else {<br />
salmon();<br />
}<br />
<br />
wrong:<br />
<br />
if (foo) {<br />
bar();<br />
baz();<br />
} else<br />
moose();<br />
</pre><br />
h. When you make a macro, protect those who might call it by using do/while and parentheses; line up your backslashes:<br />
<pre style="background:lightgrey;"><br />
right:<br />
<br />
#define DO_STUFF(a) \<br />
do { \<br />
int b = (a) + MAGIC; \<br />
do_other_stuff(b); \<br />
} while (0)<br />
<br />
wrong:<br />
<br />
#define DO_STUFF(a) \<br />
{ \<br />
int b = a + MAGIC; \<br />
do_other_stuff(b); \<br />
}<br />
</pre><br />
i. If you nest preprocessor commands, use spaces to visually delineate:<br />
<pre style="background:lightgrey;"><br />
#ifdef __KERNEL__<br />
# include <goose><br />
# define MOOSE steak<br />
#else<br />
# include <mutton><br />
# define MOOSE prancing<br />
#endif<br />
</pre><br />
j. For very long #ifdefs include the conditional with each #endif to make it readable:<br />
<pre style="background:lightgrey;"><br />
#ifdef __KERNEL__<br />
# if LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,0)<br />
/* lots<br />
of<br />
stuff */<br />
# endif /* KERNEL_VERSION(2,5,0) */<br />
#else /* !__KERNEL__ */<br />
# if HAVE_FEATURE<br />
/* more<br />
* stuff */<br />
# endif<br />
#endif /* __KERNEL__ */ <br />
</pre><br />
k. Comments should have the leading '''/*''' on the same line as the comment, and the trailing '''*/''' at the end of the last comment line. Intermediate lines should start with a '''*''' aligned with the first line's '''*''':<br />
<pre style="background:lightgrey;"> <br />
/* This is a short comment */<br />
<br />
/* This is a multi-line comment. I wish the line would wrap already,<br />
* as I don't have much to write about. */<br />
</pre><br />
l. Function declarations absolutely should NOT go into .c files, unless they are forward declarations for static functions that can't otherwise be moved before the caller. Instead, the declaration should go into the most "local" header available (preferrably *_internal.h for a given piece of code).<br />
<br />
m. Structure and constant declarations should not be declared in multiple places. Put the struct into the most "local" header possible. If it is something that is passed over the wire it needs to go into lustre_idl.h, and needs to be correctly swabbed when the RPC message is unpacked.<br />
<br />
n. The types and printf/printk formats used by Lustre code are:<br />
<pre style="background:lightgrey;"><br />
__u64 LPU64/LPX64/LPD64 (unsigned, hex, signed)<br />
size_t LPSZ (or cast to int and use %u / %d)<br />
__u32/int %u/%x/%d (unsigned, hex, signed)<br />
(unsigned) long long %llu/%llx/%lld<br />
loff_t %lld after a cast to long long (unfortunately)<br />
</pre><br />
----<br />
*'''[http://wiki.lustre.org/index.php?title=Front_Page FrontPage]'''<br />
''</div>Eebhttp://wiki.old.lustre.org/index.php?title=Coding_Guidelines&diff=4272Coding Guidelines2008-02-23T01:53:34Z<p>Eeb: </p>
<hr />
<div>''All Lustre developers should follow the guidelines in this page very strictly to avoid problems during code merges later on. Please make the required changes to the default formatting rules in the editor you use to comply to the guidelines below.<br />
<br />
== Beautiful Code ==<br />
<br />
More important than the physical layout of code (which is covered in detail below) is the thought that code is ''beautiful'' to read. What makes code "beautiful" to me? Fundamentally, I'm talking about readability and obviousness - the code must not have secrets.<br />
<br />
* Separate "ideas" are clearly separated in the program layout - e.g. blank lines group related statements and separate unrelated statements.<br />
<br />
* Declarations are easy to read. The declarations should always be separated from the functional code. Declarations should be made as locally as possible to the code they are being used.<br />
<br />
* If it's a choice between breaking the 80-chars-per-line rule and code that doesn't look good (breaks indentation rules, doesn't visually contain procedure call parameters between the brackets etc), then break the 80-chars-per-line rule. We _don't_ want stupidly long lines, but we all have enough screen resolution these days to not have to wrap lines at 80 chars.<br />
<br />
* Names are well chosen. ''"Does exactly what it says on the tin"'' is an English (UK) expression describing something that tells you what it's going to do and then does _exactly_ that. For example, when I open a box labelled "soap", I expect it to help me wash and maybe even smell nice. I'll get upset if it's no good at removing dirt, and really upset if it makes me break out in a rash. The name of a procedure or variable or structure member should tell you something about the entity, without giving you misleading information - just "what it says on the tin".<br />
<br />
* Names are well chosen. This time I mean that you don't choose names that easily become a different but valid (to the compiler) name if you make a spelling mistake. '''i''' and '''j''' aren't the worst example - '''req_portal''' and '''rep_portal''' are much worse (and taken from our own code!!!).<br />
<br />
* Names are well chosen. Don't be scared of long names, but_do_not_go_to_extremes_either. If long <br />
<br />
* Names are well chosen. I can't emphasise this issue enough - I hope you get the point.<br />
<br />
* Intelligent use of assertions. Assertions can be abused. Obviously, when over-used they hurt performance. And they can also make you think that the code author didn't know what she was doing and added them to help her ''learn'' the code. But when assertions are used properly, they combine the roles of ''active comment'' and ''software fuse''. As an ''active comment'' they tell you something about the program which you can trust more than a comment. As a ''software fuse'', they provide fault isolation between subsystem by letting you know when and where invariant assumptions are violated.<br />
<br />
I could go on, but I hope you get the idea. Notice that I didn't mention ''clever'' as a desirable attribute. It's only one step from<br />
''clever'' to ''tricky'' - consider...<br />
<br />
t = a; a = b; b = t; /* dumb swap */<br />
<br />
a ^= b; b ^= a; a ^= b; /* clever swap */<br />
<br />
...which is a very minor example. It can almost be excused, because the "cleverness" is confined to a tiny part of the code. But if the ''clever'' code had sub-structure, that sub-structure would be very hard to work on. You can't be sure you're not screwing something up until you understand completely the environment in which you're working.<br />
<br />
''Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it.'' - [http://en.wikipedia.org/wiki/Brian_Kernighan Brian W. Kernighan]<br />
<br />
IMHO, beautiful code helps code quality because it improves communication between the code author and the code reader. Since code modifiers are also code readers, the quality of communication can lead either to a virtuous circle of improving quality, or a vicious circle of degrading quality.<br />
<br />
<br />
<br />
== Formatting Guidelines ==<br />
<br />
1. There should be no tabs in any Lustre or LNET files. The exceptions are libsysio (maintained by someone else), ldiskfs and kernel patches (also part of a non-CFS project).<br />
<br />
2. Blocks should be indented by 8 spaces.<br />
<br />
3. New files should contain the following along with the license boilerplate. This will cause vim and emacs to use spaces instead of tabs for indenting. If you use a different editor, it also needs to be set to use spaces for indening Lustre code.<br />
<pre style="background:lightgrey;"><br />
/* -*- mode: c; c-basic-offset: 8; indent-tabs-mode: nil; -*-<br />
* vim:expandtab:shiftwidth=8:tabstop=8:<br />
*/<br />
</pre><br />
4. All lines should wrap at 80 characters. If it's getting too hard to wrap there, you probably need to break it up into more functions. In some cases, it is acceptable to remove a few spaces between function arguments to avoid overflowing onto the next line.<br />
<br />
5. Don't have spaces or tabs on blank lines or at the end of lines. Find these with some regexps in your patch (grep, or in vim) before attaching it to bugzilla:<br />
<br />
/[ \t]$/<br />
<br />
6. Don't use "inline" unless you're doing something so performance critical that the function call overhead will make a difference -- in other words: never. It makes debugging harder.<br />
<br />
All of our wrapping, parenthesis, brace placement, etc. rules are basically Linux kernel rules, which are basically K&R. For those of you in need of a refresher, great detail is provided below.<br />
<br />
7. For Autoconf macros, follow the [http://www.gnu.org/software/autoconf/manual/html_node/Coding-Style.html style suggested in the autoconf manual].<br />
<br />
<pre><br />
AC_CACHE_CHECK([for EMX OS/2 environment], [ac_cv_emxos2],<br />
[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([], [return __EMX__;])],<br />
[ac_cv_emxos2=yes],<br />
[ac_cv_emxos2=no])])<br />
</pre><br />
or even<br />
<pre><br />
AC_CACHE_CHECK([for EMX OS/2 environment],<br />
[ac_cv_emxos2],<br />
[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([],<br />
[return __EMX__;])],<br />
[ac_cv_emxos2=yes],<br />
[ac_cv_emxos2=no])])<br />
</pre><br />
<br />
== Great detail ==<br />
<br />
a. When you wrap, the next line should start after the parenthesis:<br />
<pre style="background:lightgrey;"><br />
right:<br />
<br />
variable = do_something_complicated(long_argument, longer_argument,<br />
longest_argument(sub_argument,<br />
foo_argument),<br />
last_argument);<br />
<br />
if (some_long_condition(arg1, arg2, arg3) < some_long_value &&<br />
another_long_condition(very_long_argument_name,<br />
another_long_argument_name) ><br />
second_long_value) {<br />
}<br />
<br />
<br />
wrong:<br />
<br />
variable = do_something_complicated(long_argument, longer_argument,<br />
longest_argument(sub_argument, foo_argument),<br />
last_argument);<br />
<br />
if (some_long_condition(arg1, arg2, arg3) < some_long_value &&<br />
another_long_condition(very_long_argument_name,<br />
another_long_argument_name) ><br />
second_long_value) {<br />
}<br />
</pre><br />
b. If you're wrapping put the operators at the end of the line, and if there are no parentheses indent 8 more:<br />
<pre style="background:lightgrey;"><br />
off = le32_to_cpu(fsd->fsd_client_start) +<br />
cl_idx * le16_to_cpu(fsd->fsd_client_size);<br />
</pre><br />
c. Binary and ternary (but not unary) operators should be separated from their arguments by one space.<br />
<pre style="background:lightgrey;"><br />
a++;<br />
b |= c;<br />
d = f > g ? 0 : 1;<br />
</pre><br />
d. Function calls should be nestled against the parentheses, the parentheses should crowd the arguments, and one space after commas:<br />
<pre style="background:lightgrey;"><br />
right: do_foo(bar, baz);<br />
wrong: do_foo ( bar,baz );<br />
</pre><br />
<br />
e. All ''if'', ''for'', ''while'', etc. expressions should be separated by a space from the parenthesis, one space after the semicolons:<br />
<pre style="background:lightgrey;"><br />
for (a = 0; a < b; a++)<br />
if (a < b || a == c)<br />
while (1)<br />
</pre><br />
f. Opening braces should be on the same line as the line that introduces the block, except for function calls. Closing braces get their own line, except for "else".<br />
<pre style="background:lightgrey;"><br />
int foo(void)<br />
{<br />
if (bar) {<br />
this();<br />
that();<br />
} else if (baz) {<br />
;<br />
} else {<br />
;<br />
}<br />
do {<br />
cow();<br />
} while (0);<br />
}<br />
</pre><br />
g. If one part of a compound ''if'' block has braces, all should.<br />
<pre style="background:lightgrey;"><br />
right:<br />
<br />
if (foo) {<br />
bar();<br />
baz();<br />
} else {<br />
salmon();<br />
}<br />
<br />
wrong:<br />
<br />
if (foo) {<br />
bar();<br />
baz();<br />
} else<br />
moose();<br />
</pre><br />
h. When you make a macro, protect those who might call it by using do/while and parentheses; line up your backslashes:<br />
<pre style="background:lightgrey;"><br />
right:<br />
<br />
#define DO_STUFF(a) \<br />
do { \<br />
int b = (a) + MAGIC; \<br />
do_other_stuff(b); \<br />
} while (0)<br />
<br />
wrong:<br />
<br />
#define DO_STUFF(a) \<br />
{ \<br />
int b = a + MAGIC; \<br />
do_other_stuff(b); \<br />
}<br />
</pre><br />
i. If you nest preprocessor commands, use spaces to visually delineate:<br />
<pre style="background:lightgrey;"><br />
#ifdef __KERNEL__<br />
# include <goose><br />
# define MOOSE steak<br />
#else<br />
# include <mutton><br />
# define MOOSE prancing<br />
#endif<br />
</pre><br />
j. For very long #ifdefs include the conditional with each #endif to make it readable:<br />
<pre style="background:lightgrey;"><br />
#ifdef __KERNEL__<br />
# if LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,0)<br />
/* lots<br />
of<br />
stuff */<br />
# endif /* KERNEL_VERSION(2,5,0) */<br />
#else /* !__KERNEL__ */<br />
# if HAVE_FEATURE<br />
/* more<br />
* stuff */<br />
# endif<br />
#endif /* __KERNEL__ */ <br />
</pre><br />
k. Comments should have the leading '''/*''' on the same line as the comment, and the trailing '''*/''' at the end of the last comment line. Intermediate lines should start with a '''*''' aligned with the first line's '''*''':<br />
<pre style="background:lightgrey;"> <br />
/* This is a short comment */<br />
<br />
/* This is a multi-line comment. I wish the line would wrap already,<br />
* as I don't have much to write about. */<br />
</pre><br />
l. Function declarations absolutely should NOT go into .c files, unless they are forward declarations for static functions that can't otherwise be moved before the caller. Instead, the declaration should go into the most "local" header available (preferrably *_internal.h for a given piece of code).<br />
<br />
m. Structure and constant declarations should not be declared in multiple places. Put the struct into the most "local" header possible. If it is something that is passed over the wire it needs to go into lustre_idl.h, and needs to be correctly swabbed when the RPC message is unpacked.<br />
<br />
n. The types and printf/printk formats used by Lustre code are:<br />
<pre style="background:lightgrey;"><br />
__u64 LPU64/LPX64/LPD64 (unsigned, hex, signed)<br />
size_t LPSZ (or cast to int and use %u / %d)<br />
__u32/int %u/%x/%d (unsigned, hex, signed)<br />
(unsigned) long long %llu/%llx/%lld<br />
loff_t %lld after a cast to long long (unfortunately)<br />
</pre><br />
----<br />
*'''[http://wiki.lustre.org/index.php?title=Front_Page FrontPage]'''<br />
''</div>Eebhttp://wiki.old.lustre.org/index.php?title=Architecture_-_Proxy_Cache&diff=9779Architecture - Proxy Cache2007-12-10T12:12:16Z<p>Eeb: </p>
<hr />
<div>== Definitions ==<br />
<br />
; '''server''' : By default, the entire distributed entity exporting a lustre file system, both data and metadata (i.e. not the individual server processes of which it is composed).<br />
<br />
; '''proxy''' : An intermediate server that aggregates its clients' filesystem operations onto the backend server it exports to them.<br />
<br />
; '''caching proxy''' : A proxy that caches data and/or metadata.<br />
<br />
; '''NV caching proxy''' : Non-volatile caching proxy - i.e. a proxy that extends its cache onto local non-volatile memory (e.g. disk, flash).<br />
<br />
; '''disconnected operation''' : How the proxy operates when one or more of its component servers can no longer communicate with one or more components of its backend servers.<br />
<br />
== Summary ==<br />
<br />
A proxy uses caching and aggregation to reduce the load on its backend<br />
server and to provide better throughput and latency to its clients.<br />
Both volatile (in-store) and non-volatile (flash, disk) cache may be<br />
applied in any combination (including none at all), as determined by<br />
caching policies. This gives the proxy many uses from simple<br />
connection aggregation to limit backend server fan-out, to filesystem<br />
replication over a wide-area network.<br />
<br />
A proxy translates credentials between the client side and the backend<br />
server side. This translation implements a security policy on the<br />
proxy's clients.<br />
<br />
== Requirements ==<br />
<br />
=== Cache control ===<br />
<br />
Manage cache contention between different backends.<br />
<br />
Cache policies<br />
- Prefetch (what to cache aggressively)<br />
- writeback (how much can backend lag)<br />
- granularity <br />
- whole F/S <br />
- single file<br />
- filesets?<br />
<br />
<br />
<br />
F/S snapshot - "cut" over servers!<br />
persistent lock pre-empts reduced coherence debate?<br />
google uses persistent locks?<br />
<br />
Performance<br />
<br />
(partial) Disconnected operation<br />
<br />
partial disconnected operation<br />
<br />
Strong coherence<br />
<br />
Local stable storage<br />
<br />
Clustered - Restriping - Data and Metadata<br />
<br />
== Use Cases ==<br />
<br />
Connection offload (Security! SSL etc)<br />
<br />
UID translation<br />
<br />
== References ==<br />
[[Category:Architecture]]<br />
[[Category:Team Rabbit]]</div>Eebhttp://wiki.old.lustre.org/index.php?title=Architecture_-_Network_Request_Scheduler&diff=9767Architecture - Network Request Scheduler2007-12-10T12:04:33Z<p>Eeb: </p>
<hr />
<div>== Definitions ==<br />
<br />
; '''NRS''' : Network Request Scheduler.<br />
<br />
; '''RPC Concurrency''' : The number of RPC requests in-flight RPC between a given client and a server.<br />
<br />
; '''Active fan-out''' : The number of clients with in-flight requests to a given server at a given time.<br />
<br />
; '''Offset stream''' : The sequence of file or disk offsets in a stream of I/O requests.<br />
<br />
; '''"Before" relation (&le;)''' : File system operations that require ordering for correctness are related by "&le;". For 2 operations ''a'' and ''b'', if ''a'' &le; ''b'', then operation ''a'' must complete reading/writing file system state before operation ''b'' can start.<br />
<br />
; '''POP''' : Partial Order Preservation. A filesystem's POP capability describes how its servers handle any "before" relations required on RPC sent to them. Servers with no POP capability have no concept of any "before" relation on incoming RPCs so clients are completely responsible for preserving it. Servers with local POP capability preserve the "before" relation within a single server, but clients are responsible for preserving any required order on RPCs sent to different servers. A set of servers with global POP capability preserves the "before" relation on all RPCs.<br />
<br />
== Summary ==<br />
<br />
The Network Request Scheduler manages incoming RPC requests on a server to<br />
provide improved and consistent performance. It does this primarily by ordering<br />
request execution to avoid client starvation and to present a workload to the<br />
backend filesystem that can be optimized more easily. It may also change RPC<br />
concurrency as active fan-out varies to reduce latency seen by the client and<br />
limit request buffering on the server.<br />
<br />
== Requirements ==<br />
<br />
=== POP Capability ===<br />
<br />
The NRS must implement any POP capability its clients require.<br />
<br />
Current Lustre servers have no POP capability therefore clients may never issue<br />
RPCs concurrently that have a "before" relation - viz. metadata RPCs are<br />
synchronous and dirty data must have been written back before locks can start to<br />
be released. This leaves the NRS free to reorder all incoming RPCs.<br />
<br />
Any POP capability should permit better RPC pipelining for improved throughput<br />
to single clients and better latency hiding when resolving lock conflicts.<br />
<br />
The implementation may choose to implement a very simple POP capability that<br />
only works for the most important use cases, since it can revert to synchronous<br />
client behaviour in complex cases.<br />
<br />
An implementation may create additional "before" relations between RPCs provided<br />
they do not conflict with any "real" ordering (i.e. no cycles in the global<br />
"before" graph). This may allow a more compact "wire" representation of the<br />
"before" relation and/or just a simpler overall implementation, at the expense<br />
of reducing the scope to optimize request order.<br />
<br />
Consider RPC requests ''a'' &le; ''b''. Implementations that could allow<br />
request ''b'' to reach a server before request ''a'' will have to log completed<br />
requests for the duration of a server epoch.<br />
<br />
A global POP capability seems to require too much and too fine-grained<br />
inter-server communication which will make it hard to implement efficiently. It<br />
should probably not be considered unless a significant use-case arises.<br />
<br />
=== Scalability ===<br />
<br />
The number of RPC requests the server may buffer at any time is the product of<br />
RPC concurrency and active fan-out - i.e. potentially many thousands of<br />
requests. Request scheduling operations should have complexity of O(log(n)) at<br />
most.<br />
<br />
=== Offset Stream Consistency ===<br />
<br />
The backend filesystem allocator determines the disk offset stream when a given<br />
file is first written. It may even turn a random file offset stream into a<br />
substantially sequential disk offset stream. The disk offset stream is<br />
repeated when the file is read, provided the file offset stream hasn't changed.<br />
Request ordering should therefore be as reproducible as possible in the face of<br />
ordering "noise" caused by network unfairness or client races.<br />
<br />
Clients should pass a "hint" in RPC requests to ensure related offset streams<br />
can be identified, reordered and merged consistently on a multi-user cluster.<br />
This "hint" should also be passed through to the backend file system and used by<br />
its allocator. The "hint" may also become the basis of a resource reservation<br />
system to guarantee share of server resource to concurrent jobs.<br />
<br />
=== Request Priority ===<br />
<br />
Request priorities enable important requests to be serviced with lower latency -<br />
e.g. writes required to clean a cache on a locking conflict. Note that high<br />
priority requests must not break any POP requirements.<br />
<br />
=== RPC Concurrency ===<br />
<br />
There are conflicting pressures on RPC concurrency. It should be high when<br />
maximum individual client performance is required - e.g. when active fan-out is<br />
low on the server and there is spare server bandwidth, or when a client must<br />
clean its cache on a lock conflict. It should be low at times of high active<br />
fan-out to reduce buffering required on the server and to limit the latency of<br />
individual client requests.<br />
<br />
=== Extendability ===<br />
<br />
The NRS must inter-operate with non-NRS-aware clients and peers, making "best<br />
efforts" scheduling descisions for them. This same policy must apply to<br />
successive client and server versions.<br />
<br />
== References ==<br />
[[Category:Architecture]]<br />
[[Category:Team_Rabbit]]</div>Eebhttp://wiki.old.lustre.org/index.php?title=Architecture_-_Libcfs&diff=9765Architecture - Libcfs2007-11-12T23:17:58Z<p>Eeb: /* Summary */</p>
<hr />
<div>----<br />
<br />
== Summary ==<br />
<br />
Libcfs provides an API comprising fundamental primitives and<br />
subsystems - e.g. process management and debugging support - used<br />
throughout Lnet, Lustre and associated utilities. This API defines a<br />
portable runtime environment implemented consistently on all supported<br />
build targets.<br />
<br />
== Requirements ==<br />
<br />
=== Runtime Environment ===<br />
<br />
Libcfs provides both process and utility environments. The process<br />
environment provides runtime support for instances of Lnet and Lustre<br />
(client and server) running both in kernel and user space. The<br />
utility environment provides runtime support for userspace application<br />
programs used to configure, control and test Lnet and Lustre.<br />
<br />
The specific build target is exposed to libcfs users via macro<br />
definitions which allow conditional compilation and build-time<br />
assertions. There are separate macro definitions at each level -<br />
e.g. utility v. process at the top level, kernel v. userspace within<br />
process etc. The main targets are listed below...<br />
<br />
==== Process Environment ====<br />
<br />
'''Kernel'''<br />
<br />
Linux is the only fully supported target at this time. Code exists<br />
for Darwin and Windows which may be resurrected as required. A<br />
Solaris target will be required for native Solaris lustre clients.<br />
<br />
Both hard and soft interrupt contexts are supported within LNDs (NB<br />
these may only call back into Lnet in thread context). Sift interrupt<br />
context is supported for timer callbacks. All other code must run in<br />
thread context.<br />
<br />
'''Userspace'''<br />
<br />
Userspace targets are broadly posix runtimes. Single threaded<br />
runtimes (e.g. catamount) elide all locking primitives. Multi-threaded<br />
runtimes use posix threads. Signal safety is not defined.<br />
<br />
==== Utility Environment ====<br />
<br />
APIs available in the utility environment are a subset of those<br />
available in the process environment for the multi-threaded userspace<br />
target.<br />
<br />
=== C dialect ===<br />
<br />
Libcfs (and Lnet and Lustre) requires C99 syntax for structure member<br />
initialization (e.g. {.member = val}).<br />
<br />
=== APIs ===<br />
<br />
Libcfs defines abstract types and APIs to operate on them. These can be<br />
grouped broadly as follows...<br />
<br />
* Scalar types with explicit precision (e.g. __u64)<br />
** Byte swapping<br />
** 64 bit arithmetic<br />
* Time<br />
** Wallclock time<br />
** Ticks<br />
** Timers<br />
* Debugging<br />
** CDEBUG etc<br />
** stack dumping etc<br />
* Lists<br />
* NID and process ID abstractions<br />
* Process Management<br />
** Process creation and termination<br />
** Process attributes<br />
** Locking<br />
** Wait queues and/or condition variables<br />
* Memory management<br />
** Allocate/Free <br />
** Leak detection<br />
** Mapping<br />
* etc (list incomplete)...<br />
<br />
=== Header Files ===<br />
<br />
All libcfs declarations for all build targets and runtimes must be<br />
included via a single public header file e.g...<br />
<br />
#include <libcfs.h><br />
<br />
Target-specific header files as determined by preprocessor directives<br />
are then included from this single public header. <br />
<br />
The graph of header file dependencies within Libcfs must be a simple<br />
tree.<br />
<br />
Compile-time assertions must be provided to ensure no libcfs header<br />
files apart from the single public header are included directly.<br />
<br />
The build system must define the macro "__LIBCFS_UTIL__" when building<br />
utilities so that the correct environment is provided.<br />
<br />
== Conversion Process ==<br />
<br />
It is the intention that all parts of Lnet and Lustre which are<br />
sufficiently generic should use Libcfs APIs, however this has to be a<br />
gradual process so that conversion does not block ongoing Lustre<br />
development - i.e. it must be possible for individual subsystems to be<br />
converted independently.<br />
<br />
The biggest obstacle is Liblustre. It currently duplicates much<br />
functionality that should be provided by Libcfs however gradual<br />
conversion within Liblustre is impossible - name clashes break the<br />
userspace build once libcfs headers are included.<br />
<br />
Common Lnet code will benefit from conversion to the Libcfs process<br />
management and synchronisation APIs since that will provide support<br />
for the multi-threaded userspace process environment. This in turn<br />
will provide better concurrency and therefore better CPU utilisation<br />
on SMPs. <br />
<br />
The extent to which LNDs exploit Libcfs APIs depends utterly on the<br />
LND. The most generic LNDs (e.g. userspace socklnd) should use Libcfs<br />
APIs almost exclusively. The most specific LNDs (e.g. viblnd) may<br />
hardly use Libcfs at all (e.g. only debug and allocation).<br />
<br />
== References ==<br />
[[Category:Architecture|Libcfs]]</div>Eebhttp://wiki.old.lustre.org/index.php?title=Architecture_-_User_Level_Access&diff=9731Architecture - User Level Access2007-08-10T18:43:21Z<p>Eeb: </p>
<hr />
<div>== Summary ==<br />
The LNET userspace API driver exports the LNET API to userspace so that userspace lustre servers and clients can use all existing kernel LNET infrastructure.<br />
<br />
== Requirements ==<br />
; LNET API : Userspace processes can use the LNET API transparently to communicate with other processes of all flavours.<br />
<br />
; Multi-user : LNET users are distinguished by LNET PID. This PID may be shared by several O/S processes which may in turn be multi-threaded. Processes sharing the same LNET PID may share ME, MD and EQ handles. Kernel threads all share the same PID.<br />
<br />
; PID assignment : PID is requested in LNetNIInit(). If this is LNET_PID_ANY, a new unique PID is created and given the real userid of the caller. If the requested PID exists already, the caller may be permitted to join it. If it does not exist, the caller may be permitted to create it. <br />
<br />
; Privilege : Privileged users may create or join any PID. Unprivileged users must set the "userflag" when requesting a PID and my only join an existing PID if the caller's real userid matches the PID's userid.<br />
<br />
; Isolation : Replies, acks and events for a given PID are isolated from other PIDs, including future incarnations of the same PID.<br />
<br />
; Cleanup : A PID is destroyed when all its processes have finalized or terminated. At this time, all outstanding communications are completed with failure in finite time and all associated state is freed.<br />
<br />
; Parameter checking : NO userspace LNET API parameters are trusted. Checking may occur at any time - e.g. invalid buffer addresses may be discovered when a communication completes rather than when it was initiated.<br />
<br />
; Memory : User pages bound to an MD are locked down until the MD has been destroyed.<br />
<br />
; EQ Callbacks : EQ callbacks in userspace are not supported. Userspace processes may dedicate one or more threads to process events eagerly.<br />
<br />
; Thread safety : All exported LNET API calls are thread-safe.<br />
<br />
; Zero-copy : Communications to/from userspace buffers are zero-copy where the kernel LND supports it.<br />
<br />
; Resource Limits : LNET kernel module parameters allow resource limits (e.g. extant PIDs, MEs, MDs, concurrent GETs and PUTs) to be set for two classes of user: privileged and unprivileged. These limits include global maxima to limit the total resource that either class of process may use in aggregate. Either class may be set to infinity or zero. The normal "getrlimit" resource limits will apply to pages associated with MDs.<br />
<br />
== References ==<br />
[[category:Architecture|User Level Access]]</div>Eeb