aiden
/
suricata
mirror of https://github.com/OISF/suricata
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

doc/devguide: Submission and style

Browse Source 
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.
pull/4777/head
Jeff Lucovsky 5 years ago committed by Victor Julien
parent 752e4828d7
commit 5f71e7a371
2 changed files with 388 additions and 2 deletions
Show Stats Download Patch File Download Diff File

336
doc/devguide/code-style.rst
Unescape Escape View File

@ -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<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

54
doc/devguide/code-submission-process.rst
Unescape Escape View File

@ -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

Write Preview
Loading…
Cancel
Save