-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<CR><LF> */
+ 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
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
projects / thirdparty / suricata.git / commitdiff
