From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 9900F11C5 for ; Wed, 8 Apr 2015 21:51:35 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga103.fm.intel.com with ESMTP; 08 Apr 2015 12:51:34 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,545,1422950400"; d="scan'208";a="692382012" Received: from irsmsx105.ger.corp.intel.com ([163.33.3.28]) by fmsmga001.fm.intel.com with ESMTP; 08 Apr 2015 12:51:33 -0700 Received: from irsmsx109.ger.corp.intel.com ([169.254.13.95]) by irsmsx105.ger.corp.intel.com ([169.254.7.2]) with mapi id 14.03.0224.002; Wed, 8 Apr 2015 20:51:32 +0100 From: "Butler, Siobhan A" To: Stephen Hemminger Thread-Topic: [dpdk-dev] tools brainstorming Thread-Index: AQHQYx105wa2dYsDE0GLqygeszi7351DCqJggABuhICAACszwA== Date: Wed, 8 Apr 2015 19:51:32 +0000 Message-ID: <0C5AFCA4B3408848ADF2A3073F7D8CC86D590354@IRSMSX109.ger.corp.intel.com> References: <3571725.20GtF5MAnU@xps13> <0C5AFCA4B3408848ADF2A3073F7D8CC86D58F9C2@IRSMSX109.ger.corp.intel.com> <20150408111603.3497f306@urahara> In-Reply-To: <20150408111603.3497f306@urahara> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] tools brainstorming X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Apr 2015 19:51:36 -0000 > -----Original Message----- > From: Stephen Hemminger [mailto:stephen@networkplumber.org] > Sent: Wednesday, April 8, 2015 7:16 PM > To: Butler, Siobhan A > Cc: Thomas Monjalon; dev@dpdk.org > Subject: Re: [dpdk-dev] tools brainstorming >=20 > Thanks for doing this, it is a great start. > I admit strong bias towards Linux kernel style. >=20 > Could you use one of the standard markup styles so that it could get put = in > documentation? Sure Stephen - will tidy it all up when we get a consensus :)=20 Thanks for the feedback=20 Siobhan=20 >=20 >=20 > > License Header > > -------------- >=20 > I prefer the file just say that it is BSD or GPL and refer to license fil= es in the > package. That way if something has to change it doesn't need a massive > license sweep >=20 >=20 > > > > Macros > > > > Do not ``#define`` or declare names in the implementation namespace > except for implementing application interfaces. > > > > 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 s= ame > 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 =3D (x) + (y); \ > > (y) +=3D 2; \ > > }while (0) > ^ bad whitespace >=20 > it is important that all examples in documentation are perfect. >=20 >=20 > > C Function Definition, Declaration and Use > > > > Prototypes > > > > It is recommended, but not required that all functions are prototyped > somewhere. > > > > Any function prototypes for private functions (that is, functions not u= sed > elsewhere) go at the top of the first source module. Functions > > local to one source module should be declared static. >=20 > I find prototypes for private functions to be redundant and error prone. > The do nothing. Better to just put private functions in the correct order= . >=20 >=20 > You also need to raise the issue that all global names need to be preface= d by > a unique string. > I see places in drivers where global names leak out causing possible late= r > name collision. >=20 > > 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) > > { >=20 > Not a big fan of that style. Prefer it on same line. >=20 >=20 > > > > Indentation is a hard tab, that is, a tab character, not a sequence of = spaces. >=20 > Also no spaces before tabs. >=20 > > NOTE General rule in DPDK, use tabs for indentation, spaces for alignme= nt. > > If you have to wrap a long statement, put the operator at the end of th= e > line, and indent again. For control statements (if, while, etc.), > > it is recommended that the next line be indented by two tabs, rather th= an > 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 =3D=3D really_long_variable_name_2 = && > > var3 =3D=3D var4){ > > x =3D y + z; /* control stmt body lines up with second line o= f */ > > a =3D b + c; /* control statement itself if single indent use= d */ > > } > > > > if (really_long_variable_name_1 =3D=3D really_long_variable_name_2 && > > var3 =3D=3D var4){ /* two tabs used */ >=20 > No. Should line up with really_long_variable_name_1 >=20 > > x =3D y + z; /* statement body no longer lines up */ > > a =3D b + c; > > } > > > > z =3D 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. Bra= ces > 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 afte= r > them. No spaces after ``(`` or ``[`` or preceding the ``]`` or ``)`` char= acters. > > error =3D function(a1, a2); > > if (error !=3D 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. > > > > Exit > > > > Exits should be 0 on success, or 1 on failure. > > exit(0); /* > > * Avoid obvious comments such as > > * "Exit 0 on success." > > */ >=20 > +11 >=20 >=20 > > > > 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. >=20 > Sorry, this is a stupid BSDism > return (-EINVAL); > is ugly >=20 > > > > 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"); > > > > > > In a userland program that is not a DPDK application, use err(3) or war= n(3). > Do not create your own variant. > > if ((four =3D malloc(sizeof(struct foo))) =3D=3D NULL) > > err(1, (char *)NULL); > > if ((six =3D (int *)overflow()) =3D=3D NULL) > > errx(1, "number overflowed"); > > return (eight); > ^ NO BSD style return () >=20 > > > > 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 > > if (likely(x > 1)) > > do_stuff(); >=20 > You need to stress that likely() and unlikely() should be used sparingly. > Many times the compiler knows better. >=20 > > > > > > Static Variables and Functions > > ------------------------------ > > > > All functions and variables that are local to a file must be declared a= s > ``static`` because it can often help the compiler to do > > some optimizations (such as, inlining the code). > > > > Functions that must be inlined have to be declared as ``static inline``= and > can be defined in a .c or a .h file. > > > > Const Attribute > > --------------- > > > > Particular care must be taken with the use of the ``const`` attribute. = It > should be used as often as possible when a variable is read-only. >=20 >=20 > A couple more things: > no UPPER or CamelCase variable names or functions >=20 > The other thing (and Intel is the worst offender) is to avoid excessive i= nlining. > For example, I am pretty sure that all rte_ring stuff really should not b= e > inline, > the compiler can do a good job (especially with LTO) even without inlinin= g.