Notes on Coding Standards/Requirements for OpenAFS Source ========================================================= This document covers the rules for coding style, lists the dependencies needed to build from source, covers how we use git and gerrit for development and code review, gives guidelines for what we expect code review to cover, and discusses how we handle compiler warnings. Prototyping and Style ===================== We have an official style for C code. Please use it. If you have GNU indent 2.2.9 or later you can format new files for this style with the following options: -npro -nbad -bap -nbc -bbo -br -ce -cdw -brs -ncdb -cp1 -ncs -di2 -ndj -nfc1 -nfca -i4 -lp -npcs -nprs -psl -sc -nsob -ts8 In prose, the indent options correspond to: * (-npro) Do not read .indent.pro files * (-nbad) Do not require a blank line after function declarations * (-bap) Require a blank line after each procedure's body * (-nbc) Do not require a newline after commas in function declarations * (-bbo) Prefer breaking long lines before boolean operators && and || * (-br) Format braces with the opening brace on the same line as the condition * (-ce) Cuddle 'else' keywords on the same line as the preceding '}' * (-cdw) Cuddle 'while' (of `do {} while ()`) keywords with preceding '}' * (-brs) Put the opening '{' on the same line as the 'struct' keyword * (-ncbd) Do not require comment delimiters ('/*' and '*/') to always be on their own lines * (-cp1) Put comments after preprocessor directives at the first tab stop after the directive * (-ncs) Do not use a space after a cast operator * (-di2) Place variable declarations immediately after (with one space separator) the type statement * (-ndj) For comments after declarations, do not left justify them behind the declarations * (-nfc1) Do not format comments in the first column as normal (i.e., allow them in contexts where comments would otherwise be indented) * (-nfca) Do not format any comments (redundant with the former?) * (-i4) Indentation is four spaces * (-lp) Line up parentheses (on subsequent lines) * (-npcs) Do not put a space after the function name in function calls * (-nprs) Do not put a space after every '(' and before every ')' * (-psl) Put the return type of a function on its own line * (-sc) Use a '*' character at the left side of multiline comments * (-nsob) Do not allow optional blank lines * (-ts8) The tab stop is 8 columns Since historic compliance with the style has been poor, caution is needed when operating on existing files. It is often suitable to do an initial style cleanup commit before making sweeping changes to a given file, and otherwise try to accommodate the prevailing style in the file when making minor changes. The style for non-C code varies. Makefiles must use literal tabs to indent rule bodies, but use spaces for any additional indentation needed. Plain text files (such as this one) should use spaces for indentation, with a 4-space indent as the default when one is needed. Other cases will be added here as they are encountered and a consensus determined for how to handle them. Prototypes for all source files in a given dir DDD should be placed in the file DDD/DDD_prototypes.h. All externally used (either API or used by other source files) routines and variables should be prototyped in this file. The prototypes should be a full prototype, with argument and return types. (That is, they should not generate a warning with gcc -Wstrict-prototypes.) Format of the prototype files should look like: Standard Copyright Notice #ifndef AFS_SRC_DDD_PROTO_H #define AFS_SRC_DDD_PROTO_H /* filename.c */ prototypes /* filename.c */ prototypes #endif /* AFS_SRC_DDD_PROTO_H */ In most of the existing prototypes, the define is DDD_PROTOTYPES_H, which is probably ok as well. The declaration of the routines should be done in ANSI style. If at some later date, it is determined that prototypes don't work on some platform properly, we can use ansi2knr during the compile. rettype routine(argtype arg) { } All routines should have a return type specified, void if nothing returned, and should have (void) if no arguments are taken. Header files should not contain macros or other definitions unless they are used across multiple source files. All routines should be declared static if they are not used outside that source file. Compiles on gcc-using machines should strive to handle using -Wstrict-prototypes -Werror (this may take a while). Routines shall be defined in source prior to use if possible, and prototyped in block at top of file if static. API documentation in the code should be done using Qt-style Doxygen comments. If you make a routine or variable static, be sure and remove it from the AIX .exp files. It's recommended to configure with --enable-checking to activate the compiler warning flags that the codebase complies with. Multiple line comment blocks should begin with only /* on one line and end with only */ on one line. Example: /* * Multiple line comment blocks should be formatted * like this example. */ Always use braces for the bodies of conditionals and loops. This makes it easier to follow the logic flow for complicated nested constructs, and reduces the risk of bugs. Use: if (some_condition) { do_some_action(); } else { do_something_else(); } while (some_condition) { do_something(); } Instead of: if (some_condition) do_some_action(); while (some_condition) do_something(); In switch statements, to fall through from one case statement to another, use AFS_FALLTHROUGH to mark the intentional fall through. Do not use fall through comments (e.g. /* fallthrough */), as some compilers do not recognize them and will flag the case statement with an implied fallthrough warning. Use: switch (x) { case 1: do_something(); AFS_FALLTHROUGH; case 2: do_something_else(); AFS_FALLTHROUGH; default: do_some_action(); } Instead of using fallthrough comments: switch (x) { case 1: do_something(); /* fallthrough */ case 2: do_something_else(); /* fallthrough */ default: do_some_action(); } Or not marking the fall through: switch (x) { case 1: do_something(); case 2: do_something_else(); default: do_some_action(); } Dependencies required to build OpenAFS from source ================================================== The following packages are required to build all of the OpenAFS code from source on various operating systems: On Debian: - autoconf, automake, bison, comerr-dev, cpio, flex, libkrb5-dev, libncurses5-dev, libpam0g-dev, libxml2-utils, perl, pkg-config; - libfuse-dev (for the FUSE-based user-mode client); - dblatex, docbook-xsl, doxygen, xsltproc (for documentation); - debhelper, dkms, dh-dkms (to build the Debian packages) On FreeBSD: - autoconf, automake, libtool; - fusefs-libs, pkgconf (for the FUSE-based user-mode client); - perl, dblatex, docbook-xsl, libxslt, python, ruby, zip (for documentation) In addition, FreeBSD systems require kernel sources and a configured kernel build directory (see section "FreeBSD Notes" in the README file). Build System ============ Do not use $< for non-pattern rules in any cross-platform dir as it requires a reasonable make that is not available on all systems. Do not have build rules that build multiple targets. Make doesn't seem able to handle this, and it interferes with -j builds. (In particular, build the rxgen targets individually and not using the flags for building all the files in one shot.) Try to test builds using gmake -j # MAKE="gmake -j #", it seems like a good way to find missing or order-dependent dependency rules. (Is there a better way to do this?) GIT Usage ========= With the broad continuous integration coverage provided by buildbot and the current code review policy for changes entering master, it's generally expected that the development branch will be buildable and functional at any given time. Git checkouts do not include files generated by autoconf. You can run regen.sh (at the top level) to create these files. You will need to have autoconf and automake installed on your system. Summary ------- Browse: https://git.openafs.org/ Clone: git clone git://git.openafs.org/openafs.git Step-by-step ------------ 1. Obtain the Git software. If you are using a system with a standard software repository, Git may already be available as a package named something like git or git-core. Otherwise, go to http://git-scm.com/ 2. Run the command: % git clone git://git.openafs.org/openafs.git This will download the full repository and leave a checked-out tree in a subdirectory of the current directory named openafs. The repository itself is in the .git subdirectory of that directory. WARNING: The repository is approximately 86MiB currently and will only grow, so it may take some time to download the first time over a slow network connection. 3. Generate the additional required files: % cd openafs % ./regen.sh The current development series is in the branch named master. The stable releases are on separate branches named something like openafs-stable_ with a separate branch for each major stable release series. Use git branch -a to see a full list of branches. OpenAFS uses the Gerrit code review system to review and merge all changes to OpenAFS. More details are at: http://wiki.openafs.org/GitDevelopers/ including more detailed Git instructions. It's by far preferred to use Gerrit to submit code changes, but if you can't for whatever reason, you can instead open a bug and submit a patch that way. Do this by sending mail to openafs-bugs@openafs.org with the patch attached. But please use Gerrit if you can; patches sent in as bugs will have to be forwarded to Gerrit by someone else, and it's easier for everyone if you can enter them into Gerrit yourself. Backport policy --------------- All patches should land on master first, unless the patch fixes a bug that only exists in the stable branch. Once a patch has been accepted into master, anyone can propose backports to stable branches. It is preferred to check with the Stable Release Manager before doing so, though, as that will give an opportunity to check where in the stack of pending changes any new addition should go. When cherry-picking a commit from another branch, please append a "cherry picked from" section in your commit message. You'll also need a separate Change-ID for Gerrit to recognize this as a separate change. One workflow to do this: 1) Use "git cherry-pick -ex" to pick your commits onto another branch. The -x option will append the appropriate "cherry picked from" message, and the -e option will open your editor for you to edit the commit message. 2) In your editor, delete the existing Change-ID line. Save and quit. 3) Run "git commit --amend", saving and quitting again. Git will run the commit hook and generate a new Change-ID for Gerrit. Code Review Guidelines ====================== OpenAFS's use of Gerrit imposes a requirement that all changes undergo code review prior to being merged to any branch, whether it be master or a stable release branch. The following guidelines discuss what the scope of that review is expected to cover. All of the points need to be addressed by some reviewer, though not necessarily by the same reviewer. When doing a review that only addresses some of these topics, indicate in the review comment which portions were/were not done. What does the change claim to do? --------------------------------- This is often taken for granted, but in order to review a change we have to know what it is trying to do (so that we can tell if it successfully does so). In our Gerrit-based workflow, this should be in the commit message itself, but a patch attached to a bug or email may not have that additional context. If the change is fixing a bug, the bug report can be expected to give some description of the issue being fixed. In addition to the sense of "what" is expected to change, a reviewer also expects to know the "why" behind the change. This becomes important when assessing if this change is the right thing to do (see below). At this part of the review, it's enough to ensure that the what and why are documented and seem plausible. Does the change do what it claims to do? ---------------------------------------- With the newfound knowledge of the goal of the change, it becomes possible to assess whether it achieves that goal. Follow through the "normal" or "success" path of the new/changed code and scrutinize what it _actually_ does. As you do this, strive to discern differences between the actual behavior and the intended behavior -- does it live up to what was advertised? If the actual changes have more things than the stated goal, that may be a sign that there are unrelated changes included -- it is often best to ask to split these into a separate commit (but if not, they should be acknowledged in the commit message as intended inclusions). It is also okay to ask for mechanical or similarly repetitive changes to be broken out into dedicated commits (so that the reviewer would just independently reproduce the mechanical change and compare the resulting tree). Having looked at the normal path, it's then time to look at the not-normal path. Error cases, edge cases, look for these wherever you can and see if the code is going to do the right thing in those scenarios. Try to spot any copy/paste issues in this phase as well -- if we are doing conceptually the same thing in more than one instance, check that each instance is actually touching the fields appropriate for that instance. Sometimes the difference is just a single character, and it's easy to miss (sometimes the copy/paste has been cleaned up once on a given line but needed to be cleaned up twice). Is there a cleanup handler that should be jumped to? Locks or other state that need to be released? It is generally reasonable to assume that the adjacent error cases/early returns are correct, and only examine what has changed between that call site and the one you're looking at. Check for edge cases -- if there are conditionals against a specific constant, verify the expected behavior when the input is that value/length, as well as just above and just below. This holds especially for string lengths (e.g., in PRNAMEs the length bound need not account for a trailing NUL) but checking for edge cases is always important. Does the change not do something it should be doing? ---------------------------------------------------- If the goal of the change is to be comprehensive about doing something (within a given scope), check that it has actually changed all the places that need to change. It can take a bit of creativity to be able to write an efficient check for this -- sometimes just searching on the API name is sufficient, but maybe there is a specific constant (for port number, AFS service number, a given data field's length, etc.) that would apply, This is probably going to require a lot of manual inspection; `${PAGER} $(git grep -l ${PATTERN})` followed by a search in each file to find the relevant instance(s) is a useful technique. Do NOT just rely on the compiler to detect incompatible/missing APIs -- we have code that is conditionally compiled based on architecture and environment, and we also want to ensure that we find relevant comments, tests, and documentation that might be exercising the interface in question. Check for comments/documentation for the affected areas of the code. Are there any comments that have become stale and need to be changed, but are not changed? Unless the goal of the change is to fix a bug and restore the documented behavior, if the behavior is changing, we should expect the documentation (if present) to be changing as well. (If there is no documentation, consider adding some.) Are there related functions or logic that should be getting a similar change to it? See above about conditionally compiled code -- sometimes we have multiple implementations of a given API depending on the environment or architecture, and often (but not always) when we need to change one, we need to change others. A network API may have multiple versions as it has evolved over time; learn about the intended differences between generations of the API to determine which one(s) should be getting a given change. If fixing a bug (especially when security-relevant), try to generalize or abstract the nature of the fix to see if there are other places where an analogous fix would apply. Returning uninitialized data to a network caller is the classic example here, but this can arise in many ways. Does the change not do what it claims to not do? ------------------------------------------------ If there is supposed to be no functional change, is that true? If a given class of behavior/logic is supposed to be unmodified, trace through the code and confirm that that's the case. Is that the right thing to do? ------------------------------ This question is a bit open-ended and sometimes hard to apply in practice, but it's usually worth taking a step back and thinking about what problem the change is trying to solve, whether it could be solved some other way, and whether solving a different problem would be more appropriate. For example, rather than trying to optimize an existing API at the cost of very convoluted logic, maybe there is a simpler API or different data structure to achieve the overall goal that has a different overall structure. Contrariwise, though, changing the behavior of a network API is pretty complicated to get right, and a local adjustment may be more worthwhile. Style and Standard Compliance ----------------------------- This is sometimes done in passing during an earlier stage of the review, but always make a point to consider whether the changes conform to the project's style guidelines. New code should conform to the project (or prevailing) style, and generally we should fix the style when making substantive changes to a given line. Often we will fix style of adjacent lines as well, if there are not too many, but it's a judgement call -- if the focus of the change becomes more about style than bugfixes, it's probably time for a separate cleanup commit. It's also important to think about whether the change conforms to the language specification; we officially target C89 plus a handful of extensions (though it may be simpler to reason about it as C99 minus a handful of features). The precise list of extensions over C89 that we use is a work in progress, but includes (and will be updated as encountered): - the long long int type - designated initializers - the __func__ predefined macro If we're using additional language extensions or more modern features, they need to be scoped to a file/situation where we can guarantee they are available. We must always try to avoid undefined behavior (though there are existing cases where we do potentially trigger it). Our network and userspace/kernel APIs need to conform to any relevant standards or specifications that apply. We cannot unilaterally change existing RPC-L (the .xg files processed by rxgen), and the scope for adding new things is limited. When we add new pioctls, they have to come from the OpenAFS-specific range (i.e., be 'O' pioctls). Com_err codes in .et files are generally managed by the central.org registrar, etc. Documentation ------------- New APIs should get documented via at least doxygen comments attached to their implementation. Changes to CLI commands require man page changes. New public APIs should also get man pages. The reviewer should check that these are being done. If the change is modifying the behavior of existing functionality that has documentation, update the documentation accordingly. (Don't forget the HISTORY section of man pages!) If there is not existing documentation, the code review is a good opportunity to ask if documentation should be added, but the answer to that question will vary on the specifics of the case in question. Test Coverage ------------- The current test coverage in the tree is very sparse. If adding to or changing the behavior of an area of the code that does have some test coverage, add tests for the new/changed behavior. If fixing a bug, add a regression test. It is a good idea to confirm that the (new) test fails with the code change reverted, though sometimes this is complicated enough to be an unreasonable burden on the reviewer. For areas of the tree that do not have existing code coverage, we should consider adding some. It's good for the reviewer to ask (and consider themself) what level of effort would be involved in adding test coverage for this part of the code and whether we should put that effort in at this time. The answer may be "not worth it right now", though. Warnings ======== OpenAFS Warning detection ------------------------- There's been a concerted effort over the last few years, by many developers, to reduce the number of warnings in the OpenAFS tree. In an attempt to prevent warnings from creeping back in, we now have the ability to break the build when new warnings appear. This is only available for systems with gcc 4.2 or later or clang 3.2 or later, and is disabled unless the --enable-checking option is supplied to configure. Because we can't remove all of the warnings, we permit file by file (and warning by warning) disabling of specific warnings. The --enable-checking=all option prevents this, and errors for any file containing a warning. (At present, using --enable-checking=all will error out quite early in the build, so this is only useful for developers attempting to clean up compiler warnings.) Disabling warnings ------------------ If warnings are unavoidable in a particular part of the build, they may be disabled in an number of ways. You can disable a single warning type in a particular file by using GCC pragmas. If a warning can be disabled with a pragma, then the switch to use will be listed in the error message you receive from the compiler. Pragmas should be wrapped in IGNORE_SOME_GCC_WARNINGS, so that they aren't used with non-gcc compilers, and can be disabled if desired. For example: #ifdef IGNORE_SOME_GCC_WARNINGS # pragma GCC diagnostic warning "-Wold-style-definition" #endif It would appear that when built with -Werror, the llvm clang compiler will still upgrade warnings that are suppresed in this way to errors. In this case, the fix is to mark that warning as ignored, but only for clang. For example: #ifdef IGNORE_SOME_GCC_WARNINGS # ifdef __clang__ # pragma GCC diagnostic ignored "-Wdeprecated-declarations" # else # pragma GCC diagnostic warning "-Wdeprecated-declarations" # endif #endif If the source cannot be changed to add a pragma, you might be able to use the autoconf function AX_APPEND_COMPILE_FLAGS to create a new macro that disables the warning and then use macro for the build options for that file. For an example, see how the autoconf macro CFLAGS_NOIMPLICIT_FALLTHROUGH is defined and used. Finally, if there isn't a way to disable the specific warning, you will need to disable all warnings for the file in question. You can do this by supplying the autoconf macro @CFLAGS_NOERROR@ in the build options for the file. For example: lex.yy.o : lex.yy.c y.tab.c ${CC} -c ${CFLAGS} @CFLAGS_NOERROR@ lex.yy.c If you add a new warning inhibition, also add it to the list below. Inhibited warnings ------------------ uss/lex.i : fallthrough : clang fallthrough, flex generated code comerr/et_lex.lex.l : fallthrough : clang fallthrough, flex generated code pragma set to ignored where included in error_table.y afs/afs_syscall.c : old-style : strict-proto : all (ukernel) : syscall pointer issues afsd/afsd_kernel.c : deprecated : daemon() marked as deprecated on Darwin bozo/bosserver.c : deprecated : daemon() marked as deprecated on Darwin bucoord/ubik_db_if.c : strict-proto : ubik_Call_SingleServer bucoord/commands.c : all : signed vs unsigned for dates external/heimdal/hcrypto/validate.c : all : statement with empty body external/heimdal/hcrypto/evp.c : cast-function-type : Linux kernel build uses -Wcast-function-type external/heimdal/hcrypto/evp-algs.c : cast-function-type : Linux kernel build uses -Wcast-function-type external/heimdal/krb5/crypto.c : use-after-free: False postive on certain GCC compilers kauth/admin_tools.c : strict-proto : ubik_Call kauth/authclient.c : strict-proto : ubik_Call nonsense libadmin/kas/afs_kasAdmin.c : strict-proto : ubik_Call nonsense libadmin/samples/rxstat_query_peer.c : all : util_RPCStatsStateGet types libadmin/samples/rxstat_query_process.c : all : util_RPCStatsStateGet types libadmin/test/client.c : all : util_RPCStatsStateGet types ubik/ubikclient.c : strict-protos : ubik_Call volser/vol-dump.c : format : afs_sfsize_t rxkad/ticket5.c : format-truncation : inside included file v5der.c in the function _heim_time2generalizedtime, the two snprintf calls raise format-truncation warnings due to the arithmetic on tm_year and tm_mon fields rxkad/ticket5.c : deprecated : hcrypto single-DES lwp/process.c : dangling-pointer : Ignore the legitimate use of saving the address of a stack variable src/rxkad/test/stress_c.c : deprecated : hcrypto single-DES auth/authcon.c : deprecated : hcrypto single-DES kauth/kaprocs.c : deprecated : hcrypto single-DES kauth/krb_udp.c : deprecated : hcrypto single-DES libafscp/afscp_util.c : deprecated : hcrypto single-DES tests/common/rxkad.c : deprecated : hcrypto single-DES