[RFC PATCH 0/4] libxl: CODING_STYLE improvements

[RFC PATCH 0/4] libxl: CODING_STYLE improvements

[Ian Jackson]

The structural considerations, error handling patterns, and so on,
have remained undocumented.  This has been a problem during recent
code submissions and reviews.

In this series I have attempted to document current best practice.
The first patch contains much new material and the others are minor
and consequential fixes.

 1/4 libxl: CODING_STYLE: Much new material
 2/4 libxl: CODING_STYLE: Deprecate `error' for out blocks
 3/4 libxl: CODING_STYLE: Mention function out parameters
 4/4 libxl: CODING_STYLE: Discuss existing style problems

Thanks,
Ian.

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

[Ian Jackson]

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) */
+
+  uint32_t domid;
+  libxl__gc *gc;
+  libxl__egc *egc;
+  libxl__ao *ao;
+
+  libxl_foo_bar_state *fbs;    /* local variable */
+  libxl_foo_bar_state foo_bar; /* inside another state struct */
+
+
+CONVENIENCE MACROS
+------------------
+
+There are a number of convenience macros which shorten the program and
+avoid opportunity for mistakes.  In some cases non-use of the macros
+produces functional bugs or incorrect error handling.  Use the macros
+whenever they are applicable.  For example:
+
+ Usually, don't use:     Instead, use (see libxl_internal.h):
+  libxl__log[v]           LOG, LOGE, LOGEV
+  libxl__sprintf          GCSPRINTF
+  libxl__*alloc et al.    GCNEW, GCNEW_ARRAY, GCREALLOC_ARRAY
+  malloc et al.           GCNEW, GCNEW_ARRAY, GCREALLOC_ARRAY with NOGC
+  isalnum etc. directly   CTYPE
+  libxl__ctx_[un]lock     CTX_LOCK, CTX_UNLOCK
+  gc=...; ao=...;         EGC_GC, AO_GC, STATE_AO_GC
+  explicit gc creation    GC_INIT, GC_FREE
+
+
+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:
+            int rc;
+
+  * There is only one error cleanup path out of the function.  It
+    starts with a label `out:'.  That error cleanup path checks for
+    each allocated resource and frees it iff necessary.  It then
+    returns rc.  For example,
+         out:
+             if (evg) libxl__evdisable_disk_eject(gc, evg);
+             if (nullfd >= 0) close(nullfd);
+             return rc;
+
+  * 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; }
+
+  * If a resource is freed in the main body of the function (for
+    example, in a loop), the corresponding variable has to be reset to
+    the sentinel at the point where it's freed.
+
+Whether to use the `out' path for successful returns as well as error
+returns is a matter of taste and convenience for the specific
+function.  Not reusing the out path is fine if the duplicated function
+exit code is only `CTX_UNLOCK; GC_FREE;' (or similar).
+
+If you reuse the `out' path for successful returns, there may be
+resources which are to be returned to the caller rather than freed.
+In that case you have to reset the local variable to `nothing here',
+to avoid the resource being freed on the out path.  That resetting
+should be done immediately after the resource value is stored at the
+applicable _r function parameter (or equivalent).  Do not test `rc' in
+the out section, to discover whether to free things.
+
+The uses of the single-line formatting in the examples above are
+permitted exceptions to the usual libxl code formatting rules.
+
+
+
+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
+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.
+
+
+ASYNCHRONOUS/LONG-RUNNING OPERATIONS
+------------------------------------
+
+All long-running operations in libxl need to use the asynchronous
+operation machinery.  Consult the programmer documentation in
+libxl_internal.h for details - search for "Machinery for asynchronous
+operations".
+
+The code for asynchronous operations should be laid out in
+chronological order.  That is, where there is a chain of callback
+functions, each subsequent function should be, textually, the next
+function in the file.  This will normally involve predeclaring the
+callback functions.  Synchronous helper functions should be separated
+out into a section preceding the main callback chain.
+
+Control flow arrangements in asynchronous operations should be made as
+simple as possible, because it can otherwise be very hard to see
+through the tangle.
+
+
+When inventing a new sub-operation in asynchronous code, consider
+whether to structure it formally as a sub-operation with its own state
+structure.  (See, for example, libxl__datacopier_*.)
+
+An ao-suboperation state structure should contain, in this order:
+  * fields that the caller must fill in, and which are,
+    effectively, the parameters to the operation, including:
+      - libxl__ao *ao
+      - the callback function pointer(s), which
+        should be named callback or callback_*.
+  * shared information fields or ones used for returning information
+    to the calling operation
+  * private fields
+These sections should be clearly demarcated by comments.
+
+An asynchronous operation should normally have an idempotent stop or
+cancel function.  It should normally also have an _init function for
+its state struct, which arranges that the stop is a no-op.
+
+The permitted order of calls into your ao operation's methods must be
+documented in comments, if it is nontrivial.
+
+
+When using an ao sub-operation, you should normally:
+ * Physically include the sub-operation state struct in your
+   own state struct;
+ * Use CONTAINER_OF to find your own state struct at the start of
+   your implementations of the sub-operation callback functions;
+ * Unconditionally initialise the sub-operation's struct (with its
+   _init method) in your own _init method.
+ * Unconditionally cancel or destroy the sub-operation in your own
+   cancel or destroy method.
+
+
+FORMATTING AND NAMING
+---------------------
+
 Blatantly copied from qemu and linux with few modifications.
 
 
-- 
1.7.10.4

[PATCH 2/4] libxl: CODING_STYLE: Deprecate `error' for out blocks

