1.. index:: 2 single: C 3 pair: C; guidelines 4 5C Coding Guidelines 6------------------- 7 8Pacemaker is a large project accepting contributions from developers with a 9wide range of skill levels and organizational affiliations, and maintained by 10multiple people over long periods of time. Following consistent guidelines 11makes reading, writing, and reviewing code easier, and helps avoid common 12mistakes. 13 14Some existing Pacemaker code does not follow these guidelines, for historical 15reasons and API backward compatibility, but new code should. 16 17 18Code Organization 19################# 20 21Pacemaker's C code is organized as follows: 22 23+-----------------+-----------------------------------------------------------+ 24| Directory | Contents | 25+=================+===========================================================+ 26| daemons | the Pacemaker daemons (pacemakerd, pacemaker-based, etc.) | 27+-----------------+-----------------------------------------------------------+ 28| include | header files for library APIs | 29+-----------------+-----------------------------------------------------------+ 30| lib | libraries | 31+-----------------+-----------------------------------------------------------+ 32| tools | command-line tools | 33+-----------------+-----------------------------------------------------------+ 34 35Source file names should be unique across the entire project, to allow for 36individual tracing via ``PCMK_trace_files``. 37 38 39.. index:: 40 single: C; library 41 single: C library 42 43Pacemaker Libraries 44################### 45 46+---------------+---------+---------------+---------------------------+-------------------------------------+ 47| Library | Symbol | Source | API Headers | Description | 48| | prefix | location | | | 49+===============+=========+===============+===========================+=====================================+ 50| libcib | cib | lib/cib | | include/crm/cib.h | .. index:: | 51| | | | | include/crm/cib/* | single: C library; libcib | 52| | | | | single: libcib | 53| | | | | | 54| | | | | API for pacemaker-based IPC and | 55| | | | | the CIB | 56+---------------+---------+---------------+---------------------------+-------------------------------------+ 57| libcrmcluster | pcmk | lib/cluster | | include/crm/cluster.h | .. index:: | 58| | | | | include/crm/cluster/* | single: C library; libcrmcluster | 59| | | | | single: libcrmcluster | 60| | | | | | 61| | | | | Abstract interface to underlying | 62| | | | | cluster layer | 63+---------------+---------+---------------+---------------------------+-------------------------------------+ 64| libcrmcommon | pcmk | lib/common | | include/crm/common/* | .. index:: | 65| | | | | some of include/crm/* | single: C library; libcrmcommon | 66| | | | | single: libcrmcommon | 67| | | | | | 68| | | | | Everything else | 69+---------------+---------+---------------+---------------------------+-------------------------------------+ 70| libcrmservice | svc | lib/services | | include/crm/services.h | .. index:: | 71| | | | | single: C library; libcrmservice | 72| | | | | single: libcrmservice | 73| | | | | | 74| | | | | Abstract interface to supported | 75| | | | | resource types (OCF, LSB, etc.) | 76+---------------+---------+---------------+---------------------------+-------------------------------------+ 77| liblrmd | lrmd | lib/lrmd | | include/crm/lrmd*.h | .. index:: | 78| | | | | single: C library; liblrmd | 79| | | | | single: liblrmd | 80| | | | | | 81| | | | | API for pacemaker-execd IPC | 82+---------------+---------+---------------+---------------------------+-------------------------------------+ 83| libpacemaker | pcmk | lib/pacemaker | | include/pacemaker*.h | .. index:: | 84| | | | | include/pcmki/* | single: C library; libpacemaker | 85| | | | | single: libpacemaker | 86| | | | | | 87| | | | | High-level APIs equivalent to | 88| | | | | command-line tool capabilities | 89| | | | | (and high-level internal APIs) | 90+---------------+---------+---------------+---------------------------+-------------------------------------+ 91| libpe_rules | pe | lib/pengine | | include/crm/pengine/* | .. index:: | 92| | | | | single: C library; libpe_rules | 93| | | | | single: libpe_rules | 94| | | | | | 95| | | | | Scheduler functionality related | 96| | | | | to evaluating rules | 97+---------------+---------+---------------+---------------------------+-------------------------------------+ 98| libpe_status | pe | lib/pengine | | include/crm/pengine/* | .. index:: | 99| | | | | single: C library; libpe_status | 100| | | | | single: libpe_status | 101| | | | | | 102| | | | | Low-level scheduler functionality | 103+---------------+---------+---------------+---------------------------+-------------------------------------+ 104| libstonithd | stonith | lib/fencing | | include/crm/stonith-ng.h| .. index:: | 105| | | | | include/crm/fencing/* | single: C library; libstonithd | 106| | | | | single: libstonithd | 107| | | | | | 108| | | | | API for pacemaker-fenced IPC | 109+---------------+---------+---------------+---------------------------+-------------------------------------+ 110 111 112Public versus Internal APIs 113___________________________ 114 115Pacemaker libraries have both internal and public APIs. Internal APIs are those 116used only within Pacemaker; public APIs are those offered (via header files and 117documentation) for external code to use. 118 119Generic functionality needed by Pacemaker itself, such as string processing or 120XML processing, should remain internal, while functions providing useful 121high-level access to Pacemaker capabilities should be public. When in doubt, 122keep APIs internal, because it's easier to expose a previously internal API 123than hide a previously public API. 124 125Internal APIs can be changed as needed. 126 127The public API/ABI should maintain a degree of stability so that external 128applications using it do not need to be rewritten or rebuilt frequently. Many 129OSes/distributions avoid breaking API/ABI compatibility within a major release, 130so if Pacemaker breaks compatibility, that significantly delays when OSes 131can package the new version. Therefore, changes to public APIs should be 132backward-compatible (as detailed throughout this chapter), unless we are doing 133a (rare) release where we specifically intend to break compatibility. 134 135External applications known to use Pacemaker's public C API include 136`sbd <https://github.com/ClusterLabs/sbd>`_ and dlm_controld. 137 138 139.. index:: 140 pair: C; API documentation 141 single: Doxygen 142 143API Documentation 144_________________ 145 146Pacemaker uses `Doxygen <https://www.doxygen.nl/manual/docblocks.html>`_ 147to automatically generate its 148`online API documentation <https://clusterlabs.org/pacemaker/doxygen/>`_, 149so all public API (header files, functions, structs, enums, etc.) should be 150documented with Doxygen comment blocks. Other code may be documented in the 151same way if desired, with an ``\internal`` tag in the Doxygen comment. 152 153Simple example of an internal function with a Doxygen comment block: 154 155.. code-block:: c 156 157 /*! 158 * \internal 159 * \brief Return string length plus 1 160 * 161 * Return the number of characters in a given string, plus one. 162 * 163 * \param[in] s A string (must not be NULL) 164 * 165 * \return The length of \p s plus 1. 166 */ 167 static int 168 f(const char *s) 169 { 170 return strlen(s) + 1; 171 } 172 173 174API Header File Naming 175______________________ 176 177* Internal API headers should be named ending in ``_internal.h``, in the same 178 location as public headers, with the exception of libpacemaker, which for 179 historical reasons keeps internal headers in ``include/pcmki/pcmki_*.h``). 180 181* If a library needs to share symbols just within the library, header files for 182 these should be named ending in ``_private.h`` and located in the library 183 source directory (not ``include``). Such functions should be declared as 184 ``G_GNUC_INTERNAL``, to aid compiler efficiency (glib defines this 185 symbol appropriately for the compiler). 186 187Header files that are not library API are located in the same locations as 188other source code. 189 190 191.. index:: 192 pair: C; naming 193 194API Symbol Naming 195_________________ 196 197Exposed API symbols (non-``static`` function names, ``struct`` and ``typedef`` 198names in header files, etc.) must begin with the prefix appropriate to the 199library (shown in the table at the beginning of this section). This reduces the 200chance of naming collisions when external software links against the library. 201 202The prefix is usually lowercase but may be all-caps for some defined constants 203and macros. 204 205Public API symbols should follow the library prefix with a single underbar 206(for example, ``pcmk_something``), and internal API symbols with a double 207underbar (for example, ``pcmk__other_thing``). 208 209File-local symbols (such as static functions) and non-library code do not 210require a prefix, though a unique prefix indicating an executable (controld, 211crm_mon, etc.) can be helpful to indicate symbols shared between multiple 212source files for the executable. 213 214 215.. index:: 216 pair: C; boilerplate 217 pair: license; C 218 pair: copyright; C 219 220C Boilerplate 221############# 222 223Every C file should start with a short copyright and license notice: 224 225.. code-block:: c 226 227 /* 228 * Copyright <YYYY[-YYYY]> the Pacemaker project contributors 229 * 230 * The version control history for this file may have further details. 231 * 232 * This source code is licensed under <LICENSE> WITHOUT ANY WARRANTY. 233 */ 234 235*<LICENSE>* should follow the policy set forth in the 236`COPYING <https://github.com/ClusterLabs/pacemaker/blob/master/COPYING>`_ file, 237generally one of "GNU General Public License version 2 or later (GPLv2+)" 238or "GNU Lesser General Public License version 2.1 or later (LGPLv2.1+)". 239 240Header files should additionally protect against multiple inclusion by defining 241a unique symbol of the form ``PCMK__<capitalized_header_name>__H``. For 242example: 243 244.. code-block:: c 245 246 #ifndef PCMK__MY_HEADER__H 247 # define PCMK__MY_HEADER__H 248 249 // header code here 250 251 #endif // PCMK__MY_HEADER__H 252 253Public API header files should additionally declare "C" compatibility for 254inclusion by C++, and give a Doxygen file description. For example: 255 256.. code-block:: c 257 258 #ifdef __cplusplus 259 extern "C" { 260 #endif 261 262 /*! 263 * \file 264 * \brief My brief description here 265 * \ingroup core 266 */ 267 268 // header code here 269 270 #ifdef __cplusplus 271 } 272 #endif 273 274 275.. index:: 276 pair: C; whitespace 277 278Line Formatting 279############### 280 281* Indentation must be 4 spaces, no tabs. 282 283* Do not leave trailing whitespace. 284 285* Lines should be no longer than 80 characters unless limiting line length 286 hurts readability. 287 288 289.. index:: 290 pair: C; comment 291 292Comments 293######## 294 295.. code-block:: c 296 297 /* Single-line comments may look like this */ 298 299 // ... or this 300 301 /* Multi-line comments should start immediately after the comment opening. 302 * Subsequent lines should start with an aligned asterisk. The comment 303 * closing should be aligned and on a line by itself. 304 */ 305 306 307.. index:: 308 single: C; pointer 309 310Variables 311######### 312 313* Pointers: 314 315.. code-block:: c 316 317 /* (1) The asterisk goes by the variable name, not the type; 318 * (2) Avoid leaving pointers uninitialized, to lessen the impact of 319 * use-before-assignment bugs 320 */ 321 char *my_string = NULL; 322 323 // Use space before asterisk and after closing parenthesis in a cast 324 char *foo = (char *) bar; 325 326* Global variables should be avoided in libraries when possible. State 327 information should instead be passed as function arguments (often as a 328 structure). This is not for thread safety -- Pacemaker's use of forking 329 ensures it will never be threaded -- but it does minimize overhead, 330 improve readability, and avoid obscure side effects. 331 332* Time intervals are sometimes represented in Pacemaker code as user-defined 333 text specifications (for example, "10s"), other times as an integer number of 334 seconds or milliseconds, and still other times as a string representation 335 of an integer number. Variables for these should be named with an indication 336 of which is being used (for example, use ``interval_spec``, ``interval_ms``, 337 or ``interval_ms_s`` instead of ``interval``). 338 339 340 341.. index:: 342 pair: C; operator 343 344Operators 345######### 346 347.. code-block:: c 348 349 // Operators have spaces on both sides 350 x = a; 351 352 /* (1) Do not rely on operator precedence; use parentheses when mixing 353 * operators with different priority, for readability. 354 * (2) No space is used after an opening parenthesis or before a closing 355 * parenthesis. 356 */ 357 x = a + b - (c * d); 358 359 360.. index:: 361 single: C; if 362 single: C; else 363 single: C; while 364 single: C; for 365 single: C; switch 366 367Control Statements (if, else, while, for, switch) 368################################################# 369 370.. code-block:: c 371 372 /* 373 * (1) The control keyword is followed by a space, a left parenthesis 374 * without a space, the condition, a right parenthesis, a space, and the 375 * opening bracket on the same line. 376 * (2) Always use braces around control statement blocks, even if they only 377 * contain one line. This makes code review diffs smaller if a line gets 378 * added in the future, and avoids the chance of bad indenting making a 379 * line incorrectly appear to be part of the block. 380 * (3) The closing bracket is on a line by itself. 381 */ 382 if (v < 0) { 383 return 0; 384 } 385 386 /* "else" and "else if" are on the same line with the previous ending brace 387 * and next opening brace, separated by a space. Blank lines may be used 388 * between blocks to help readability. 389 */ 390 if (v > 0) { 391 return 0; 392 393 } else if (a == 0) { 394 return 1; 395 396 } else { 397 return 2; 398 } 399 400 /* Do not use assignments in conditions. This ensures that the developer's 401 * intent is always clear, makes code reviews easier, and reduces the chance 402 * of using assignment where comparison is intended. 403 */ 404 // Do this ... 405 a = f(); 406 if (a) { 407 return 0; 408 } 409 // ... NOT this 410 if (a = f()) { 411 return 0; 412 } 413 414 /* It helps readability to use the "!" operator only in boolean 415 * comparisons, and explicitly compare numeric values against 0, 416 * pointers against NULL, etc. This helps remind the reader of the 417 * type being compared. 418 */ 419 int i = 0; 420 char *s = NULL; 421 bool cond = false; 422 423 if (!cond) { 424 return 0; 425 } 426 if (i == 0) { 427 return 0; 428 } 429 if (s == NULL) { 430 return 0; 431 } 432 433 /* In a "switch" statement, indent "case" one level, and indent the body of 434 * each "case" another level. 435 */ 436 switch (expression) { 437 case 0: 438 command1; 439 break; 440 case 1: 441 command2; 442 break; 443 default: 444 command3; 445 break; 446 } 447 448 449.. index:: 450 single: C; struct 451 452Structures 453########## 454 455Changes to structures defined in public API headers (adding or removing 456members, or changing member types) are generally not possible without breaking 457API compatibility. However, there are exceptions: 458 459* Public API structures can be designed such that they can be allocated only 460 via API functions, not declared directly or allocated with standard memory 461 functions using ``sizeof``. 462 463 * This can be enforced simply by documentating the limitation, in which case 464 new ``struct`` members can be added to the end of the structure without 465 breaking compatibility. 466 467 * Alternatively, the structure definition can be kept in an internal header, 468 with only a pointer type definition kept in a public header, in which case 469 the structure definition can be changed however needed. 470 471 472.. index:: 473 single: C; enum 474 475Enumerations 476############ 477 478* New values should usually be added to the end of public API enumerations, 479 because the compiler will define the values to 0, 1, etc., in the order 480 given, and inserting a value in the middle would change the numerical values 481 of all later values, breaking code compiled with the old values. However, if 482 enum numerical values are explicitly specified rather than left to the 483 compiler, new values can be added anywhere. 484 485* When defining constant integer values, enum should be preferred over 486 ``#define`` or ``const`` when possible. This allows type checking without 487 consuming memory. 488 489Flag groups 490___________ 491 492Pacemaker often uses flag groups (also called bit fields or bitmasks) for a 493collection of boolean options (flags/bits). 494 495This is more efficient for storage and manipulation than individual booleans, 496but its main advantage is when used in public APIs, because using another bit 497in a bitmask is backward compatible, whereas adding a new function argument (or 498sometimes even a structure member) is not. 499 500.. code-block:: c 501 502 #include <stdint.h> 503 504 /* (1) Define an enumeration to name the individual flags, for readability. 505 * An enumeration is preferred to a series of "#define" constants 506 * because it is typed, and logically groups the related names. 507 * (2) Define the values using left-shifting, which is more readable and 508 * less error-prone than hexadecimal literals (0x0001, 0x0002, 0x0004, 509 * etc.). 510 * (3) Using a comma after the last entry makes diffs smaller for reviewing 511 * if a new value needs to be added or removed later. 512 */ 513 enum pcmk__some_bitmask_type { 514 pcmk__some_value = (1 << 0), 515 pcmk__other_value = (1 << 1), 516 pcmk__another_value = (1 << 2), 517 }; 518 519 /* The flag group itself should be an unsigned type from stdint.h (not 520 * the enum type, since it will be a mask of the enum values and not just 521 * one of them). uint32_t is the most common, since we rarely need more than 522 * 32 flags, but a smaller or larger type could be appropriate in some 523 * cases. 524 */ 525 uint32_t flags = pcmk__some_value|pcmk__other_value; 526 527 /* If the values will be used only with uint64_t, define them accordingly, 528 * to make compilers happier. 529 */ 530 enum pcmk__something_else { 531 pcmk__whatever = (UINT64_C(1) << 0), 532 }; 533 534We have convenience functions for checking flags (see ``pcmk_any_flags_set()``, 535``pcmk_all_flags_set()``, and ``pcmk_is_set()``) as well as setting and 536clearing them (see ``pcmk__set_flags_as()`` and ``pcmk__clear_flags_as()``, 537usually used via wrapper macros defined for specific flag groups). These 538convenience functions should be preferred to direct bitwise arithmetic, for 539readability and logging consistency. 540 541 542.. index:: 543 pair: C; function 544 545Functions 546######### 547 548Function names should be unique across the entire project, to allow for 549individual tracing via ``PCMK_trace_functions``, and make it easier to search 550code and follow detail logs. 551 552 553Function Definitions 554____________________ 555 556.. code-block:: c 557 558 /* 559 * (1) The return type goes on its own line 560 * (2) The opening brace goes by itself on a line 561 * (3) Use "const" with pointer arguments whenever appropriate, to allow the 562 * function to be used by more callers. 563 */ 564 int 565 my_func1(const char *s) 566 { 567 return 0; 568 } 569 570 /* Functions with no arguments must explicitly list them as void, 571 * for compatibility with strict compilers 572 */ 573 int 574 my_func2(void) 575 { 576 return 0; 577 } 578 579 /* 580 * (1) For functions with enough arguments that they must break to the next 581 * line, align arguments with the first argument. 582 * (2) When a function argument is a function itself, use the pointer form. 583 * (3) Declare functions and file-global variables as ``static`` whenever 584 * appropriate. This gains a slight efficiency in shared libraries, and 585 * helps the reader know that it is not used outside the one file. 586 */ 587 static int 588 my_func3(int bar, const char *a, const char *b, const char *c, 589 void (*callback)()) 590 { 591 return 0; 592 } 593 594 595Return Values 596_____________ 597 598Functions that need to indicate success or failure should follow one of the 599following guidelines. More details, including functions for using them in user 600messages and converting from one to another, can be found in 601``include/crm/common/results.h``. 602 603* A **standard Pacemaker return code** is one of the ``pcmk_rc_*`` enum values 604 or a system errno code, as an ``int``. 605 606* ``crm_exit_t`` (the ``CRM_EX_*`` enum values) is a system-independent code 607 suitable for the exit status of a process, or for interchange between nodes. 608 609* Other special-purpose status codes exist, such as ``enum ocf_exitcode`` for 610 the possible exit statuses of OCF resource agents (along with some 611 Pacemaker-specific extensions). It is usually obvious when the context calls 612 for such. 613 614* Some older Pacemaker APIs use the now-deprecated "legacy" return values of 615 ``pcmk_ok`` or the positive or negative value of one of the ``pcmk_err_*`` 616 constants or system errno codes. 617 618* Functions registered with external libraries (as callbacks for example) 619 should use the appropriate signature defined by those libraries, rather than 620 follow Pacemaker guidelines. 621 622Of course, functions may have return values that aren't success/failure 623indicators, such as a pointer, integer count, or bool. 624 625 626Public API Functions 627____________________ 628 629Unless we are doing a (rare) release where we break public API compatibility, 630new public API functions can be added, but existing function signatures (return 631type, name, and argument types) should not be changed. To work around this, an 632existing function can become a wrapper for a new function. 633 634 635.. index:: 636 pair: C; memory 637 638Memory Management 639################# 640 641* Always use ``calloc()`` rather than ``malloc()``. It has no additional cost on 642 modern operating systems, and reduces the severity and security risks of 643 uninitialized memory usage bugs. 644 645* Ensure that all dynamically allocated memory is freed when no longer needed, 646 and not used after it is freed. This can be challenging in the more 647 event-driven, callback-oriented sections of code. 648 649* Free dynamically allocated memory using the free function corresponding to 650 how it was allocated. For example, use ``free()`` with ``calloc()``, and 651 ``g_free()`` with most glib functions that allocate objects. 652 653 654.. index:: 655 pair: C; logging 656 657Logging 658####### 659 660* When format strings are used for derived data types whose implementation may 661 vary across platforms (``pid_t``, ``time_t``, etc.), the safest approach is 662 to use ``%lld`` in the format string, and cast the value to ``long long``. 663 664* Do *not* pass ``NULL`` as an argument to satisfy the ``%s`` format specifier 665 in logging (and more generally, ``printf``-style) functions. When the string 666 "<null>" is a sufficient output representation in such case, you can use the 667 ``crm_str()`` convenience macro; otherwise, the ternary operator is an 668 obvious choice. 669 670* Do not rely on ``%s`` handling ``NULL`` values properly. While the standard 671 library functions might, not all Pacemaker API using them does, and it's 672 safest to get in the habit of always ensuring format values are non-NULL. 673 674 675.. index:: 676 pair: C; regular expression 677 678Regular Expressions 679################### 680 681- Use ``REG_NOSUB`` with ``regcomp()`` whenever possible, for efficiency. 682- Be sure to use ``regfree()`` appropriately. 683 684 685.. index:: 686 single: Makefile.am 687 688Makefiles 689######### 690 691Pacemaker uses 692`automake <https://www.gnu.org/software/automake/manual/automake.html>`_ 693for building, so the Makefile.am in each directory should be edited rather than 694Makefile.in or Makefile, which are automatically generated. 695 696* Public API headers are installed (by adding them to a ``HEADERS`` variable in 697 ``Makefile.am``), but internal API headers are not (by adding them to 698 ``noinst_HEADERS``). 699 700 701.. index:: 702 pair: C; vim settings 703 704vim Settings 705############ 706 707Developers who use ``vim`` to edit source code can add the following settings 708to their ``~/.vimrc`` file to follow Pacemaker C coding guidelines: 709 710.. code-block:: none 711 712 " follow Pacemaker coding guidelines when editing C source code files 713 filetype plugin indent on 714 au FileType c setlocal expandtab tabstop=4 softtabstop=4 shiftwidth=4 textwidth=80 715 autocmd BufNewFile,BufRead *.h set filetype=c 716 let c_space_errors = 1 717