[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] libxl: refactor toolstack save restore code
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 <wei.liu2@xxxxxxxxxx> > --- > 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. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |