From 5155f6f4b12fda8c064ecc45539e7d253156eae0 Mon Sep 17 00:00:00 2001 From: Andrea Bolognani Date: Mon, 6 Apr 2020 14:43:56 +0200 Subject: [PATCH] docs: Add coding-style.rst MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This part represents the biggest chunk of the existing hacking.rst, and despite that its utility is very limited because 'make syntax-check' already guarantees most of the rules are followed over time. Until the glorious day we finally codify our coding style completely into a configuration for a tool such as clang-format and thus no longer need a plain English description of it, move this part to a separate page. Signed-off-by: Andrea Bolognani Reviewed-by: Daniel P. Berrangé --- build-aux/syntax-check.mk | 2 +- docs/coding-style.rst | 919 ++++++++++++++++++++++++++++++++++++++ docs/hacking.rst | 914 ------------------------------------- 3 files changed, 920 insertions(+), 915 deletions(-) create mode 100644 docs/coding-style.rst diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index 6ffea7afb9..8bee1bbbf1 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -2048,7 +2048,7 @@ exclude_file_name_regexp--sc_prohibit_readlink = \ exclude_file_name_regexp--sc_prohibit_setuid = ^src/util/virutil\.c|tools/virt-login-shell\.c$$ exclude_file_name_regexp--sc_prohibit_snprintf = \ - ^(build-aux/syntax-check\.mk|docs/hacking\.rst|tools/virt-login-shell\.c)$$ + ^(build-aux/syntax-check\.mk|docs/coding-style\.rst|tools/virt-login-shell\.c)$$ exclude_file_name_regexp--sc_prohibit_strtol = ^examples/.*$$ diff --git a/docs/coding-style.rst b/docs/coding-style.rst new file mode 100644 index 0000000000..151ea87b6a --- /dev/null +++ b/docs/coding-style.rst @@ -0,0 +1,919 @@ +============ +Coding style +============ + +.. contents:: + +Naming conventions +================== + +When reading libvirt code, a number of different naming +conventions will be evident due to various changes in thinking +over the course of the project's lifetime. The conventions +documented below should be followed when creating any entirely new +files in libvirt. When working on existing files, while it is +desirable to apply these conventions, keeping a consistent style +with existing code in that particular file is generally more +important. The overall guiding principal is that every file, enum, +struct, function, macro and typedef name must have a 'vir' or +'VIR' prefix. All local scope variable names are exempt, and +global variables are exempt, unless exported in a header file. + +File names + File naming varies depending on the subdirectory. The preferred + style is to have a 'vir' prefix, followed by a name which + matches the name of the functions / objects inside the file. + For example, a file containing an object 'virHashtable' is + stored in files 'virhashtable.c' and 'virhashtable.h'. + Sometimes, methods which would otherwise be declared 'static' + need to be exported for use by a test suite. For this purpose a + second header file should be added with a suffix of 'priv', + e.g. 'virhashtablepriv.h'. Use of underscores in file names is + discouraged when using the 'vir' prefix style. The 'vir' prefix + naming applies to src/util, src/rpc and tests/ directories. + Most other directories do not follow this convention. + +Enum type & field names + All enums should have a 'vir' prefix in their typedef name, and + each following word should have its first letter in uppercase. + The enum name should match the typedef name with a leading + underscore. The enum member names should be in all uppercase, + and use an underscore to separate each word. The enum member + name prefix should match the enum typedef name. + + :: + + typedef enum _virSocketType virSocketType; + enum _virSocketType { + VIR_SOCKET_TYPE_IPV4, + VIR_SOCKET_TYPE_IPV6, + }; + +Struct type names + All structs should have a 'vir' prefix in their typedef name, + and each following word should have its first letter in + uppercase. The struct name should be the same as the typedef + name with a leading underscore. A second typedef should be + given for a pointer to the struct with a 'Ptr' suffix. + + :: + + typedef struct _virHashTable virHashTable; + typedef virHashTable *virHashTablePtr; + struct _virHashTable { + ... + }; + +Function names + All functions should have a 'vir' prefix in their name, + followed by one or more words with first letter of each word + capitalized. Underscores should not be used in function names. + If the function is operating on an object, then the function + name prefix should match the object typedef name, otherwise it + should match the filename. Following this comes the verb / + action name, and finally an optional subject name. For example, + given an object 'virHashTable', all functions should have a + name 'virHashTable$VERB' or 'virHashTable$VERB$SUBJECT", e.g. + 'virHashTableLookup' or 'virHashTableGetValue'. + +Macro names + All macros should have a "VIR" prefix in their name, followed + by one or more uppercase words separated by underscores. The + macro argument names should be in lowercase. Aside from having + a "VIR" prefix there are no common practices for the rest of + the macro name. + +Code indentation +================ + +Libvirt's C source code generally adheres to some basic +code-formatting conventions. The existing code base is not totally +consistent on this front, but we do prefer that contributed code +be formatted similarly. In short, use spaces-not-TABs for +indentation, use 4 spaces for each indentation level, and other +than that, follow the K&R style. + +If you use Emacs, the project includes a file .dir-locals.el that +sets up the preferred indentation. If you use vim, append the +following to your ~/.vimrc file: + +:: + + set nocompatible + filetype on + set autoindent + set smartindent + set cindent + set tabstop=8 + set shiftwidth=4 + set expandtab + set cinoptions=(0,:0,l1,t0,L3 + filetype plugin indent on + au FileType make setlocal noexpandtab + au BufRead,BufNewFile *.am setlocal noexpandtab + match ErrorMsg /\s\+$\| \+\ze\t/ + +Or if you don't want to mess your ~/.vimrc up, you can save the +above into a file called .lvimrc (not .vimrc) located at the root +of libvirt source, then install a vim script from +http://www.vim.org/scripts/script.php?script_id=1408, which will +load the .lvimrc only when you edit libvirt code. + +Code formatting (especially for new code) +========================================= + +With new code, we can be even more strict. Please apply the +following function (using GNU indent) to any new code. Note that +this also gives you an idea of the type of spacing we prefer +around operators and keywords: + +:: + + indent-libvirt() + { + indent -bad -bap -bbb -bli4 -br -ce -brs -cs -i4 -l75 -lc75 \ + -sbi4 -psl -saf -sai -saw -sbi4 -ss -sc -cdw -cli4 -npcs -nbc \ + --no-tabs "$@" + } + +Note that sometimes you'll have to post-process that output +further, by piping it through ``expand -i``, since some leading +TABs can get through. Usually they're in macro definitions or +strings, and should be converted anyhow. + +Libvirt requires a C99 compiler for various reasons. However, most +of the code base prefers to stick to C89 syntax unless there is a +compelling reason otherwise. For example, it is preferable to use +``/* */`` comments rather than ``//``. Also, when declaring local +variables, the prevailing style has been to declare them at the +beginning of a scope, rather than immediately before use. + +Bracket spacing +--------------- + +The keywords ``if``, ``for``, ``while``, and ``switch`` must have +a single space following them before the opening bracket. E.g. + +:: + + if(foo) // Bad + if (foo) // Good + +Function implementations must **not** have any whitespace between +the function name and the opening bracket. E.g. + +:: + + int foo (int wizz) // Bad + int foo(int wizz) // Good + +Function calls must **not** have any whitespace between the +function name and the opening bracket. E.g. + +:: + + bar = foo (wizz); // Bad + bar = foo(wizz); // Good + +Function typedefs must **not** have any whitespace between the +closing bracket of the function name and opening bracket of the +arg list. E.g. + +:: + + typedef int (*foo) (int wizz); // Bad + typedef int (*foo)(int wizz); // Good + +There must not be any whitespace immediately following any opening +bracket, or immediately prior to any closing bracket. E.g. + +:: + + int foo( int wizz ); // Bad + int foo(int wizz); // Good + +Commas +------ + +Commas should always be followed by a space or end of line, and +never have leading space; this is enforced during 'make +syntax-check'. + +:: + + call(a,b ,c);// Bad + call(a, b, c); // Good + +When declaring an enum or using a struct initializer that occupies +more than one line, use a trailing comma. That way, future edits +to extend the list only have to add a line, rather than modify an +existing line to add the intermediate comma. Any sentinel +enumerator value with a name ending in \_LAST is exempt, since you +would extend such an enum before the \_LAST element. Another +reason to favor trailing commas is that it requires less effort to +produce via code generators. Note that the syntax checker is +unable to enforce a style of trailing commas, so there are +counterexamples in existing code which do not use it; also, while +C99 allows trailing commas, remember that JSON and XDR do not. + +:: + + enum { + VALUE_ONE, + VALUE_TWO // Bad + }; + enum { + VALUE_THREE, + VALUE_FOUR, // Good + }; + +Semicolons +---------- + +Semicolons should never have a space beforehand. Inside the +condition of a ``for`` loop, there should always be a space or +line break after each semicolon, except for the special case of an +infinite loop (although more infinite loops use ``while``). While +not enforced, loop counters generally use post-increment. + +:: + + for (i = 0 ;i < limit ; ++i) { // Bad + for (i = 0; i < limit; i++) { // Good + for (;;) { // ok + while (1) { // Better + +Empty loop bodies are better represented with curly braces and a +comment, although use of a semicolon is not currently rejected. + +:: + + while ((rc = waitpid(pid, &st, 0) == -1) && + errno == EINTR); // ok + while ((rc = waitpid(pid, &st, 0) == -1) && + errno == EINTR) { // Better + /* nothing */ + } + +Curly braces +------------ + +Omit the curly braces around an ``if``, ``while``, ``for`` etc. +body only when both that body and the condition itself occupy a +single line. In every other case we require the braces. This +ensures that it is trivially easy to identify a +single-\ *statement* loop: each has only one *line* in its body. + +:: + + while (expr) // single line body; {} is forbidden + single_line_stmt(); + +:: + + while (expr(arg1, + arg2)) // indentation makes it obvious it is single line, + single_line_stmt(); // {} is optional (not enforced either way) + +:: + + while (expr1 && + expr2) { // multi-line, at same indentation, {} required + single_line_stmt(); + } + +However, the moment your loop/if/else body extends on to a second +line, for whatever reason (even if it's just an added comment), +then you should add braces. Otherwise, it would be too easy to +insert a statement just before that comment (without adding +braces), thinking it is already a multi-statement loop: + +:: + + while (true) // BAD! multi-line body with no braces + /* comment... */ + single_line_stmt(); + +Do this instead: + +:: + + while (true) { // Always put braces around a multi-line body. + /* comment... */ + single_line_stmt(); + } + +There is one exception: when the second body line is not at the +same indentation level as the first body line: + +:: + + if (expr) + die("a diagnostic that would make this line" + " extend past the 80-column limit")); + +It is safe to omit the braces in the code above, since the +further-indented second body line makes it obvious that this is +still a single-statement body. + +To reiterate, don't do this: + +:: + + if (expr) // BAD: no braces around... + while (expr_2) { // ... a multi-line body + ... + } + +Do this, instead: + +:: + + if (expr) { + while (expr_2) { + ... + } + } + +However, there is one exception in the other direction, when even +a one-line block should have braces. That occurs when that +one-line, brace-less block is an ``if`` or ``else`` block, and the +counterpart block **does** use braces. In that case, put braces +around both blocks. Also, if the ``else`` block is much shorter +than the ``if`` block, consider negating the ``if``-condition and +swapping the bodies, putting the short block first and making the +longer, multi-line block be the ``else`` block. + +:: + + if (expr) { + ... + ... + } + else + x = y; // BAD: braceless "else" with braced "then", + // and short block last + + if (expr) + x = y; // BAD: braceless "if" with braced "else" + else { + ... + ... + } + +Keeping braces consistent and putting the short block first is +preferred, especially when the multi-line body is more than a few +lines long, because it is easier to read and grasp the semantics +of an if-then-else block when the simpler block occurs first, +rather than after the more involved block: + +:: + + if (!expr) { + x = y; // putting the smaller block first is more readable + } else { + ... + ... + } + +But if negating a complex condition is too ugly, then at least add +braces: + +:: + + if (complex expr not worth negating) { + ... + ... + } else { + x = y; + } + +Use hanging braces for compound statements: the opening brace of a +compound statement should be on the same line as the condition +being tested. Only top-level function bodies, nested scopes, and +compound structure declarations should ever have { on a line by +itself. + +:: + + void + foo(int a, int b) + { // correct - function body + int 2d[][] = { + { // correct - complex initialization + 1, 2, + }, + }; + if (a) + { // BAD: compound brace on its own line + do_stuff(); + } + { // correct - nested scope + int tmp; + if (a < b) { // correct - hanging brace + tmp = b; + b = a; + a = tmp; + } + } + } + +Conditional expressions +----------------------- + +For readability reasons new code should avoid shortening +comparisons to 0 for numeric types. Boolean and pointer +comparisions may be shortened. All long forms are okay: + +:: + + virFooPtr foos = NULL; + size nfoos = 0; + bool hasFoos = false; + + GOOD: + if (!foos) + if (!hasFoos) + if (nfoos == 0) + if (foos == NULL) + if (hasFoos == true) + + BAD: + if (!nfoos) + if (nfoos) + +New code should avoid the ternary operator as much as possible. +Specifically it must never span more than one line or nest: + +:: + + BAD: + char *foo = baz ? + virDoSomethingReallyComplex(driver, vm, something, baz->foo) : + NULL; + + char *foo = bar ? bar->baz ? bar->baz->foo : "nobaz" : "nobar"; + +Preprocessor +------------ + +Macros defined with an ALL_CAPS name should generally be assumed +to be unsafe with regards to arguments with side-effects (that is, +MAX(a++, b--) might increment a or decrement b too many or too few +times). Exceptions to this rule are explicitly documented for +macros in viralloc.h and virstring.h. + +For variadic macros, stick with C99 syntax: + +:: + + #define vshPrint(_ctl, ...) fprintf(stdout, __VA_ARGS__) + +Use parenthesis when checking if a macro is defined, and use +indentation to track nesting: + +:: + + #if defined(HAVE_POSIX_FALLOCATE) && !defined(HAVE_FALLOCATE) + # define fallocate(a, ignored, b, c) posix_fallocate(a, b, c) + #endif + +C types +------- + +Use the right type. + +Scalars +~~~~~~~ + +- If you're using ``int`` or ``long``, odds are good that there's + a better type. +- If a variable is counting something, be sure to declare it with + an unsigned type. +- If it's memory-size-related, use ``size_t`` (use ``ssize_t`` + only if required). +- If it's file-size related, use uintmax_t, or maybe ``off_t``. +- If it's file-offset related (i.e., signed), use ``off_t``. +- If it's just counting small numbers use ``unsigned int``; (on + all but oddball embedded systems, you can assume that that type + is at least four bytes wide). +- If a variable has boolean semantics, give it the ``bool`` type + and use the corresponding ``true`` and ``false`` macros. +- In the unusual event that you require a specific width, use a + standard type like ``int32_t``, ``uint32_t``, ``uint64_t``, + etc. +- While using ``bool`` is good for readability, it comes with + minor caveats: + + - Don't use ``bool`` in places where the type size must be + constant across all systems, like public interfaces and + on-the-wire protocols. Note that it would be possible + (albeit wasteful) to use ``bool`` in libvirt's logical wire + protocol, since XDR maps that to its lower-level ``bool_t`` + type, which **is** fixed-size. + - Don't compare a bool variable against the literal, ``true``, + since a value with a logical non-false value need not be + ``1``. I.e., don't write ``if (seen == true) ...``. Rather, + write ``if (seen)...``. + +Of course, take all of the above with a grain of salt. If you're +about to use some system interface that requires a type like +``size_t``, ``pid_t`` or ``off_t``, use matching types for any +corresponding variables. + +Also, if you try to use e.g., ``unsigned int`` as a type, and that +conflicts with the signedness of a related variable, sometimes +it's best just to use the **wrong** type, if *pulling the thread* +and fixing all related variables would be too invasive. + +Finally, while using descriptive types is important, be careful +not to go overboard. If whatever you're doing causes warnings, or +requires casts, then reconsider or ask for help. + +Pointers +~~~~~~~~ + +Ensure that all of your pointers are *const-correct*. Unless a +pointer is used to modify the pointed-to storage, give it the +``const`` attribute. That way, the reader knows up-front that this +is a read-only pointer. Perhaps more importantly, if we're +diligent about this, when you see a non-const pointer, you're +guaranteed that it is used to modify the storage it points to, or +it is aliased to another pointer that is. + +Attribute annotations +--------------------- + +Use the following annotations to help the compiler and/or static +analysis tools understand the code better: + ++-------------------------------+------------------------------------------------------------+ +| Macro | Meaning | ++===============================+============================================================+ +| ``ATTRIBUTE_NONNULL`` | passing NULL for this parameter is not allowed | ++-------------------------------+------------------------------------------------------------+ +| ``ATTRIBUTE_PACKED`` | force a structure to be packed | ++-------------------------------+------------------------------------------------------------+ +| ``G_GNUC_FALLTHROUGH`` | allow code reuse by multiple switch cases | ++-------------------------------+------------------------------------------------------------+ +| ``G_GNUC_NO_INLINE`` | the function is mocked in the test suite | ++-------------------------------+------------------------------------------------------------+ +| ``G_GNUC_NORETURN`` | the function never returns | ++-------------------------------+------------------------------------------------------------+ +| ``G_GNUC_NULL_TERMINATED`` | last parameter must be NULL | ++-------------------------------+------------------------------------------------------------+ +| ``G_GNUC_PRINTF`` | validate that the formatting string matches parameters | ++-------------------------------+------------------------------------------------------------+ +| ``G_GNUC_UNUSED`` | parameter is unused in this implementation of the function | ++-------------------------------+------------------------------------------------------------+ +| ``G_GNUC_WARN_UNUSED_RESULT`` | the return value must be checked | ++-------------------------------+------------------------------------------------------------+ + +File handling +------------- + +Usage of the ``fdopen()``, ``close()``, ``fclose()`` APIs is +deprecated in libvirt code base to help avoiding double-closing of +files or file descriptors, which is particularly dangerous in a +multi-threaded application. Instead of these APIs, use the macros +from virfile.h + +- Open a file from a file descriptor: + + :: + + if ((file = VIR_FDOPEN(fd, "r")) == NULL) { + virReportSystemError(errno, "%s", + _("failed to open file from file descriptor")); + return -1; + } + /* fd is now invalid; only access the file using file variable */ + +- Close a file descriptor: + + :: + + if (VIR_CLOSE(fd) < 0) { + virReportSystemError(errno, "%s", _("failed to close file")); + } + +- Close a file: + + :: + + if (VIR_FCLOSE(file) < 0) { + virReportSystemError(errno, "%s", _("failed to close file")); + } + +- Close a file or file descriptor in an error path, without + losing the previous ``errno`` value: + + :: + + VIR_FORCE_CLOSE(fd); + VIR_FORCE_FCLOSE(file); + +String comparisons +------------------ + +Do not use the strcmp, strncmp, etc functions directly. Instead +use one of the following semantically named macros + +- For strict equality: + + :: + + STREQ(a,b) + STRNEQ(a,b) + +- For case insensitive equality: + + :: + + STRCASEEQ(a,b) + STRCASENEQ(a,b) + +- For strict equality of a substring: + + :: + + STREQLEN(a,b,n) + STRNEQLEN(a,b,n) + +- For case insensitive equality of a substring: + + :: + + STRCASEEQLEN(a,b,n) + STRCASENEQLEN(a,b,n) + +- For strict equality of a prefix: + + :: + + STRPREFIX(a,b) + +- To avoid having to check if a or b are NULL: + + :: + + STREQ_NULLABLE(a, b) + STRNEQ_NULLABLE(a, b) + +String copying +-------------- + +Do not use the strncpy function. According to the man page, it +does **not** guarantee a NULL-terminated buffer, which makes it +extremely dangerous to use. Instead, use one of the replacement +functions provided by libvirt: + +:: + + virStrncpy(char *dest, const char *src, size_t n, size_t destbytes) + +The first two arguments have the same meaning as for strncpy, +namely the destination and source of the copy operation. Unlike +strncpy, the function will always copy exactly the number of bytes +requested and make sure the destination is NULL-terminated, as the +source is required to be; sanity checks are performed to ensure +the size of the destination, as specified by the last argument, is +sufficient for the operation to succeed. On success, 0 is +returned; on failure, a value <0 is returned instead. + +:: + + virStrcpy(char *dest, const char *src, size_t destbytes) + +Use this variant if you know you want to copy the entire src +string into dest. + +:: + + virStrcpyStatic(char *dest, const char *src) + +Use this variant if you know you want to copy the entire src +string into dest **and** you know that your destination string is +a static string (i.e. that sizeof(dest) returns something +meaningful). Note that this is a macro, so arguments could be +evaluated more than once. + +:: + + dst = g_strdup(src); + dst = g_strndup(src, n); + +You should avoid using strdup or strndup directly as they do not +handle out-of-memory errors, and do not allow a NULL source. Use +``g_strdup`` and ``g_strndup`` from GLib which abort on OOM and +handle NULL source by returning NULL. + +Variable length string buffer +----------------------------- + +If there is a need for complex string concatenations, avoid using +the usual sequence of malloc/strcpy/strcat/snprintf functions and +make use of either the +`GString `__ +type from GLib or the virBuffer API. If formatting XML or QEMU +command line is needed, use the virBuffer API described in +virbuffer.h, since it has helper functions for those. + +Typical usage is as follows: + +:: + + char * + somefunction(...) + { + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + + ... + + virBufferAddLit(&buf, "\n"); + + ... + + if (some_error) + return NULL; /* g_auto will free the memory used so far */ + + ... + + virBufferAddLit(&buf, "\n"); + + ... + + if (virBufferCheckError(&buf) < 0) + return NULL; + + return virBufferContentAndReset(&buf); + } + +Include files +------------- + +There are now quite a large number of include files, both libvirt +internal and external, and system includes. To manage all this +complexity it's best to stick to the following general plan for +all \*.c source files: + +:: + + /* + * Copyright notice + * .... + * .... + * .... + * + */ + + #include Must come first in every file. + + #include Any system includes you need. + #include + #include + + #if WITH_NUMACTL Some system includes aren't supported + # include everywhere so need these #if guards. + #endif + + #include "internal.h" Include this first, after system includes. + + #include "util.h" Any libvirt internal header files. + #include "buf.h" + + static int + myInternalFunc() The actual code. + { + ... + +Of particular note: **Do not** include libvirt/libvirt.h, +libvirt/virterror.h, libvirt/libvirt-qemu.h, or +libvirt/libvirt-lxc.h. They are included by "internal.h" already +and there are some special reasons why you cannot include these +files explicitly. One of the special cases, "libvirt/libvirt.h" is +included prior to "internal.h" in "remote_protocol.x", to avoid +exposing \*_LAST enum elements. + +Printf-style functions +---------------------- + +Whenever you add a new printf-style function, i.e., one with a +format string argument and following "..." in its prototype, be +sure to use gcc's printf attribute directive in the prototype. For +example, here's the one for virCommandAddEnvFormat in +vircommand.h: + +:: + + void virCommandAddEnvFormat(virCommandPtr cmd, const char *format, ...) + G_GNUC_PRINTF(2, 3); + +This makes it so gcc's -Wformat and -Wformat-security options can +do their jobs and cross-check format strings with the number and +types of arguments. + +When printing to a string, consider using GString or virBuffer for +incremental allocations, g_strdup_printf for a one-shot +allocation, and g_snprintf for fixed-width buffers. Only use +g_sprintf, if you can prove the buffer won't overflow. + +Error message format +-------------------- + +Error messages visible to the user should be short and +descriptive. All error messages are translated using gettext and +thus must be wrapped in ``_()`` macro. To simplify the translation +work, the error message must not be concatenated from various +parts. To simplify searching for the error message in the code the +strings should not be broken even if they result into a line +longer than 80 columns and any formatting modifier should be +enclosed by quotes or other obvious separator. If a string used +with ``%s`` can be NULL the NULLSTR macro must be used. + +:: + + GOOD: virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to connect to remote host '%s'"), hostname) + + BAD: virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to %s to remote host '%s'"), + "connect", hostname); + + BAD: virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to connect " + "to remote host '%s'), + hostname); + +Use of goto +----------- + +The use of goto is not forbidden, and goto is widely used +throughout libvirt. While the uncontrolled use of goto will +quickly lead to unmaintainable code, there is a place for it in +well structured code where its use increases readability and +maintainability. In general, if goto is used for error recovery, +it's likely to be ok, otherwise, be cautious or avoid it all +together. + +The typical use of goto is to jump to cleanup code in the case of +a long list of actions, any of which may fail and cause the entire +operation to fail. In this case, a function will have a single +label at the end of the function. It's almost always ok to use +this style. In particular, if the cleanup code only involves +free'ing memory, then having multiple labels is overkill. g_free() +and most of the functions named XXXFree() in libvirt is required +to handle NULL as its arg. This does not apply to libvirt's public +APIs. Thus you can safely call free on all the variables even if +they were not yet allocated (yes they have to have been +initialized to NULL). This is much simpler and clearer than having +multiple labels. Note that most of libvirt's type declarations can +be marked with either ``g_autofree`` or ``g_autoptr`` which uses +the compiler's ``__attribute__((cleanup))`` that calls the +appropriate free function when the variable goes out of scope. + +There are a couple of signs that a particular use of goto is not +ok: + +- You're using multiple labels. If you find yourself using + multiple labels, you're strongly encouraged to rework your code + to eliminate all but one of them. +- The goto jumps back up to a point above the current line of + code being executed. Please use some combination of looping + constructs to re-execute code instead; it's almost certainly + going to be more understandable by others. One well-known + exception to this rule is restarting an i/o operation following + EINTR. +- The goto jumps down to an arbitrary place in the middle of a + function followed by further potentially failing calls. You + should almost certainly be using a conditional and a block + instead of a goto. Perhaps some of your function's logic would + be better pulled out into a helper function. + +Although libvirt does not encourage the Linux kernel wind/unwind +style of multiple labels, there's a good general discussion of the +issue archived at +`KernelTrap `__ + +When using goto, please use one of these standard labels if it +makes sense: + +:: + + error: A path only taken upon return with an error code + cleanup: A path taken upon return with success code + optional error + no_memory: A path only taken upon return with an OOM error code + retry: If needing to jump upwards (e.g., retry on EINTR) + +Top-level labels should be indented by one space (putting them on +the beginning of the line confuses function context detection in +git): + +:: + + int foo() + { + /* ... do stuff ... */ + cleanup: + /* ... do other stuff ... */ + } diff --git a/docs/hacking.rst b/docs/hacking.rst index 9962802bc5..356dbc506b 100644 --- a/docs/hacking.rst +++ b/docs/hacking.rst @@ -356,920 +356,6 @@ include: - `YouCompleteMe `__, a vim plugin for libclang-powered semantic code completion. -Naming conventions -================== - -When reading libvirt code, a number of different naming -conventions will be evident due to various changes in thinking -over the course of the project's lifetime. The conventions -documented below should be followed when creating any entirely new -files in libvirt. When working on existing files, while it is -desirable to apply these conventions, keeping a consistent style -with existing code in that particular file is generally more -important. The overall guiding principal is that every file, enum, -struct, function, macro and typedef name must have a 'vir' or -'VIR' prefix. All local scope variable names are exempt, and -global variables are exempt, unless exported in a header file. - -File names - File naming varies depending on the subdirectory. The preferred - style is to have a 'vir' prefix, followed by a name which - matches the name of the functions / objects inside the file. - For example, a file containing an object 'virHashtable' is - stored in files 'virhashtable.c' and 'virhashtable.h'. - Sometimes, methods which would otherwise be declared 'static' - need to be exported for use by a test suite. For this purpose a - second header file should be added with a suffix of 'priv', - e.g. 'virhashtablepriv.h'. Use of underscores in file names is - discouraged when using the 'vir' prefix style. The 'vir' prefix - naming applies to src/util, src/rpc and tests/ directories. - Most other directories do not follow this convention. - -Enum type & field names - All enums should have a 'vir' prefix in their typedef name, and - each following word should have its first letter in uppercase. - The enum name should match the typedef name with a leading - underscore. The enum member names should be in all uppercase, - and use an underscore to separate each word. The enum member - name prefix should match the enum typedef name. - - :: - - typedef enum _virSocketType virSocketType; - enum _virSocketType { - VIR_SOCKET_TYPE_IPV4, - VIR_SOCKET_TYPE_IPV6, - }; - -Struct type names - All structs should have a 'vir' prefix in their typedef name, - and each following word should have its first letter in - uppercase. The struct name should be the same as the typedef - name with a leading underscore. A second typedef should be - given for a pointer to the struct with a 'Ptr' suffix. - - :: - - typedef struct _virHashTable virHashTable; - typedef virHashTable *virHashTablePtr; - struct _virHashTable { - ... - }; - -Function names - All functions should have a 'vir' prefix in their name, - followed by one or more words with first letter of each word - capitalized. Underscores should not be used in function names. - If the function is operating on an object, then the function - name prefix should match the object typedef name, otherwise it - should match the filename. Following this comes the verb / - action name, and finally an optional subject name. For example, - given an object 'virHashTable', all functions should have a - name 'virHashTable$VERB' or 'virHashTable$VERB$SUBJECT", e.g. - 'virHashTableLookup' or 'virHashTableGetValue'. - -Macro names - All macros should have a "VIR" prefix in their name, followed - by one or more uppercase words separated by underscores. The - macro argument names should be in lowercase. Aside from having - a "VIR" prefix there are no common practices for the rest of - the macro name. - -Code indentation -================ - -Libvirt's C source code generally adheres to some basic -code-formatting conventions. The existing code base is not totally -consistent on this front, but we do prefer that contributed code -be formatted similarly. In short, use spaces-not-TABs for -indentation, use 4 spaces for each indentation level, and other -than that, follow the K&R style. - -If you use Emacs, the project includes a file .dir-locals.el that -sets up the preferred indentation. If you use vim, append the -following to your ~/.vimrc file: - -:: - - set nocompatible - filetype on - set autoindent - set smartindent - set cindent - set tabstop=8 - set shiftwidth=4 - set expandtab - set cinoptions=(0,:0,l1,t0,L3 - filetype plugin indent on - au FileType make setlocal noexpandtab - au BufRead,BufNewFile *.am setlocal noexpandtab - match ErrorMsg /\s\+$\| \+\ze\t/ - -Or if you don't want to mess your ~/.vimrc up, you can save the -above into a file called .lvimrc (not .vimrc) located at the root -of libvirt source, then install a vim script from -http://www.vim.org/scripts/script.php?script_id=1408, which will -load the .lvimrc only when you edit libvirt code. - -Code formatting (especially for new code) -========================================= - -With new code, we can be even more strict. Please apply the -following function (using GNU indent) to any new code. Note that -this also gives you an idea of the type of spacing we prefer -around operators and keywords: - -:: - - indent-libvirt() - { - indent -bad -bap -bbb -bli4 -br -ce -brs -cs -i4 -l75 -lc75 \ - -sbi4 -psl -saf -sai -saw -sbi4 -ss -sc -cdw -cli4 -npcs -nbc \ - --no-tabs "$@" - } - -Note that sometimes you'll have to post-process that output -further, by piping it through ``expand -i``, since some leading -TABs can get through. Usually they're in macro definitions or -strings, and should be converted anyhow. - -Libvirt requires a C99 compiler for various reasons. However, most -of the code base prefers to stick to C89 syntax unless there is a -compelling reason otherwise. For example, it is preferable to use -``/* */`` comments rather than ``//``. Also, when declaring local -variables, the prevailing style has been to declare them at the -beginning of a scope, rather than immediately before use. - -Bracket spacing ---------------- - -The keywords ``if``, ``for``, ``while``, and ``switch`` must have -a single space following them before the opening bracket. E.g. - -:: - - if(foo) // Bad - if (foo) // Good - -Function implementations must **not** have any whitespace between -the function name and the opening bracket. E.g. - -:: - - int foo (int wizz) // Bad - int foo(int wizz) // Good - -Function calls must **not** have any whitespace between the -function name and the opening bracket. E.g. - -:: - - bar = foo (wizz); // Bad - bar = foo(wizz); // Good - -Function typedefs must **not** have any whitespace between the -closing bracket of the function name and opening bracket of the -arg list. E.g. - -:: - - typedef int (*foo) (int wizz); // Bad - typedef int (*foo)(int wizz); // Good - -There must not be any whitespace immediately following any opening -bracket, or immediately prior to any closing bracket. E.g. - -:: - - int foo( int wizz ); // Bad - int foo(int wizz); // Good - -Commas ------- - -Commas should always be followed by a space or end of line, and -never have leading space; this is enforced during 'make -syntax-check'. - -:: - - call(a,b ,c);// Bad - call(a, b, c); // Good - -When declaring an enum or using a struct initializer that occupies -more than one line, use a trailing comma. That way, future edits -to extend the list only have to add a line, rather than modify an -existing line to add the intermediate comma. Any sentinel -enumerator value with a name ending in \_LAST is exempt, since you -would extend such an enum before the \_LAST element. Another -reason to favor trailing commas is that it requires less effort to -produce via code generators. Note that the syntax checker is -unable to enforce a style of trailing commas, so there are -counterexamples in existing code which do not use it; also, while -C99 allows trailing commas, remember that JSON and XDR do not. - -:: - - enum { - VALUE_ONE, - VALUE_TWO // Bad - }; - enum { - VALUE_THREE, - VALUE_FOUR, // Good - }; - -Semicolons ----------- - -Semicolons should never have a space beforehand. Inside the -condition of a ``for`` loop, there should always be a space or -line break after each semicolon, except for the special case of an -infinite loop (although more infinite loops use ``while``). While -not enforced, loop counters generally use post-increment. - -:: - - for (i = 0 ;i < limit ; ++i) { // Bad - for (i = 0; i < limit; i++) { // Good - for (;;) { // ok - while (1) { // Better - -Empty loop bodies are better represented with curly braces and a -comment, although use of a semicolon is not currently rejected. - -:: - - while ((rc = waitpid(pid, &st, 0) == -1) && - errno == EINTR); // ok - while ((rc = waitpid(pid, &st, 0) == -1) && - errno == EINTR) { // Better - /* nothing */ - } - -Curly braces ------------- - -Omit the curly braces around an ``if``, ``while``, ``for`` etc. -body only when both that body and the condition itself occupy a -single line. In every other case we require the braces. This -ensures that it is trivially easy to identify a -single-\ *statement* loop: each has only one *line* in its body. - -:: - - while (expr) // single line body; {} is forbidden - single_line_stmt(); - -:: - - while (expr(arg1, - arg2)) // indentation makes it obvious it is single line, - single_line_stmt(); // {} is optional (not enforced either way) - -:: - - while (expr1 && - expr2) { // multi-line, at same indentation, {} required - single_line_stmt(); - } - -However, the moment your loop/if/else body extends on to a second -line, for whatever reason (even if it's just an added comment), -then you should add braces. Otherwise, it would be too easy to -insert a statement just before that comment (without adding -braces), thinking it is already a multi-statement loop: - -:: - - while (true) // BAD! multi-line body with no braces - /* comment... */ - single_line_stmt(); - -Do this instead: - -:: - - while (true) { // Always put braces around a multi-line body. - /* comment... */ - single_line_stmt(); - } - -There is one exception: when the second body line is not at the -same indentation level as the first body line: - -:: - - if (expr) - die("a diagnostic that would make this line" - " extend past the 80-column limit")); - -It is safe to omit the braces in the code above, since the -further-indented second body line makes it obvious that this is -still a single-statement body. - -To reiterate, don't do this: - -:: - - if (expr) // BAD: no braces around... - while (expr_2) { // ... a multi-line body - ... - } - -Do this, instead: - -:: - - if (expr) { - while (expr_2) { - ... - } - } - -However, there is one exception in the other direction, when even -a one-line block should have braces. That occurs when that -one-line, brace-less block is an ``if`` or ``else`` block, and the -counterpart block **does** use braces. In that case, put braces -around both blocks. Also, if the ``else`` block is much shorter -than the ``if`` block, consider negating the ``if``-condition and -swapping the bodies, putting the short block first and making the -longer, multi-line block be the ``else`` block. - -:: - - if (expr) { - ... - ... - } - else - x = y; // BAD: braceless "else" with braced "then", - // and short block last - - if (expr) - x = y; // BAD: braceless "if" with braced "else" - else { - ... - ... - } - -Keeping braces consistent and putting the short block first is -preferred, especially when the multi-line body is more than a few -lines long, because it is easier to read and grasp the semantics -of an if-then-else block when the simpler block occurs first, -rather than after the more involved block: - -:: - - if (!expr) { - x = y; // putting the smaller block first is more readable - } else { - ... - ... - } - -But if negating a complex condition is too ugly, then at least add -braces: - -:: - - if (complex expr not worth negating) { - ... - ... - } else { - x = y; - } - -Use hanging braces for compound statements: the opening brace of a -compound statement should be on the same line as the condition -being tested. Only top-level function bodies, nested scopes, and -compound structure declarations should ever have { on a line by -itself. - -:: - - void - foo(int a, int b) - { // correct - function body - int 2d[][] = { - { // correct - complex initialization - 1, 2, - }, - }; - if (a) - { // BAD: compound brace on its own line - do_stuff(); - } - { // correct - nested scope - int tmp; - if (a < b) { // correct - hanging brace - tmp = b; - b = a; - a = tmp; - } - } - } - -Conditional expressions ------------------------ - -For readability reasons new code should avoid shortening -comparisons to 0 for numeric types. Boolean and pointer -comparisions may be shortened. All long forms are okay: - -:: - - virFooPtr foos = NULL; - size nfoos = 0; - bool hasFoos = false; - - GOOD: - if (!foos) - if (!hasFoos) - if (nfoos == 0) - if (foos == NULL) - if (hasFoos == true) - - BAD: - if (!nfoos) - if (nfoos) - -New code should avoid the ternary operator as much as possible. -Specifically it must never span more than one line or nest: - -:: - - BAD: - char *foo = baz ? - virDoSomethingReallyComplex(driver, vm, something, baz->foo) : - NULL; - - char *foo = bar ? bar->baz ? bar->baz->foo : "nobaz" : "nobar"; - -Preprocessor ------------- - -Macros defined with an ALL_CAPS name should generally be assumed -to be unsafe with regards to arguments with side-effects (that is, -MAX(a++, b--) might increment a or decrement b too many or too few -times). Exceptions to this rule are explicitly documented for -macros in viralloc.h and virstring.h. - -For variadic macros, stick with C99 syntax: - -:: - - #define vshPrint(_ctl, ...) fprintf(stdout, __VA_ARGS__) - -Use parenthesis when checking if a macro is defined, and use -indentation to track nesting: - -:: - - #if defined(HAVE_POSIX_FALLOCATE) && !defined(HAVE_FALLOCATE) - # define fallocate(a, ignored, b, c) posix_fallocate(a, b, c) - #endif - -C types -------- - -Use the right type. - -Scalars -~~~~~~~ - -- If you're using ``int`` or ``long``, odds are good that there's - a better type. -- If a variable is counting something, be sure to declare it with - an unsigned type. -- If it's memory-size-related, use ``size_t`` (use ``ssize_t`` - only if required). -- If it's file-size related, use uintmax_t, or maybe ``off_t``. -- If it's file-offset related (i.e., signed), use ``off_t``. -- If it's just counting small numbers use ``unsigned int``; (on - all but oddball embedded systems, you can assume that that type - is at least four bytes wide). -- If a variable has boolean semantics, give it the ``bool`` type - and use the corresponding ``true`` and ``false`` macros. -- In the unusual event that you require a specific width, use a - standard type like ``int32_t``, ``uint32_t``, ``uint64_t``, - etc. -- While using ``bool`` is good for readability, it comes with - minor caveats: - - - Don't use ``bool`` in places where the type size must be - constant across all systems, like public interfaces and - on-the-wire protocols. Note that it would be possible - (albeit wasteful) to use ``bool`` in libvirt's logical wire - protocol, since XDR maps that to its lower-level ``bool_t`` - type, which **is** fixed-size. - - Don't compare a bool variable against the literal, ``true``, - since a value with a logical non-false value need not be - ``1``. I.e., don't write ``if (seen == true) ...``. Rather, - write ``if (seen)...``. - -Of course, take all of the above with a grain of salt. If you're -about to use some system interface that requires a type like -``size_t``, ``pid_t`` or ``off_t``, use matching types for any -corresponding variables. - -Also, if you try to use e.g., ``unsigned int`` as a type, and that -conflicts with the signedness of a related variable, sometimes -it's best just to use the **wrong** type, if *pulling the thread* -and fixing all related variables would be too invasive. - -Finally, while using descriptive types is important, be careful -not to go overboard. If whatever you're doing causes warnings, or -requires casts, then reconsider or ask for help. - -Pointers -~~~~~~~~ - -Ensure that all of your pointers are *const-correct*. Unless a -pointer is used to modify the pointed-to storage, give it the -``const`` attribute. That way, the reader knows up-front that this -is a read-only pointer. Perhaps more importantly, if we're -diligent about this, when you see a non-const pointer, you're -guaranteed that it is used to modify the storage it points to, or -it is aliased to another pointer that is. - -Attribute annotations ---------------------- - -Use the following annotations to help the compiler and/or static -analysis tools understand the code better: - -+-------------------------------+------------------------------------------------------------+ -| Macro | Meaning | -+===============================+============================================================+ -| ``ATTRIBUTE_NONNULL`` | passing NULL for this parameter is not allowed | -+-------------------------------+------------------------------------------------------------+ -| ``ATTRIBUTE_PACKED`` | force a structure to be packed | -+-------------------------------+------------------------------------------------------------+ -| ``G_GNUC_FALLTHROUGH`` | allow code reuse by multiple switch cases | -+-------------------------------+------------------------------------------------------------+ -| ``G_GNUC_NO_INLINE`` | the function is mocked in the test suite | -+-------------------------------+------------------------------------------------------------+ -| ``G_GNUC_NORETURN`` | the function never returns | -+-------------------------------+------------------------------------------------------------+ -| ``G_GNUC_NULL_TERMINATED`` | last parameter must be NULL | -+-------------------------------+------------------------------------------------------------+ -| ``G_GNUC_PRINTF`` | validate that the formatting string matches parameters | -+-------------------------------+------------------------------------------------------------+ -| ``G_GNUC_UNUSED`` | parameter is unused in this implementation of the function | -+-------------------------------+------------------------------------------------------------+ -| ``G_GNUC_WARN_UNUSED_RESULT`` | the return value must be checked | -+-------------------------------+------------------------------------------------------------+ - -File handling -------------- - -Usage of the ``fdopen()``, ``close()``, ``fclose()`` APIs is -deprecated in libvirt code base to help avoiding double-closing of -files or file descriptors, which is particularly dangerous in a -multi-threaded application. Instead of these APIs, use the macros -from virfile.h - -- Open a file from a file descriptor: - - :: - - if ((file = VIR_FDOPEN(fd, "r")) == NULL) { - virReportSystemError(errno, "%s", - _("failed to open file from file descriptor")); - return -1; - } - /* fd is now invalid; only access the file using file variable */ - -- Close a file descriptor: - - :: - - if (VIR_CLOSE(fd) < 0) { - virReportSystemError(errno, "%s", _("failed to close file")); - } - -- Close a file: - - :: - - if (VIR_FCLOSE(file) < 0) { - virReportSystemError(errno, "%s", _("failed to close file")); - } - -- Close a file or file descriptor in an error path, without - losing the previous ``errno`` value: - - :: - - VIR_FORCE_CLOSE(fd); - VIR_FORCE_FCLOSE(file); - -String comparisons ------------------- - -Do not use the strcmp, strncmp, etc functions directly. Instead -use one of the following semantically named macros - -- For strict equality: - - :: - - STREQ(a,b) - STRNEQ(a,b) - -- For case insensitive equality: - - :: - - STRCASEEQ(a,b) - STRCASENEQ(a,b) - -- For strict equality of a substring: - - :: - - STREQLEN(a,b,n) - STRNEQLEN(a,b,n) - -- For case insensitive equality of a substring: - - :: - - STRCASEEQLEN(a,b,n) - STRCASENEQLEN(a,b,n) - -- For strict equality of a prefix: - - :: - - STRPREFIX(a,b) - -- To avoid having to check if a or b are NULL: - - :: - - STREQ_NULLABLE(a, b) - STRNEQ_NULLABLE(a, b) - -String copying --------------- - -Do not use the strncpy function. According to the man page, it -does **not** guarantee a NULL-terminated buffer, which makes it -extremely dangerous to use. Instead, use one of the replacement -functions provided by libvirt: - -:: - - virStrncpy(char *dest, const char *src, size_t n, size_t destbytes) - -The first two arguments have the same meaning as for strncpy, -namely the destination and source of the copy operation. Unlike -strncpy, the function will always copy exactly the number of bytes -requested and make sure the destination is NULL-terminated, as the -source is required to be; sanity checks are performed to ensure -the size of the destination, as specified by the last argument, is -sufficient for the operation to succeed. On success, 0 is -returned; on failure, a value <0 is returned instead. - -:: - - virStrcpy(char *dest, const char *src, size_t destbytes) - -Use this variant if you know you want to copy the entire src -string into dest. - -:: - - virStrcpyStatic(char *dest, const char *src) - -Use this variant if you know you want to copy the entire src -string into dest **and** you know that your destination string is -a static string (i.e. that sizeof(dest) returns something -meaningful). Note that this is a macro, so arguments could be -evaluated more than once. - -:: - - dst = g_strdup(src); - dst = g_strndup(src, n); - -You should avoid using strdup or strndup directly as they do not -handle out-of-memory errors, and do not allow a NULL source. Use -``g_strdup`` and ``g_strndup`` from GLib which abort on OOM and -handle NULL source by returning NULL. - -Variable length string buffer ------------------------------ - -If there is a need for complex string concatenations, avoid using -the usual sequence of malloc/strcpy/strcat/snprintf functions and -make use of either the -`GString `__ -type from GLib or the virBuffer API. If formatting XML or QEMU -command line is needed, use the virBuffer API described in -virbuffer.h, since it has helper functions for those. - -Typical usage is as follows: - -:: - - char * - somefunction(...) - { - g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - - ... - - virBufferAddLit(&buf, "\n"); - - ... - - if (some_error) - return NULL; /* g_auto will free the memory used so far */ - - ... - - virBufferAddLit(&buf, "\n"); - - ... - - if (virBufferCheckError(&buf) < 0) - return NULL; - - return virBufferContentAndReset(&buf); - } - -Include files -------------- - -There are now quite a large number of include files, both libvirt -internal and external, and system includes. To manage all this -complexity it's best to stick to the following general plan for -all \*.c source files: - -:: - - /* - * Copyright notice - * .... - * .... - * .... - * - */ - - #include Must come first in every file. - - #include Any system includes you need. - #include - #include - - #if WITH_NUMACTL Some system includes aren't supported - # include everywhere so need these #if guards. - #endif - - #include "internal.h" Include this first, after system includes. - - #include "util.h" Any libvirt internal header files. - #include "buf.h" - - static int - myInternalFunc() The actual code. - { - ... - -Of particular note: **Do not** include libvirt/libvirt.h, -libvirt/virterror.h, libvirt/libvirt-qemu.h, or -libvirt/libvirt-lxc.h. They are included by "internal.h" already -and there are some special reasons why you cannot include these -files explicitly. One of the special cases, "libvirt/libvirt.h" is -included prior to "internal.h" in "remote_protocol.x", to avoid -exposing \*_LAST enum elements. - -Printf-style functions ----------------------- - -Whenever you add a new printf-style function, i.e., one with a -format string argument and following "..." in its prototype, be -sure to use gcc's printf attribute directive in the prototype. For -example, here's the one for virCommandAddEnvFormat in -vircommand.h: - -:: - - void virCommandAddEnvFormat(virCommandPtr cmd, const char *format, ...) - G_GNUC_PRINTF(2, 3); - -This makes it so gcc's -Wformat and -Wformat-security options can -do their jobs and cross-check format strings with the number and -types of arguments. - -When printing to a string, consider using GString or virBuffer for -incremental allocations, g_strdup_printf for a one-shot -allocation, and g_snprintf for fixed-width buffers. Only use -g_sprintf, if you can prove the buffer won't overflow. - -Error message format --------------------- - -Error messages visible to the user should be short and -descriptive. All error messages are translated using gettext and -thus must be wrapped in ``_()`` macro. To simplify the translation -work, the error message must not be concatenated from various -parts. To simplify searching for the error message in the code the -strings should not be broken even if they result into a line -longer than 80 columns and any formatting modifier should be -enclosed by quotes or other obvious separator. If a string used -with ``%s`` can be NULL the NULLSTR macro must be used. - -:: - - GOOD: virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to connect to remote host '%s'"), hostname) - - BAD: virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to %s to remote host '%s'"), - "connect", hostname); - - BAD: virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to connect " - "to remote host '%s'), - hostname); - -Use of goto ------------ - -The use of goto is not forbidden, and goto is widely used -throughout libvirt. While the uncontrolled use of goto will -quickly lead to unmaintainable code, there is a place for it in -well structured code where its use increases readability and -maintainability. In general, if goto is used for error recovery, -it's likely to be ok, otherwise, be cautious or avoid it all -together. - -The typical use of goto is to jump to cleanup code in the case of -a long list of actions, any of which may fail and cause the entire -operation to fail. In this case, a function will have a single -label at the end of the function. It's almost always ok to use -this style. In particular, if the cleanup code only involves -free'ing memory, then having multiple labels is overkill. g_free() -and most of the functions named XXXFree() in libvirt is required -to handle NULL as its arg. This does not apply to libvirt's public -APIs. Thus you can safely call free on all the variables even if -they were not yet allocated (yes they have to have been -initialized to NULL). This is much simpler and clearer than having -multiple labels. Note that most of libvirt's type declarations can -be marked with either ``g_autofree`` or ``g_autoptr`` which uses -the compiler's ``__attribute__((cleanup))`` that calls the -appropriate free function when the variable goes out of scope. - -There are a couple of signs that a particular use of goto is not -ok: - -- You're using multiple labels. If you find yourself using - multiple labels, you're strongly encouraged to rework your code - to eliminate all but one of them. -- The goto jumps back up to a point above the current line of - code being executed. Please use some combination of looping - constructs to re-execute code instead; it's almost certainly - going to be more understandable by others. One well-known - exception to this rule is restarting an i/o operation following - EINTR. -- The goto jumps down to an arbitrary place in the middle of a - function followed by further potentially failing calls. You - should almost certainly be using a conditional and a block - instead of a goto. Perhaps some of your function's logic would - be better pulled out into a helper function. - -Although libvirt does not encourage the Linux kernel wind/unwind -style of multiple labels, there's a good general discussion of the -issue archived at -`KernelTrap `__ - -When using goto, please use one of these standard labels if it -makes sense: - -:: - - error: A path only taken upon return with an error code - cleanup: A path taken upon return with success code + optional error - no_memory: A path only taken upon return with an OOM error code - retry: If needing to jump upwards (e.g., retry on EINTR) - -Top-level labels should be indented by one space (putting them on -the beginning of the line confuses function context detection in -git): - -:: - - int foo() - { - /* ... do stuff ... */ - cleanup: - /* ... do other stuff ... */ - } - Libvirt committer guidelines ============================ -- GitLab