From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH for-4.6 v2 3/6] tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content Date: Tue, 4 Aug 2015 10:55:57 +0100 Message-ID: <1438682157.31129.82.camel@citrix.com> References: <1438617395-20329-1-git-send-email-andrew.cooper3@citrix.com> <1438617395-20329-4-git-send-email-andrew.cooper3@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1438617395-20329-4-git-send-email-andrew.cooper3@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: Andrew Cooper , Xen-devel Cc: Wei Liu , Ian Jackson List-Id: xen-devel@lists.xenproject.org On Mon, 2015-08-03 at 16:56 +0100, Andrew Cooper wrote: > The new EMULATOR_XENSTORE_DATA content is a sequence of NUL terminated > key/value strings, with the key relative to the device model's xenstore > tree. > > A sample might look like (as decoded by verify-stream-v2): > > Emulator Xenstore Data (Qemu Upstream, idx 0) > 'physmap/1f00000/start_addr' = 'f0000000' > 'physmap/1f00000/size' = '800000' > 'physmap/1f00000/name' = 'vga.vram' > > This patch introduces libxl helpers to save and restore this new format, > which reimplement the existing libxl__toolstack_{save,restore}() logic. > > Signed-off-by: Andrew Cooper > --- > CC: Ian Campbell > CC: Ian Jackson > CC: Wei Liu > > v2: > * Factor out pointer arithmatic into helper functions > > NB: I was unable to remove the rel_start bit I think you got almost all the way there and it is a pretty easy series of changes to get rid of it. See below. > , but its use has changed > slightly and is hopefully more clear now. > --- > tools/libxl/libxl_dom.c | 138 ++++++++++++++++++++++++++++++++++++++++++ > tools/libxl/libxl_internal.h | 4 ++ > 2 files changed, 142 insertions(+) > > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c > index 5555fea..feed7b6 100644 > --- a/tools/libxl/libxl_dom.c > +++ b/tools/libxl/libxl_dom.c > @@ -1151,6 +1151,76 @@ int libxl__toolstack_restore(uint32_t domid, const uint8_t *ptr, > return ret; > } > > +/* > + * Inspect the buffer between start and end, and return a pointer to the > + * character following the NUL terminator of start, or NULL if start is not > + * terminated before end. > + */ > +static const char *_next_string(const char *start, const char *end) I can never remember who owns the _[a-z].* namespace and I can't be bothered to look it up right now, but even if it is the application I think there is no real reason to use it here or for append_string anyway, their names are OK for static functions without the leading _. > +int libxl__save_emulator_xenstore_data(libxl__domain_suspend_state *dss, > + char **callee_buf, > + uint32_t *callee_len) > +{ > + STATE_AO_GC(dss->ao); > + const char *xs_path; > + char **entries, *buf = NULL; > + unsigned int nr_entries, rel_start, i, j, len = 0; > + int rc; > + > + const uint32_t domid = dss->domid; > + const uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid); > + > + rel_start = strlen(libxl__device_model_xs_path(gc, dm_domid, domid, "/")); Instead of allocating this string just to count it's length, keep the string itself as "xs_path" and... > + /* path + rel_start is the xenstore path start from the dm root. */ > + > + xs_path = libxl__device_model_xs_path(gc, dm_domid, domid, "/physmap"); > + if (!xs_path) { rc = 0; goto out; } ... drop this, and ... > + > + entries = libxl__xs_directory(gc, 0, xs_path, &nr_entries); s|xs_path|GCSPRINTF("%s/%s", xs_path, "physmap")| (maybe fiddle with where the / lives depending on what xs_path looks like). > + if (!entries || nr_entries == 0) { rc = 0; goto out; } > + > + for (i = 0; i < nr_entries; ++i) { > + static const char *const physmap_subkeys[] = { > + "start_addr", "size", "name" > + }; > + > + for (j = 0; j < ARRAY_SIZE(physmap_subkeys); ++j) { > + const char *key = libxl__device_model_xs_path(gc, dm_domid, domid, > + "/physmap/%s/%s", entries[i], physmap_subkeys[j]); ... make this: GCSPRINTF("physmap/%s/%s", entries[i], subkeys[j]). and... > + if (!key) { rc = ERROR_FAIL; goto out; } > + > + const char *val = libxl__xs_read(gc, XBT_NULL, key); ... s|key|GCSPRINTF("%s/%s", xs_path, key)| (or use another variable for clarity if you prefer) (Before you complain about the second allocation libxl__device_model_xs_path is two anyway) Then... > + > + if (!val) { rc = ERROR_FAIL; goto out; } > + > + _append_string(gc, &buf, &len, key + rel_start); append_string(gc, ..., key) > + _append_string(gc, &buf, &len, val); append_string(gc, ..., val) Ian.