X-Git-Url: https://git.librecmc.org/?a=blobdiff_plain;ds=sidebyside;f=docs%2Fstyle-guide.txt;h=71eb6291444f61aa5441f2114f22ef43e82df9e1;hb=e0fe93759339a9e043cd0489f5bfabd59b5fcb78;hp=374a822b23ed51d8718712cf4ffe0ec5736e9912;hpb=3680c58084410768b562cb1982a4189d33880031;p=oweals%2Fbusybox.git diff --git a/docs/style-guide.txt b/docs/style-guide.txt index 374a822b2..71eb62914 100644 --- a/docs/style-guide.txt +++ b/docs/style-guide.txt @@ -26,7 +26,9 @@ Here is the order in which code should be laid out in a file: - commented author name and email address(es) - commented GPL boilerplate - commented longer description / notes for the program (if needed) - - #includes and #defines + - #includes of .h files with angle brackets (<>) around them + - #includes of .h files with quotes ("") around them + - #defines (if any, note the section below titled "Avoid the Preprocessor") - const and global variables - function declarations (if necessary) - function implementations @@ -107,27 +109,39 @@ between it and the opening control block statement. Examples: Don't do this either: while (!done){ + do{ And for heaven's sake, don't do this: while (!done) { + do { Do this instead: while (!done) { + do { +Exceptions: -Paren Spacing -~~~~~~~~~~~~~ + - if you have long logic statements that need to be wrapped, then uncuddling + the bracket to improve readability is allowed: + + if (some_really_long_checks && some_other_really_long_checks \ + && some_more_really_long_checks) + { + do_foo_now; + +Spacing around Parentheses +~~~~~~~~~~~~~~~~~~~~~~~~~~ -Put a space between C keywords and left parens, but not between -function names and the left paren that starts it's parameter list (whether it -is being declared or called). Examples: +Put a space between C keywords and left parens, but not between function names +and the left paren that starts it's parameter list (whether it is being +declared or called). Examples: Don't do this: @@ -145,6 +159,19 @@ is being declared or called). Examples: ... baz = my_func(1, 2); +Also, don't put a space between the left paren and the first term, nor between +the last arg and the right paren. + + Don't do this: + + if ( x < 1 ) + strcmp( thisstr, thatstr ) + + Do this instead: + + if (x < 1) + strcmp(thisstr, thatstr) + Cuddled Elses ~~~~~~~~~~~~~ @@ -182,7 +209,6 @@ block. Example: - Variable and Function Names --------------------------- @@ -195,28 +221,66 @@ because it looks like whitespace; using lower-case is easy on the eyes. hitList TotalChars - szFileName (blech) + szFileName + pf_Nfol_TriState Preferred: hit_list total_chars file_name + sensible_name + +Exceptions: + + - Enums, macros, and constant variables are occasionally written in all + upper-case with words optionally seperatedy by underscores (i.e. FIFOTYPE, + ISBLKDEV()). + + - Nobody is going to get mad at you for using 'pvar' as the name of a + variable that is a pointer to 'var'. + + +Converting to K&R +~~~~~~~~~~~~~~~~~ + +The Busybox codebase is very much a mixture of code gathered from a variety of +sources. This explains why the current codebase contains such a hodge-podge of +different naming styles (Java, Pascal, K&R, just-plain-weird, etc.). The K&R +guideline explained above should therefore be used on new files that are added +to the repository. Furthermore, the maintainer of an existing file that uses +alternate naming conventions should, at his own convenience, convert those +names over to K&R style. Converting variable names is a very low priority +task. + +If you want to do a search-and-replace of a single variable name in different +files, you can do the following in the busybox directory: -The exception to this rule are enums, macros, and constant variables which -should all be in upper-case, with words optionally seperatedy by underscores -(i.e. FIFOTYPE, ISBLKDEV()). + $ perl -pi -e 's/\bOldVar\b/new_var/g' *.[ch] -Note: The Busybox codebase is very much a mixture of code gathered from a -variety of sources. This explains why the current codebase contains such a -hodge-podge of different naming styles (Java, Pascal, K&R, just-plain-weird, -etc.). The K&R guideline explained above should therefore be used on new files -that are added to the repository. Furthermore, the maintainer of an existing -file that uses alternate naming conventions should -- at his own convenience --- convert those names over to K&R style; converting variable names is a very -low priority task. Perhaps in the future we will include some magical Perl -script that can go through and convert variable names, left as an exercise for -the reader for now. +If you want to convert all the non-K&R vars in your file all at once, follow +these steps: + + - In the busybox directory type 'examples/mk2knr.pl files-to-convert'. This + does not do the actual conversion, rather, it generates a script called + 'convertme.pl' that shows what will be converted, giving you a chance to + review the changes beforehand. + + - Review the 'convertme.pl' script that gets generated in the busybox + directory and remove / edit any of the substitutions in there. Please + especially check for false positives (strings that should not be + converted). + + - Type './convertme.pl same-files-as-before' to perform the actual + conversion. + + - Compile and see if everything still works. + +Please be aware of changes that have cascading effects into other files. For +example, if you're changing the name of something in, say utility.c, you +should probably run 'examples/mk2knr.pl utility.c' at first, but when you run +the 'convertme.pl' script you should run it on _all_ files like so: +'./convertme.pl *.[ch]'. @@ -238,13 +302,13 @@ Use 'const var' for declaring constants. #define var 80 Do this instead, when the variable is in a header file and will be used in - several source files: + several source files: - const int var = 80; + const int var = 80; Or do this when the variable is used only in a single source file: - static const int var = 80; + static const int var = 80; Declaring variables as '[static] const' gives variables an actual type and makes the compiler do type checking for you; the preprocessor does _no_ type @@ -270,24 +334,25 @@ Use 'static inline' instead of a macro. } Static inline functions are greatly preferred over macros. They provide type -safety, have no length limitations, no formatting limitations, and under gcc -they are as cheap as macros. Besides, really long macros with backslashes at -the end of each line are ugly as sin. +safety, have no length limitations, no formatting limitations, have an actual +return value, and under gcc they are as cheap as macros. Besides, really long +macros with backslashes at the end of each line are ugly as sin. The Folly of #ifdef ~~~~~~~~~~~~~~~~~~~ Code cluttered with ifdefs is difficult to read and maintain. Don't do it. -Instead, put your ifdefs in a header, and conditionally define 'static inline' -functions, (or *maybe* macros), which are used in the code. +Instead, put your ifdefs at the top of your .c file (or in a header), and +conditionally define 'static inline' functions, (or *maybe* macros), which are +used in the code. Don't do this: ret = my_func(bar, baz); if (!ret) return -1; - #ifdef BB_FEATURE_FUNKY + #ifdef CONFIG_FEATURE_FUNKY maybe_do_funky_stuff(bar, baz); #endif @@ -295,7 +360,12 @@ functions, (or *maybe* macros), which are used in the code. (in .h header file) - #ifndef BB_FEATURE_FUNKY + #ifdef CONFIG_FEATURE_FUNKY + static inline void maybe_do_funky_stuff (int bar, int baz) + { + /* lotsa code in here */ + } + #else static inline void maybe_do_funky_stuff (int bar, int baz) {} #endif @@ -307,7 +377,7 @@ functions, (or *maybe* macros), which are used in the code. maybe_do_funky_stuff(bar, baz); The great thing about this approach is that the compiler will optimize away -the "no-op" case when the feature is turned off. +the "no-op" case (the empty function) when the feature is turned off. Note also the use of the word 'maybe' in the function name to indicate conditional execution. @@ -355,6 +425,82 @@ perfect world, we would have a streq() function in the string library, but that ain't the world we're living in. +Avoid Dangerous String Functions +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Unfortunately, the way C handles strings makes them prone to overruns when +certain library functions are (mis)used. The following table offers a summary +of some of the more notorious troublemakers: + +function overflows preferred +---------------------------------------- +strcpy dest string strncpy +strcat dest string strncat +gets string it gets fgets +getwd buf string getcwd +[v]sprintf str buffer [v]snprintf +realpath path buffer use with pathconf +[vf]scanf its arguments just avoid it + + +The above is by no means a complete list. Be careful out there. + + + +Avoid Big Static Buffers +------------------------ + +First, some background to put this discussion in context: Static buffers look +like this in code: + + /* in a .c file outside any functions */ + static char buffer[BUFSIZ]; /* happily used by any function in this file, + but ick! big! */ + +The problem with these is that any time any busybox app is run, you pay a +memory penalty for this buffer, even if the applet that uses said buffer is +not run. This can be fixed, thusly: + + static char *buffer; + ... + other_func() + { + strcpy(buffer, lotsa_chars); /* happily uses global *buffer */ + ... + foo_main() + { + buffer = xmalloc(sizeof(char)*BUFSIZ); + ... + +However, this approach trades bss segment for text segment. Rather than +mallocing the buffers (and thus growing the text size), buffers can be +declared on the stack in the *_main() function and made available globally by +assigning them to a global pointer thusly: + + static char *pbuffer; + ... + other_func() + { + strcpy(pbuffer, lotsa_chars); /* happily uses global *pbuffer */ + ... + foo_main() + { + char *buffer[BUFSIZ]; /* declared locally, on stack */ + pbuffer = buffer; /* but available globally */ + ... + +This last approach has some advantages (low code size, space not used until +it's needed), but can be a problem in some low resource machines that have +very limited stack space (e.g., uCLinux). + +A macro is declared in busybox.h that implements compile-time selection +between xmalloc() and stack creation, so you can code the line in question as + + RESERVE_CONFIG_BUFFER(buffer, BUFSIZ); + +and the right thing will happen, based on your configuration. + + Miscellaneous Coding Guidelines ------------------------------- @@ -370,7 +516,8 @@ When in doubt about the proper behavior of a Busybox program (output, formatting, options, etc.), model it after the equivalent GNU program. Doesn't matter how that program behaves on some other flavor of *NIX; doesn't matter what the POSIX standard says or doesn't say, just model Busybox -programs after their GNU counterparts and nobody has to get hurt. +programs after their GNU counterparts and it will make life easier on (nearly) +everyone. The only time we deviate from emulating the GNU behavior is when: @@ -475,15 +622,13 @@ one comment) before the block, rather than commenting each and every line. There is an optimal ammount of commenting that a program can have; you can comment too much as well as too little. -A picture is really worth a thousand words here, so here is an example that -illustrates emphasizing logical blocks: +A picture is really worth a thousand words here, the following example +illustrates how to emphasize logical blocks: while (line = get_line_from_file(fp)) { /* eat the newline, if any */ - if (line[strlen(line)-1] == '\n') { - line[strlen(line)-1] = '\0'; - } + chomp(line); /* ignore blank lines */ if (strlen(file_to_act_on) == 0) { @@ -499,3 +644,46 @@ illustrates emphasizing logical blocks: /* clean up */ free(line); } + + +Processing Options with getopt +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +If your applet needs to process command-line switches, please use getopt() to +do so. Numerous examples can be seen in many of the existing applets, but +basically it boils down to two things: at the top of the .c file, have this +line in the midst of your #includes: + + #include + +And a code block similar to the following near the top of your applet_main() +routine: + + while ((opt = getopt(argc, argv, "abc")) > 0) { + switch (opt) { + case 'a': + do_a_opt = 1; + break; + case 'b': + do_b_opt = 1; + break; + case 'c': + do_c_opt = 1; + break; + default: + show_usage(); /* in utility.c */ + } + } + +If your applet takes no options (such as 'init'), there should be a line +somewhere in the file reads: + + /* no options, no getopt */ + +That way, when people go grepping to see which applets need to be converted to +use getopt, they won't get false positives. + +Additional Note: Do not use the getopt_long library function and do not try to +hand-roll your own long option parsing. Busybox applets should only support +short options. Explanations and examples of the short options should be +documented in usage.h.