From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH 1/4] libxl: CODING_STYLE: Much new material Date: Mon, 10 Nov 2014 12:27:33 +0000 Message-ID: <1415622453.25176.4.camel@citrix.com> References: <1415198630-29937-1-git-send-email-ian.jackson@eu.citrix.com> <1415198630-29937-2-git-send-email-ian.jackson@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1415198630-29937-2-git-send-email-ian.jackson@eu.citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: xen-devel@lists.xensource.com, stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org 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 > --- > 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