[dpdk-dev] tools brainstorming
Neil Horman
nhorman at tuxdriver.com
Mon Apr 13 17:02:15 CEST 2015
On Wed, Apr 08, 2015 at 10:43:53AM +0000, Butler, Siobhan A wrote:
> Hi all,
> To add to the tools brainstorming - I propose we use the following Coding Standards as the basis of guidelines on coding style going forward.
> The style outlined below is in alignment with the current convention used for the majority of the project.
> Any thoughts/suggestions or feedback welcome.
> Thanks
> Siobhan :)
> <siobhan.a.butler at intel.com>
>
>
>
> Coding Style
> ~~~~~~~~~~
>
> Description
> -----------
>
> This document specifies the preferred style for source files in the DPDK source tree.
> It is based on the Linux Kernel coding guidelines and the FreeBSD 7.2 Kernel Developer's Manual (see man style(9)),
> but was heavily modified for the needs of the DPDK. Many of the style rules are implicit in the examples.
> Be careful to check the examples before assuming that style is silent on an issue.
>
> General Guidelines
> ------------------
>
> The rules and guidelines given in this document cannot cover every situation, so the following general guidelines should be used as a fallback:
> The code style should be consistent within each individual file, and within each file in a given directory or module - in the case of creating new files
> The primary reason for coding standards is to increase code readability and comprehensibility, therefore always use whatever option will make the code easiest to read.
>
> The following more specific recommendations apply to all sections, both for C and assembly code:
> Line length is recommended to be not more than 80 characters, including comments. [Tab stop size should be assumed to be at least 4-characters wide]
> Indentation should be to no more than 3 levels deep.
> NOTE The above are recommendations, and not hard limits. However, it is expected that the recommendations should be followed in all but the rarest situations.
> C Comment Style
>
> Usual Comments
> --------------
>
> These comments should be used in normal cases. To document a public API, a doxygen-like format must be used: refer to Doxygen Documentation.
> /*
> * VERY important single-line comments look like this.
> */
>
> /* Most single-line comments look like this. */
>
> /*
> * Multi-line comments look like this. Make them real sentences. Fill
> * them so they look like real paragraphs.
> */
>
> License Header
> --------------
>
> Each file should begin with a special comment tag which will contain the appropriate copyright and license for the file (Generally BSD License).
> After any copyright header, a blank line should be left before any other contents, e.g. include statements in a C file.
>
> C Preprocessor Directives
> -------------------------
>
> Header Includes
>
> In DPDK sources, the include files should be ordered as following:
> libc includes (system includes first)
> DPDK EAL includes
> DPDK misc libraries includes
> application-specific includes
>
> Example:
> #include <stdio.h>
> #include <stdlib.h>
>
> #include <rte_eal.h>
>
> #include <rte_ring.h>
> #include <rte_mempool.h>
>
> #include "application.h"
It doesn't really matter to me, for the sake of consistency, it might be
worthwhile mandating search path includes only (< >), and adding a -I . to the
CFLAGS in the Makefile. That way a grep for "*.*<.*>" returns all your include
files
>
>
> Global pathnames are defined in <paths.h>. Pathnames local to the program go in "pathnames.h" in the local directory.
> #include <paths.h>
This is a design issue, not a coding style issue. Where an application chooses
to put its pathnames are its business, and not something we should codify here.
Also, paths.h doesn't exist so it should probably not be referenced here
>
>
> Leave another blank line before the user include files.
> #include "pathnames.h" /* Local includes in double quotes. */
>
> NOTE Please avoid, as much as possible, including headers from other headers file. Doing so should be properly explained and justified.
> Headers should be protected against multiple inclusion with the usual:
Are you sure you want to do that?
[nhorman at hmsreliant dpdk]$ find . -name '*.h' | xargs grep include | wc -l
1300
What would the justification be? Its common practice to do this, so I'm not
sure why you would discourage it.
> #ifndef _FILE_H_
> #define _FILE_H_
>
> /* Code */
>
> #endif /* _FILE_H_ */
>
>
> Macros
>
> Do not ``#define`` or declare names in the implementation namespace except for implementing application interfaces.
>
I'm not sure I understand what this means. Can you clarify the intent here?
> The names of ``unsafe`` macros (ones that have side effects), and the names of macros for manifest constants, are all in uppercase.
>
> The expansions of expression-like macros are either a single token or have outer parentheses. If a macro is an inline expansion of a function,
> the function name is all in lowercase and the macro has the same name all in uppercase. Right-justify the backslashes;
> it makes it easier to read. If the macro encapsulates a compound statement, enclose it in a do loop, so that it can be used safely in if statements.
> Any final statement-terminating semicolon should be supplied by the macro invocation rather than the macro, to make parsing easier for pretty-printers and editors.
> #define MACRO(x, y) do { \
> variable = (x) + (y); \
> (y) += 2; \
> }while (0)
>
> NOTE Wherever possible, enums and typedefs should be preferred to macros, since they provide additional degrees
> of type-safety and can allow compilers to emit extra warnings about unsafe code.
>
> Conditional Compilation
> -----------------------
>
> When code is conditionally compiled using #ifdef or #if, a comment may be added following the matching #endif or #else to
> permit the reader to easily discern where conditionally compiled code regions end. This comment should be used only for
> (subjectively) long regions, regions greater than 20 lines, or where a series of nested #ifdef 's may be confusing to the reader.
> Exceptions may be made for cases where code is conditionally not compiled for the purposes of lint(1), even though the uncompiled
> region may be small. The comment should be separated from the #endif or #else by a single space. For short conditionally compiled regions,
> a closing comment should not be used.
>
> The comment for #endif should match the expression used in the corresponding #if or #ifdef. The comment for #else and #elif
> should match the inverse of the expression(s) used in the preceding #if and/or #elif statements. In the comments,
> the subexpression defined(FOO) is abbreviated as FOO. For the purposes of comments, #ifndef FOO is treated as #if !defined(FOO).
> #ifdef KTRACE
> #include <sys/ktrace.h>
> #endif
>
> #ifdef COMPAT_43
> /* A large region here, or other conditional code. */
> #else /* !COMPAT_43 */
> /* Or here. */
> #endif /* COMPAT_43 */
>
> #ifndef COMPAT_43
> /* Yet another large region here, or other conditional code. */
> #else /* COMPAT_43 */
> /* Or here. */
> #endif /* !COMPAT_43 */
>
> NOTE Conditional compilation should be used only when absolutely necessary, as it increases the number of target binaries that need to be built and tested.
> C Types
>
> Integers
>
> For fixed/minimum-size integer values, the project uses the form uintXX_t (from stdint.h) instead of older BSD-style integer identifiers of the form u_intXX_t.
>
> Enumerations
> ------------
>
> Enumeration values are all uppercase.
> enum enumtype { ONE, TWO } et;
>
>
> Bitfields
> ---------
>
> The developer should group bitfields that are included in the same integer, as follows:
> struct grehdr {
> uint16_t rec:3,
> srr:1,
> seq:1,
> key:1,
> routing:1,
> csum:1,
> version:3,
> reserved:4,
> ack:1;
> /* ... */
> }
>
>
> Variable Declarations
> ---------------------
>
> In declarations, do not put any whitespace between asterisks and adjacent tokens, except for tokens that are identifiers related to types.
> (These identifiers are the names of basic types, type qualifiers, and typedef-names other than the one being declared.)
> Separate these identifiers from asterisks using a single space.
>
> Structure Declarations
>
> When declaring variables in structures, declare them sorted by use, then by size (largest to smallest), and then in alphabetical order.
> Alignment constraints may override the previous rules. The first category normally does not apply, but there are exceptions.
> Each structure element gets its own line. Try to make the structure readable by aligning the member names using spaces as shown below.
> Names following extremely long types, which therefore cannot be easily aligned with the rest, should be separated by a single space.
> struct foo {
> struct foo *next; /* List of active foo. */
> struct mumble amumble; /* Comment for mumble. */
> int bar; /* Try to align the comments. */
> struct verylongtypename *baz; /* Won't fit with other members */
> };
>
This is going to cause conflicts with ABI preservation. While its fine to do
when creating a new structure, you need to be very careful about shuffling
structure members around on public facing structures. Recommend not mandating
this. The other option is to allow this, but start converting public facing
api's to use opaque types with get/set routines, so that library internals can
codify the offsets to member structures appropriately.
>
> Major structures should be declared at the top of the file in which they are used, or in separate header files if they are used
> in multiple source files. Use of the structures should be by separate declarations and should be extern if they are declared in a header file.
>
> Queues
>
> Use queue(3) macros rather than rolling your own lists, whenever possible. Thus, the previous example would be better written:
> #include <sys/queue.h>
>
> struct foo {
> LIST_ENTRY(foo) link; /* Use queue macros for foo lists. */
> struct mumble amumble; /* Comment for mumble. */
> int bar; /* Try to align the comments. */
> struct verylongtypename *baz; /* Won't fit with other members */
> };
> LIST_HEAD(, foo) foohead; /* Head of global foo list. */
>
>
> DPDK also provides an optimized way to store elements in lockless rings. This should be used in all data-path code, when there are several
> consumer and/or producers to avoid locking for concurrent access.
>
You probably want to reference the api directly in some way here, so people can
go look up how to do that.
> Typedefs
>
> Avoid using typedefs for structure types. For example, use:
> struct my_struct_type {
> /* ... */
> };
>
> struct my_struct_type my_var;
>
>
> rather than:
> typedef struct my_struct_type {
> /* ... */
> } my_struct_type;
>
> my_struct_type my_var
>
>
> Typedefs are problematic because they do not properly hide their underlying type; for example, you need to know if the typedef is
> the structure itself, as shown above, or a pointer to the structure.
This isn't really true. If you make the structure opaque, so that it references
a externally declared structure, then its just a handle, and can be used without
type knoweldge (assuming the appropriate API is built for it).
> In addition, they must be declared exactly once, whereas an
> incomplete structure type can be mentioned as many times as necessary. Typedefs are difficult to use in stand-alone header files.
> The header that defines the typedef must be included before the header that uses it,
This is an excellent reason to allow header include chains.
> or by the header that uses it (which causes namespace pollution),
> or there must be a back-door mechanism for obtaining the typedef.
> NOTE #defines used instead of typedefs also are problematic (since they do not propagate the pointer type correctly due to direct text replacement).
> For example, ``#define pint int *`` does not work as expected, while ``typedef int *pint`` does work. As stated when discussing macros, typedefs
> should be preferred to macros in cases like this.
> When convention requires a typedef; make its name match the struct tag. Avoid typedefs ending in ``_t``, except as specified in Standard C or by POSIX.
> /* Make the structure name match the typedef. */
> typedef struct bar {
> int level;
> } BAR;
> typedef int foo; /* This is foo. */
> typedef const long baz; /* This is baz. */
>
So, I'd suggest removing the explination here. The rule above seems reasonably
clear (don't typedef structures), but the reasoning sort of devolves into a
discussion on why typedefs and macros are hard (but still sometimes necessecary
as referenced above). I think it would be enough to say "DPDK prefers that
structures be used to codify complex data types as a matter of style". Its
really the reason to do so.
Note also, in mandating this, you are hindering the development of API's that
use opaque data types. You can still do it of course, but you have to be sure
to define your data types as anonymous structures:
extern struct foo;
rather than new types. Not a big deal, but something to be aware of.
>
> C Function Definition, Declaration and Use
>
> Prototypes
>
> It is recommended, but not required that all functions are prototyped somewhere.
IIRC it is actually required at the moment because public functions with no
prototypes generate warnings (-Wmising-prototypes is currently implied in one of
the gcc warning options)
>
> Any function prototypes for private functions (that is, functions not used elsewhere) go at the top of the first source module. Functions
> local to one source module should be declared static.
>
> Functions used from other parts of code (external API) must be prototyped in the relevant include file.
> Function prototypes should be listed in a logical order, preferably alphabetical unless there is a compelling reason to use a different ordering.
>
> Functions that are used locally in more than one module go into a separate header file, for example, "extern.h".
>
> Do not use the ``__P`` macro.
>
Just out of curiosity, has this been a problem? __P was introduced in c89, and
I don't think I've seen it in code since C99 was released. I don't mind
including it, but I'm curious to know the history here.
> Functions that are part of an external API should be documented using Doxygen-like comments above declarations. See the Doxgen documentation topic for details.
>
> Associate names with parameter types, for example:
> void function(int fd);
>
>
> Short function prototypes should be contained on a single line. Longer prototypes, e.g. those with many parameters,
> can be split across multiple lines. Multi-line prototypes should use space-indentation to enable function parameters to line up:
> static char *function1(int _arg, const char *_arg2,
> struct foo *_arg3,
> struct bar *_arg4,
> struct baz *_arg5);
2 things:
1) Clarify the meaning of space indentation. Is it ok to use tabs and spaces
for alignment, or are only spaces allowed (note the code currently uses the
former).
2) You say function prameters should "line up", but in the example you give,
they don't. What I think you want is:
static char *function1(int _arg, const char *_arg2,
struct foo *_arg3,
struct bar *_arg4,
struct baz *_arg5);
> static void usage(void);
>
>
> Definitions
> -----------
>
> The function type should be on a line by itself preceding the function. The opening brace of the function body should be on a line by itself.
> static char *
> function(int a1, int a2, float fl, int a4)
> {
>
The example immediately above this section doesn't follow this convention. You
should fix that.
>
> Do not declare functions inside other functions. ANSI C states that such declarations have file scope regardless of the nesting of the declaration.
> Hiding file declarations in what appears to be a local scope is undesirable and will elicit complaints from a good compiler.
>
> Old-style (K&R) function declaration should not be used, use ANSI function declarations instead as shown below. Long argument lists
> should be wrapped as described above in the function prototypes section.
> /*
> * All major routines should have a comment briefly describing what
> * they do. The comment before the "main" routine should describe
> * what the program does.
> */
> int
> main(int argc, char *argv[])
> {
> char *ep;
> long num;
> int ch;
>
>
> C Command Line Parsing
> ----------------------
>
> For consistency, getopt(3) should be used to parse options. Options should be sorted in the getopt(3) call and the switch statement,
> unless parts of the switch cascade. Elements in a switch statement that cascade should have a FALLTHROUGH comment.
> Numerical arguments should be checked for accuracy. Code that cannot be reached should have a NOTREACHED comment.
> while ((ch = getopt(argc, argv, "abNn:")) != -1)
> switch (ch) { /* Indent the switch. */
> case 'a': /* Don't indent the case. */
> aflag = 1; /* Indent case body one tab. */
> /* FALLTHROUGH */
> case 'b':
> bflag = 1;
> break;
> case 'N':
> Nflag = 1;
> break;
> case 'n':
> num = strtol(optarg, &ep, 10);
> if (num <= 0 || *ep != '\0') {
> warnx("illegal number, -n argument -- %s",
> optarg);
> usage();
> }
> break;
> case '?':
> default:
> usage();
> /* NOTREACHED */
> }
> argc -= optind;
> argv += optind;
>
I'm not sure we need this section. I understand we have lots of examples that
use getopt, but by and large this addresses application coding, which is
somewhat outside of the purview of the DPDK. I'm wholly supportive of some
style guidelines regarding switch statements mind you, but I'm not sure we need
to mandate the usage of getopt.
>
>
>
>
> C Indentation
> -------------
>
> Control Statements and Loops
>
> Include a space after keywords (if, while, for, return, switch). Do not use braces (``{`` and ``}``) for control statements with zero or just a
> single statement, unless that statement is more than a single line in which case the braces are permitted. Forever loops are done with for statements, not while statements.
> for (p = buf; *p != '\0'; ++p)
> ; /* nothing */
> for (;;)
> stmt;
> for (;;) {
> z = a + really + long + statement + that + needs +
> two + lines + gets + indented + on + the +
> second + and + subsequent + lines;
> }
> for (;;) {
> if (cond)
> stmt;
> }
> if (val != NULL)
> val = realloc(val, newsize);
>
>
> Parts of a for loop may be left empty. It is recommended that you do not put declarations inside blocks unless the routine is unusually complicated.
> for (; cnt < 15; cnt++) {
> stmt1;
> stmt2;
> }
>
>
> Indentation is a hard tab, that is, a tab character, not a sequence of spaces.
> NOTE General rule in DPDK, use tabs for indentation, spaces for alignment.
> If you have to wrap a long statement, put the operator at the end of the line, and indent again. For control statements (if, while, etc.),
> it is recommended that the next line be indented by two tabs, rather than one, to prevent confusion as to whether the second line of the
> control statement forms part of the statement body or not. For non-control statements, this issue does not apply, so they can be indented
> by a single tab. However, a two-tab indent is recommended in this case also to keep consistency across all statement types.
> while (really_long_variable_name_1 == really_long_variable_name_2 &&
> var3 == var4){
> x = y + z; /* control stmt body lines up with second line of */
> a = b + c; /* control statement itself if single indent used */
> }
>
> if (really_long_variable_name_1 == really_long_variable_name_2 &&
> var3 == var4){ /* two tabs used */
> x = y + z; /* statement body no longer lines up */
> a = b + c;
> }
>
> z = a + really + long + statement + that + needs +
> two + lines + gets + indented + on + the +
> second + and + subsequent + lines;
>
>
> Do not add whitespace at the end of a line.
>
> Closing and opening braces go on the same line as the else keyword. Braces that are not necessary should be left out.
> if (test)
> stmt;
> else if (bar) {
> stmt;
> stmt;
> } else
> stmt;
>
>
> Function Calls
> --------------
>
> Do not use spaces after function names. Commas should have a space after them. No spaces after ``(`` or ``[`` or preceding the ``]`` or ``)`` characters.
> error = function(a1, a2);
> if (error != 0)
> exit(error);
>
>
> Operators
> ---------
>
> Unary operators do not require spaces, binary operators do. Do not use parentheses unless they are required for precedence or unless the
> statement is confusing without them. Remember that other people may be more easily confused than you.
>
I recommend changing this such that all logical operators require spaces around
them. It simplifies the rules when writing code. Also, it appears from some
quick grepping that always including space around unary or binary logical
operations is the de-facto rule.
> Exit
>
> Exits should be 0 on success, or 1 on failure.
> exit(0); /*
> * Avoid obvious comments such as
> * "Exit 0 on success."
> */
> }
>
This again codifies application behavior, not coding style. I don't think we
need to mandate that as we have no purview over application behavior (example
applications being the exception here of course).
>
> Local Variables
> ---------------
>
> When declaring variables in functions, declare them sorted by size, then in alphabetical order. Multiple variables per line are OK.
> If a line overflows reuse the type keyword.
>
> Be careful to not obfuscate the code by initializing variables in the declarations, only the last variable on a line should be initialized.
> If multiple variables are to be initialised when defined, put one per line. Do not use function calls in initializers.
> int i = 0, j = 0, k = 0; /* bad, too many initializer */
>
> char a = 0; /* OK, one variable per line with initializer */
> char b = 0;
>
> float x, y = 0.0; /* OK, only last variable has initializer */
>
>
> Casts and sizeof
>
> Casts and sizeof statements are not followed by a space. Always write sizeof statements with parenthesis.
> The redundant parenthesis rules do not apply to sizeof(var) instances.
>
What redundant parenthesis rules? I presume you are referring to implied
practice of not including parenthesis in operations that don't change the order
of operations? If so, you probably want to state that clearly ealier so reader
have something to reference here.
> C Style and Conventions
>
> NULL Pointers
>
> NULL is the preferred null pointer constant. Use NULL instead of ``(type *)0`` or ``(type *)NULL`` in contexts where the compiler knows the type,
> for example, in assignments. Use ``(type *)NULL`` in other contexts, in particular for all function args.
> (Casting is essential for variadic args and is necessary for other args if the function prototype might not be in scope.) Test pointers against NULL, for example, use::
> (p = f()) == NULL
>
>
> not::
> !(p = f())
>
>
> Do not use ! for tests unless it is a boolean, for example, use::
> if (*p == '\0')
>
>
> not::
> if (!*p)
>
>
> Return Value
> ------------
>
> If possible, functions should return 0 on success and a negative value on error. The negative value should be ``-errno`` if relevant, for example, ``-EINVAL``.
>
> Routines returning ``void *`` should not have their return values cast to any pointer type.
> (Typecasting can prevent the compiler from warning about missing prototypes as any implicit definition of a function returns int - which, unlike "void *" needs a typecast to assign to a pointer variable.)
> NOTE The above rule applies to malloc, as well as to DPDK functions.
> Values in return statements should be enclosed in parentheses.
>
> Logging and Errors
> ------------------
>
> In the DPDK environment, use the logging interface provided::
> #define RTE_LOGTYPE_TESTAPP1 RTE_LOGTYPE_USER1
> #define RTE_LOGTYPE_TESTAPP2 RTE_LOGTYPE_USER2
>
> /* enable these logs type */
> rte_set_log_type(RTE_LOGTYPE_TESTAPP1, 1);
> rte_set_log_type(RTE_LOGTYPE_TESTAPP2, 1);
>
> /* log in debug level */
> rte_set_log_level(RTE_LOG_DEBUG);
> RTE_LOG(DEBUG, TESTAPP1, "this is is a debug level message\n");
> RTE_LOG(INFO, TESTAPP1, "this is is a info level message\n");
> RTE_LOG(WARNING, TESTAPP1, "this is is a warning level message\n");
>
> /* log in info level */
> rte_set_log_level(RTE_LOG_INFO);
> RTE_LOG(DEBUG, TESTAPP2, "debug level message (not displayed)\n");
>
While I'm not opposed at all to mandating a Log mechanism, it seems to me that
this is a review issue, not a coding style issue. It will also be almost
impossible to codify this requirement in a tool (how will a tool know when you
are trying to do logging, but aren't using the above log macros)?
>
> In a userland program that is not a DPDK application, use err(3) or warn(3). Do not create your own variant.
> if ((four = malloc(sizeof(struct foo))) == NULL)
> err(1, (char *)NULL);
> if ((six = (int *)overflow()) == NULL)
> errx(1, "number overflowed");
> return (eight);
> }
>
Not within our purview, don't include this. If its not part of the DPDK,
we don't get to mandate style on it.
>
> Variable Arguments List
> -----------------------
>
> Variable numbers of arguments should look like this:
> #include <stdarg.h>
>
> void
> vaf(const char *fmt, ...)
> {
> va_list ap;
>
> va_start(ap, fmt);
> STUFF;
> va_end(ap);
> /* No return needed for void functions. */
> }
>
> static void
> usage()
> {
> /* Insert an empty line if the function has no local variables. */
>
>
This isn't a coding style issue, this is just how variable arguments work in C.
I think you can remove this.
> Printf
> ------
>
> Use printf(3), not fputs(3), puts(3), putchar(3) or whatever. It is faster and usually cleaner, and helps to avoid unnecessary bugs. However, be aware of format string bugs::
> int
> main(int argc, char **argv)
> {
> if(argc != 2)
> exit(1);
> printf(argv[1]); /* bad ! */
> printf("%s", argv[1]); /* correct */
>
>
Again, not a coding style issue. There may be perfectly good reasons to use
putchar/putc/puts, I don't think our coding sytle guideline needs to restrict
their usage. Let developers review the appropriateness of the calls used in the
code.
> Usage
> -----
>
> Usage statements should look like the manual pages SYNOPSIS. The usage statement should be structured in the following order:
> 1. Options without operands come first, in alphabetical order, inside a single set of brackets (``[`` and ``]``).
> 2. Options with operands come next, also in alphabetical order, with each option and its argument inside its own pair of brackets.
> 3. Required arguments (if any) are next, listed in the order they should be specified on the command line.
> 4. Finally, any optional arguments, listed in the order they should be specified, and all inside brackets.
>
> A bar (`|') separates ``either-or`` options/arguments, and multiple options/arguments, which are specified together, are placed in a single set of brackets.
> "usage: f [-aDde] [-b b_arg] [-m m_arg] req1 req2 [opt1 [opt2]]\n"
> "usage: f [-a | -b] [-c [-dEe] [-n number]]\n"
>
> (void)fprintf(stderr, "usage: f [-ab]\n");
> exit(1);
> }
>
>
> Note that the manual page options description should list the options in pure alphabetical order. That is, without regard to
> whether an option takes arguments or not. The alphabetical ordering should take into account the case ordering shown above.
>
> Branch Prediction
> -----------------
>
> When a test is done in a critical zone (called often or in a data path) use the ``likely()`` and ``unlikely()`` macros. They are expanded
> as a compiler builtin and allow the developer to indicate if the branch is likely to be taken or not. Example:
> #include <rte_branch_prediction.h>
> if (likely(x > 1))
> do_stuff();
>
Don't need this. Developers know when to use proper branch prediction.
Mandating it can lead to inappropriate guesses, espeically if you codify this
check in a tool that mandates all conditionals be branch predicted.
The remainder of the document looks fine
Neil
More information about the dev
mailing list