From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH] libxl: refactor toolstack save restore code Date: Wed, 17 Jun 2015 11:36:37 +0100 Message-ID: <1434537397.13744.311.camel@citrix.com> References: <1433523702-4540-1-git-send-email-wei.liu2@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Z5Ai2-0005BN-9F for xen-devel@lists.xenproject.org; Wed, 17 Jun 2015 10:36:42 +0000 In-Reply-To: <1433523702-4540-1-git-send-email-wei.liu2@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: Wei Liu Cc: Xen-devel , Ian Jackson , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org On Fri, 2015-06-05 at 18:01 +0100, Wei Liu wrote: > This patch does following things: > 1. Document v1 format. > 2. Factor out function to handle QEMU restore data and function to > handle v1 blob for restore path. > 3. Refactor save function to generate different blobs in the order > specified in format specification. > 4. Change functions to use "goto out" idiom. > > No functional changes introduced. > > Signed-off-by: Wei Liu > --- > I wrote this patch when exploring the idea of have toolstack save / restore v2 > to record max pages information. Though that idea has been abandon it wouldn't > hurt to have clearer code structure and documentation. > --- > tools/libxl/libxl_dom.c | 247 +++++++++++++++++++++++++++++------------------- > 1 file changed, 150 insertions(+), 97 deletions(-) > > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c > index 867172a..23c4691 100644 > --- a/tools/libxl/libxl_dom.c > +++ b/tools/libxl/libxl_dom.c > @@ -1032,6 +1032,15 @@ struct libxl__physmap_info { > char name[]; > }; > > +/* Bump version every time when toolstack saved data changes. > + * Different types of data are arranged in the specified order. > + * > + * Version 1: > + * uint32_t version > + * QEMU physmap data: > + * uint32_t count > + * libxl__physmap_info * count > + */ > #define TOOLSTACK_SAVE_VERSION 1 > > static inline char *restore_helper(libxl__gc *gc, uint32_t dm_domid, > @@ -1043,66 +1052,97 @@ static inline char *restore_helper(libxl__gc *gc, uint32_t dm_domid, > phys_offset, node); > } > > -int libxl__toolstack_restore(uint32_t domid, const uint8_t *buf, > - uint32_t size, void *user) > +static int libxl__toolstack_restore_qemu(libxl__gc *gc, uint32_t domid, > + const uint8_t *buf, uint32_t size) > { > - libxl__save_helper_state *shs = user; > - libxl__domain_create_state *dcs = CONTAINER_OF(shs, *dcs, shs); > - STATE_AO_GC(dcs->ao); > - int i, ret; > - const uint8_t *ptr = buf; > - uint32_t count = 0, version = 0; > - struct libxl__physmap_info* pi; > + int rc, i; > + uint32_t count; > char *xs_path; > uint32_t dm_domid; > + struct libxl__physmap_info *pi; > > - LOG(DEBUG,"domain=%"PRIu32" toolstack data size=%"PRIu32, domid, size); > - > - if (size < sizeof(version) + sizeof(count)) { > + if (size < sizeof(count)) { > LOG(ERROR, "wrong size"); > - return -1; > - } > - > - memcpy(&version, ptr, sizeof(version)); > - ptr += sizeof(version); > - > - if (version != TOOLSTACK_SAVE_VERSION) { > - LOG(ERROR, "wrong version"); > - return -1; > + rc = -1; > + goto out; Per-coding style a variable called rc must never contain anything other than a libxl error code. This function even seems to use it for both, which is even worse. It should either be called "r" or "ret" and be -1 style or it should be rc and use ERROR_FOO style, consistently on or the other. > } > > - memcpy(&count, ptr, sizeof(count)); > - ptr += sizeof(count); > + memcpy(&count, buf, sizeof(count)); > + buf += sizeof(count); Calling it ptr instead of buf would have made the patch a lot smaller and easier to see the wood for the trees etc. > +int libxl__toolstack_restore(uint32_t domid, const uint8_t *buf, > + uint32_t size, void *user) > +{ > + libxl__save_helper_state *shs = user; > + libxl__domain_create_state *dcs = CONTAINER_OF(shs, *dcs, shs); > + STATE_AO_GC(dcs->ao); > + int rc; > + const uint8_t *ptr = buf; > + uint32_t version = 0, bufsize; > + > + LOG(DEBUG,"domain=%"PRIu32" toolstack data size=%"PRIu32, domid, size); > + > + if (size < sizeof(version)) { > + LOG(ERROR, "wrong size"); > + rc = -1; As above. Ian.