Re: [PATCH 1/4] libxl: CODING_STYLE: Much new material

[Ian Campbell <Ian.Campbell@citrix.com>]

On Wed, 2014-11-05 at 14:43 +0000, Ian Jackson wrote:
> Discuss:
> 
>     Memory allocation
>     Conventional variable names
>     Convenience macros
>     Error handling
>     Idempotent data structure construction/destruction
>     Asynchronous/long-running operations
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  tools/libxl/CODING_STYLE |  169 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 168 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/CODING_STYLE b/tools/libxl/CODING_STYLE
> index 110a48f..3e72852 100644
> --- a/tools/libxl/CODING_STYLE
> +++ b/tools/libxl/CODING_STYLE
> @@ -1,6 +1,173 @@
> -Libxenlight Coding Style
> +LIBXENLIGHT CODING STYLE
>  ========================
>  
> +
> +MEMORY ALLOCATION
> +-----------------
> +
> +Memory allocation for libxl-internal purposes should normally be done
> +with the provided gc mechanisms; there is then no need to free.  See
> +"libxl memory management" in libxl.h.
> +
> +
> +CONVENTIONAL VARIABLE NAMES
> +---------------------------
> +
> +The following local variable names should be used where applicable:
> +
> +  int rc;    /* a libxl error code - and not anything else */
> +  int r;     /* the return value from a system call (or libxc call) */

Quite a bit more "ret" for this one. Probably quite a few are being
misused as rc too, which is perhaps why you omitted it?

> +ERROR HANDLING
> +--------------
> +
> +Unless, there are good reasons to do otherwise, the following error
> +handling and cleanup paradigm should be used:
> +
> +  * All local variables referring to resources which might need
> +    cleaning up are declared at the top of the function, and
> +    initialised to a sentinel value indicating "nothing allocated".
> +    For example,
> +            libxl_evgen_disk_eject *evg = NULL;
> +            int nullfd = -1;
> +
> +  * If the function is to return a libxl error value, `rc' is
> +    used to contain the error codem, but it is NOT initialised:

I suspect this is a typo? (but then I never studied latin...)

> +  * Function calls which might fail (ie most function calls) are
> +    handled by putting the return/status value into a variable, and
> +    then checking it in a separate statement:
> +            evg->vdev = strdup(vdev);
> +            if (!evg->vdev) { rc = ERROR_NOMEM; goto out; }

A slightly dodgy example because this should be GCSTRDUP(NOGC, vdev) and
therefore can't fail ;-)

> +IDEMPOTENT DATA STRUCTURE CONSTRUCTION/DESTRUCTION
> +--------------------------------------------------
> +
> +Nontrivial data structures (in structs) should come with an idempotent
> +_destroy function, which must free all resources associated with the

_dispose.

> +data structure (but not free the struct itself).
> +
> +Such a struct should also come with an _init function which
> +initialises the struct so that _destroy is a no-op.

again