From: Jeff Lucovsky Date: Mon, 30 Mar 2020 15:01:59 +0000 (-0400) Subject: doc/devguide: Submission and style X-Git-Tag: suricata-6.0.0-beta1~561 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=refs%2Fpull%2F4777%2Fhead;p=thirdparty%2Fsuricata.git doc/devguide: Submission and style This commit adds code submission and coding style guidelines to the devguide. Most of the material is a straight port from the wiki but there have been some content modifications and additions. --- diff --git a/doc/devguide/code-style.rst b/doc/devguide/code-style.rst index bff8915cf8..0c55cc99d8 100644 --- a/doc/devguide/code-style.rst +++ b/doc/devguide/code-style.rst @@ -1,2 +1,334 @@ -Code Style -========== +Coding Style +============ + +Suricata uses a fairly strict coding style. This document describes it. + +Formatting +~~~~~~~~~~ + +Line length +^^^^^^^^^^^ + +There is a soft limit of ~80 characters. + +When wrapping lines that are too long, they should be indented at least 8 spaces from previous line. You should attempt to wrap the minimal portion of the line to meet the 80 character limit. + +Indent +^^^^^^ + +We use 4 space indentation. + +.. code-block:: c + + int DecodeEthernet(ThreadVars *tv, DecodeThreadVars *dtv, Packet *p, + uint8_t *pkt, uint16_t len, PacketQueue *pq) + { + SCPerfCounterIncr(dtv->counter_eth, tv->sc_perf_pca); + + if (unlikely(len < ETHERNET_HEADER_LEN)) { + ENGINE_SET_INVALID_EVENT(p, ETHERNET_PKT_TOO_SMALL); + return TM_ECODE_FAILED; + } + +Braces +^^^^^^ + +Functions should have the opening brace on a newline: + +.. code-block:: c + + int SomeFunction(void) + { + DoSomething(); + } + + +Control statements should have the opening brace on the same line: + +.. code-block:: c + + if (unlikely(len < ETHERNET_HEADER_LEN)) { + ENGINE_SET_INVALID_EVENT(p, ETHERNET_PKT_TOO_SMALL); + return TM_ECODE_FAILED; + } + +Opening and closing braces go on the same line as as the _else_ (also known as a "cuddled else"). + +.. code-block:: c + + if (this) { + DoThis(); + } else { + DoThat(); + } + +Flow +~~~~ + +Don't use conditions and statements on the same line. E.g. + +.. code-block:: c + + if (a) b = a; // <- wrong + + if (a) + b = a; // <- right + +Don't use unnecessary branching. E.g.: + +.. code-block:: c + + if (error) { + goto error; + } else { + a = b; + } + + +Can be written as: + +.. code-block:: c + + if (error) { + goto error; + } + a = b; + +Functions +~~~~~~~~~ + +parameter names +^^^^^^^^^^^^^^^ + +TODO + +Function names +^^^^^^^^^^^^^^ + +Function names are NamedLikeThis(). + +.. code-block:: c + + static ConfNode *ConfGetNodeOrCreate(char *name, int final) + +static vs non-static +^^^^^^^^^^^^^^^^^^^^ + +Functions should be declared static whenever possible. + +inline +^^^^^^ + +The inlining of functions should be used only in critical paths. + +curly braces / brackets +^^^^^^^^^^^^^^^^^^^^^^^ + +Functions should have the opening bracket on a newline: + +.. code-block:: c + + int SomeFunction(void) + { + DoSomething(); + } + +Note: this is a fairly new requirement, so you'll encounter a lot of non-compliant code. + +Variables +~~~~~~~~~ + +Names +^^^^^ + +A variable is ``named_like_this`` in all lowercase. + +.. code-block:: c + + ConfNode *parent_node = root; + +Generally, use descriptive variable names. + +In loop vars, make sure ``i`` is a signed int type. + +Scope +^^^^^ + +TODO + +Macros +~~~~~~ + +TODO + +Comments +~~~~~~~~ + +TODO + +Function comments +^^^^^^^^^^^^^^^^^ + +We use Doxygen, functions are documented using Doxygen notation: + +.. code-block:: c + + /** + * \brief Helper function to get a node, creating it if it does not + * exist. + * + * This function exits on memory failure as creating configuration + * nodes is usually part of application initialization. + * + * \param name The name of the configuration node to get. + * \param final Flag to set created nodes as final or not. + * + * \retval The existing configuration node if it exists, or a newly + * created node for the provided name. On error, NULL will be returned. + */ + static ConfNode *ConfGetNodeOrCreate(char *name, int final) + +General comments +^^^^^^^^^^^^^^^^ + +We use ``/* foobar */`` style and try to avoid ``//`` style. + +File names +~~~~~~~~~~ + +File names are all lowercase and have a .c. .h or .rs (Rust) extension. + +Most files have a _subsystem_ prefix, e.g. ``detect-dsize.c, util-ip.c`` + +Some cases have a multi-layer prefix, e.g. ``util-mpm-ac.c`` + +Enums +~~~~~ + +TODO + +Structures and typedefs +~~~~~~~~~~~~~~~~~~~~~~~ + +TODO + +switch statements +~~~~~~~~~~~~~~~~~ + +Switch statements are indented like in the following example, so the 'case' is indented from the switch: + +.. code-block:: c + + switch (ntohs(p->ethh->eth_type)) { + case ETHERNET_TYPE_IP: + DecodeIPV4(tv, dtv, p, pkt + ETHERNET_HEADER_LEN, + len - ETHERNET_HEADER_LEN, pq); + break; + +Fall through cases will be commented with ``/* fall through */``. E.g.: + +.. code-block:: c + + switch (suri->run_mode) { + case RUNMODE_PCAP_DEV: + case RUNMODE_AFP_DEV: + case RUNMODE_PFRING: + /* find payload for interface and use it */ + default_packet_size = GetIfaceMaxPacketSize(suri->pcap_dev); + if (default_packet_size) + break; + /* fall through */ + default: + default_packet_size = DEFAULT_PACKET_SIZE; + +const +~~~~~ + +TODO + +goto +~~~~ + +Goto statements should be used with care. Generally, we use it primarily for error handling. E.g.: + +.. code-block:: c + + static DetectFileextData *DetectFileextParse (char *str) + { + DetectFileextData *fileext = NULL; + + fileext = SCMalloc(sizeof(DetectFileextData)); + if (unlikely(fileext == NULL)) + goto error; + + memset(fileext, 0x00, sizeof(DetectFileextData)); + + if (DetectContentDataParse("fileext", str, &fileext->ext, &fileext->len, &fileext->flags) == -1) { + goto error; + } + + return fileext; + + error: + if (fileext != NULL) + DetectFileextFree(fileext); + return NULL; + } + +Unittests +~~~~~~~~~ + +When writing unittests that use when using a data array containing a protocol message, please put an explanatory comment that contain the readable content of the message + +So instead of: + +.. code-block:: c + + int SMTPProcessDataChunkTest02(void) + { + char mimemsg[] = {0x4D, 0x49, 0x4D, 0x45, 0x2D, 0x56, 0x65, 0x72, + +you should have something like: + +.. code-block:: c + + int SMTPParserTest14(void) + { + /* 220 mx.google.com ESMTP d15sm986283wfl.6 */ + static uint8_t welcome_reply[] = { 0x32, 0x32, 0x30, 0x20, + +Banned functions +~~~~~~~~~~~~~~~~ + ++------------+---------------+-----------+ +| function | replacement | reason | ++============+===============+===========+ +| strok | strtok_r | | ++------------+---------------+-----------+ +| sprintf | snprintf | unsafe | ++------------+---------------+-----------+ +| strcat | strlcat | unsafe | ++------------+---------------+-----------+ +| strcpy | strlcpy | unsafe | ++------------+---------------+-----------+ +| strncpy | strlcat | | ++------------+---------------+-----------+ +| strncat | strlcpy | | ++------------+---------------+-----------+ +| strndup | |OS specific| ++------------+---------------+-----------+ +| strchrnul | | | ++------------+---------------+-----------+ +| rand | | | ++------------+---------------+-----------+ +| rand_r | | | ++------------+---------------+-----------+ +| index | | | ++------------+---------------+-----------+ +| rindex | | | ++------------+---------------+-----------+ +| bzero | memset | | ++------------+---------------+-----------+ + +Also, check the existing code. If yours is wildly different, it's wrong. +Example: https://github.com/oisf/suricata/blob/master/src/decode-ethernet.c diff --git a/doc/devguide/code-submission-process.rst b/doc/devguide/code-submission-process.rst index 9b673639db..8a7a34a34a 100644 --- a/doc/devguide/code-submission-process.rst +++ b/doc/devguide/code-submission-process.rst @@ -1,2 +1,56 @@ Code Submission Process ======================= + +Commits +~~~~~~~ + +#. Commits need to be logically separated. Don't fix unrelated things in one commit. +#. Don't add unnecessary commits, if commit 2 fixes commit 1 merge them together (squash) +#. Commits need to have proper messages, explaining anything that is non-trivial +#. Commits should not at the same time change, rename and/or move code. Use separate commits + for each of this, e.g, a commit to rename files, then a commit to change the code. +#. Documentation updates should be in their own commit (not mixed with code commits) +#. Commit messages need to be properly formatted: + * Meaningful and short (50 chars max) subject line followed by an empty line + * Naming convention: prefix message with sub-system ("rule parsing: fixing foobar"). If + you're not sure what to use, look at past commits to the file(s) in your PR. + * Description, wrapped at ~72 characters +#. Commits should be individually compilable, starting with the oldest commit. Make sure that + each commit can be built if it and the preceding commits in the PR are used. + +Information that needs to be part of a commit (if applicable): + +#. Ticket it fixes. E.g. "Fixes Bug #123." +#. Compiler warnings addressed. +#. Coverity Scan issues addressed. +#. Static analyzer error it fixes (cppcheck/scan-build/etc) + +Pull Requests +~~~~~~~~~~~~~ + +A github pull request is actually just a pointer to a branch in your tree. Github provides a review interface that we use. + +#. A branch can only be used in for an individual PR. +#. A branch should not be updated after the pull request +#. A pull request always needs a good description (link to issue tracker if related to a ticket). +#. Incremental pull requests need to link to the prior iteration +#. Incremental pull requests need to describe changes since the last PR +#. Link to the ticket(s) that are addressed to it. +#. When fixing an issue, update the issue status to ``In Review`` after submitting the PR. +#. Links to the prscript builds* +#. Pull requests are automatically tested using github actions (https://github.com/OISF/suricata/blob/master/.github/workflows/builds.yml). + Failing builds won't be considered and should be closed immediately. +#. Pull requests that change, or add a feature should include a documentation update commit + + (*) access to prscript is limited to trusted devs. For the rest of you, ask Victor, Eric, Andreas or Jason to run it for you. + +Tests and QA +~~~~~~~~~~~~ + +As much as possible, new functionality should be easy to QA. + +#. Add ``suricata-verify`` tests for verification. See https://github.com/OISF/suricata-verify +#. Add unittests if a ``suricata-verify`` test isn't possible. +#. Provide pcaps that reproduce the problem. Try to trim as much as possible to the pcap includes the minimal + set of packets that demonstrate the problem. +#. Provide example rules if the code added new keywords or new options to existing keywords