* [PATCH 1/4] libxl: CODING_STYLE: Much new material
2014-11-14 18:52 [PATCH for-4.5 v2 0/4] libxl: CODING_STYLE Ian Jackson
@ 2014-11-14 18:52 ` Ian Jackson
2014-11-14 18:52 ` [PATCH 2/4] libxl: CODING_STYLE: Deprecate `error' for out blocks Ian Jackson
` (4 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Ian Jackson @ 2014-11-14 18:52 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Wei Liu, Ian Campbell
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>
---
v2: Fix typo `codem'.
Fix example to use libxl__xs_get_dompath not something that
ought to be GCSTRDUP.
Talk about *_dispose not *_destroy.
---
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..32d058f 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 code, 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:
+ char *dompath = libxl__xs_get_dompath(gc, bl->domid);
+ if (!dompath) { rc = ERROR_FAIL; 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
+_dispose 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 _dispose 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
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 4/4] libxl: CODING_STYLE: Discuss existing style problems
2014-11-14 18:52 [PATCH for-4.5 v2 0/4] libxl: CODING_STYLE Ian Jackson
` (2 preceding siblings ...)
2014-11-14 18:52 ` [PATCH 3/4] libxl: CODING_STYLE: Mention function out parameters Ian Jackson
@ 2014-11-14 18:52 ` Ian Jackson
2014-11-14 18:56 ` [PATCH for-4.5 v2 0/4] libxl: CODING_STYLE Konrad Rzeszutek Wilk
2014-11-15 14:18 ` Wei Liu
5 siblings, 0 replies; 8+ messages in thread
From: Ian Jackson @ 2014-11-14 18:52 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Wei Liu, Ian Campbell
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>
---
v2: When not fixing up existing style problems, new code should
conform to prevailing existing style nearby.
---
tools/libxl/CODING_STYLE | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/tools/libxl/CODING_STYLE b/tools/libxl/CODING_STYLE
index 9a1d6e0..f5b5890 100644
--- a/tools/libxl/CODING_STYLE
+++ b/tools/libxl/CODING_STYLE
@@ -2,6 +2,24 @@ 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.
+
+In this case, the new code should conform to the prevailing style in
+the area being touched.
+
+
MEMORY ALLOCATION
-----------------
--
1.7.10.4
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH for-4.5 v2 0/4] libxl: CODING_STYLE
2014-11-14 18:52 [PATCH for-4.5 v2 0/4] libxl: CODING_STYLE Ian Jackson
` (3 preceding siblings ...)
2014-11-14 18:52 ` [PATCH 4/4] libxl: CODING_STYLE: Discuss existing style problems Ian Jackson
@ 2014-11-14 18:56 ` Konrad Rzeszutek Wilk
2014-11-15 14:18 ` Wei Liu
5 siblings, 0 replies; 8+ messages in thread
From: Konrad Rzeszutek Wilk @ 2014-11-14 18:56 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, Wei Liu, Ian Campbell
On Fri, Nov 14, 2014 at 06:52:37PM +0000, Ian Jackson wrote:
> The structural considerations, error handling patterns, and so on, in
> libxl 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
>
> * = Modified patches, to take into account review comments from
> Ian Campbell.
>
> I would like to suggest that this ought to go into 4.5 because:
> - it would be useful documentation to assist with any nontrivial
> bugfixes that need to be made to libxl
> - there isn't any risk of it actually breaking anything, because
> it's just doc changes.
Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> Thanks,
> Ian.
^ permalink raw reply [flat|nested] 8+ messages in thread