Re: [PATCH for-4.6 v2 3/6] tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content

[Ian Campbell <ian.campbell@citrix.com>]

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 <andrew.cooper3@citrix.com>
> ---
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> 
> 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.