[Ian Jackson]

We should have only one name for this and `out' is more prevalent.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/CODING_STYLE |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/CODING_STYLE b/tools/libxl/CODING_STYLE
index 3e72852..8dcc22b 100644
--- a/tools/libxl/CODING_STYLE
+++ b/tools/libxl/CODING_STYLE
@@ -239,7 +239,7 @@ variable that is used to hold a temporary value.
 
 Local variables used to store return values should have descriptive name
 like "rc" or "ret". Following the same reasoning the label used as exit
-path should be called "out" or "error".
+path should be called "out".
 
 Variables, type names and function names are
 lower_case_with_underscores.
-- 
1.7.10.4

[PATCH 3/4] libxl: CODING_STYLE: Mention function out parameters

[Ian Jackson]

We seem to use both `_r' and `_out'.  Document both.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/CODING_STYLE |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/libxl/CODING_STYLE b/tools/libxl/CODING_STYLE
index 8dcc22b..68d769d 100644
--- a/tools/libxl/CODING_STYLE
+++ b/tools/libxl/CODING_STYLE
@@ -241,6 +241,9 @@ Local variables used to store return values should have descriptive name
 like "rc" or "ret". Following the same reasoning the label used as exit
 path should be called "out".
 
+Function arguments which are used to return values to the caller
+should be suffixed `_r' or `_out'.
+
 Variables, type names and function names are
 lower_case_with_underscores.
 Type names and function names use the prefix libxl__ when internal to
-- 
1.7.10.4

[PATCH 4/4] libxl: CODING_STYLE: Discuss existing style problems

[Ian Jackson]

Document that:
 - the existing code is not all confirming yet
 - code should conform
 - we will sometimes accept patches with nonconforming elements if
   they don't make matters worse.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/CODING_STYLE |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/tools/libxl/CODING_STYLE b/tools/libxl/CODING_STYLE
index 68d769d..c836f3c 100644
--- a/tools/libxl/CODING_STYLE
+++ b/tools/libxl/CODING_STYLE
@@ -2,6 +2,21 @@ LIBXENLIGHT CODING STYLE
 ========================
 
 
+AN APOLOGY AND WARNING
+----------------------
+
+Much of the code in libxl does not yet follow this coding style
+document in every respect.  However, new code is expected to conform.
+
+Patches to improve the style of existing code are welcome.  Please
+separate these out from functional changes.
+
+If it is not feasible to conform fully to the style while patching old
+code, without doing substantial style reengineering first, we may
+accept patches which contain nonconformant elements, provided that
+they don't make the coding style problem worse overall.
+
+
 MEMORY ALLOCATION
 -----------------
 
-- 
1.7.10.4

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

[Wei Liu]

On Wed, Nov 05, 2014 at 02:43:47PM +0000, Ian Jackson wrote:
[...]
> +  * 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; }
> +
> +  * If a resource is freed in the main body of the function (for
> +    example, in a loop), the corresponding variable has to be reset to
> +    the sentinel at the point where it's freed.
> +

However useful this one is, in practice this looks very tedious to
enforce and easy to slip under reviewer's eyes.

> +Whether to use the `out' path for successful returns as well as error
> +returns is a matter of taste and convenience for the specific
> +function.  Not reusing the out path is fine if the duplicated function
> +exit code is only `CTX_UNLOCK; GC_FREE;' (or similar).
> +
> +If you reuse the `out' path for successful returns, there may be
> +resources which are to be returned to the caller rather than freed.
> +In that case you have to reset the local variable to `nothing here',
> +to avoid the resource being freed on the out path.  That resetting
> +should be done immediately after the resource value is stored at the
> +applicable _r function parameter (or equivalent).  Do not test `rc' in
> +the out section, to discover whether to free things.
> +
> +The uses of the single-line formatting in the examples above are
> +permitted exceptions to the usual libxl code formatting rules.
> +
> +
> +
> +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.
> +

Is it worth mentioning that IDL-defined structures already have _dispose
and _init autogenerated, and _dispose is not idempotent at the moment?
In fact, the generated _dispose function explicitly poisons the
structure.

Wei.

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

[Ian Campbell]

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

Re: [PATCH 4/4] libxl: CODING_STYLE: Discuss existing style problems

[Ian Campbell]

On Wed, 2014-11-05 at 14:43 +0000, Ian Jackson wrote:
> +If it is not feasible to conform fully to the style while patching old
> +code, without doing substantial style reengineering first, we may
> +accept patches which contain nonconformant elements, provided that
> +they don't make the coding style problem worse overall.

I'd perhaps be a little more explicit and say that the exceptional code
should conform to the (probably implicit) prevailing style in the area
so far as that is possible.

Ian.

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

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 1/4] libxl: CODING_STYLE: Much new material"):
> On Wed, 2014-11-05 at 14:43 +0000, Ian Jackson wrote:
> > +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?

I have avoided specifying `ret' here because ret is used
indiscriminately for pretty much any int returned from a function.

> > +  * 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...)

Fixed.

> > +  * 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 ;-)

Example replaced with:
    char *dompath = libxl__xs_get_dompath(gc, bl->domid);
    if (!dompath) { rc = ERROR_FAIL; goto out; }

> > +Nontrivial data structures (in structs) should come with an idempotent
> > +_destroy function, which must free all resources associated with the
> 
> _dispose.

Fixed (twice).

Thanks,
Ian.

Re: [PATCH 4/4] libxl: CODING_STYLE: Discuss existing style problems

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 4/4] libxl: CODING_STYLE: Discuss existing style problems"):
> On Wed, 2014-11-05 at 14:43 +0000, Ian Jackson wrote:
> > +If it is not feasible to conform fully to the style while patching old
> > +code, without doing substantial style reengineering first, we may
> > +accept patches which contain nonconformant elements, provided that
> > +they don't make the coding style problem worse overall.
> 
> I'd perhaps be a little more explicit and say that the exceptional code
> should conform to the (probably implicit) prevailing style in the area
> so far as that is possible.

Yes, done.

Ian.

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

[Ian Campbell]

On Wed, 2014-11-05 at 14:43 +0000, Ian Jackson wrote:

Sorry to be a bit late to the game, do I win a prize? ;-)

[...]
> + Usually, don't use:     Instead, use (see libxl_internal.h):
> +  libxl__log[v]           LOG, LOGE, LOGEV
> +  libxl__sprintf          GCSPRINTF
> +  libxl__*alloc et al.    GCNEW, GCNEW_ARRAY, GCREALLOC_ARRAY
> +  malloc et al.           GCNEW, GCNEW_ARRAY, GCREALLOC_ARRAY with NOGC

This last one isn't possible, because those macros do not take an explicit
gc (they assume gc in the calling context) so you can't pass them NOGC.

Ian.