Re: [Xen-devel] [PATCH for-4.6 5/8] tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Andrew Cooper writes ("[PATCH for-4.6 5/8] tools/libxl: Save and restore 
EMULATOR_XENSTORE_DATA content"):
> 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.

> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
> CC: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
>  tools/libxl/libxl_dom.c      |  141 
> ++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_internal.h |    4 ++
>  2 files changed, 145 insertions(+)
> 
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 5555fea..885eb5e 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -1151,6 +1151,65 @@ int libxl__toolstack_restore(uint32_t domid, const 
> uint8_t *ptr,
>      return ret;
>  }
>  
> +int libxl__restore_emulator_xenstore_data(libxl__domain_create_state *dcs,
> +                                          const char *ptr, uint32_t size)
> +{
> +    STATE_AO_GC(dcs->ao);
> +    const char *key = ptr, *val;
> +    int rc;
> +
> +    const uint32_t domid = dcs->guest_domid;
> +    const uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
> +    const char *xs_root = libxl__device_model_xs_path(gc, dm_domid, domid\
, "");

I see wrap damage due to your overly long lines.  (Throughout.)

> +
> +    while ( size ) {

Coding style.  (Throughout.)

> +        /* Sanitise key. */
> +        size_t keylen = strnlen(key, size);

I think this code suggests to me that the format chosen is not very
nice.  But, anyway:

Can we please have a black box subfunction which extracts the next
nul-terminated string from this buffer ?  That would isolate all the
pointer arithmetic and give it a formal interface.

You might consider whether it is better to work with
  const char *end = ptr + size;
since that avoids the need to remember to update both ptr and size
whenever walking through the array (a common source of security bugs).

> +int libxl__save_emulator_xenstore_data(libxl__domain_suspend_state *dss,
> +                                       uint8_t **callee_buf,
> +                                       uint32_t *callee_len)
> +{
...
> +    /* &foo[rel_start] is the xenstore path starting at the 'p' of physmap */
> +    rel_start = strlen(xs_path) - 7;

This is horrible.

> +    entries = libxl__xs_directory(gc, 0, xs_path, &nr_entries);
> +    if (!entries || nr_entries == 0) { rc = 0; goto out; }
> +
> +    for (i = 0; i < nr_entries; ++i) {
> +        const char *start_addr, *start_addr_val,
> +            *size, *size_val, *name, *name_val;
> +
> +        start_addr = libxl__device_model_xs_path(
> +            gc, dm_domid, domid, "/physmap/%s/start_addr", entries[i]);
> +        size = libxl__device_model_xs_path(
> +            gc, dm_domid, domid, "/physmap/%s/size", entries[i]);
> +        name = libxl__device_model_xs_path(
> +            gc, dm_domid, domid, "/physmap/%s/name", entries[i]);

I don't understand why you don't copy the whole subdirectory.  This
needs to be explained.

If you do want to copy only some of the keys, this thing where you do
tiny bits of the work, each time three times, is a very strange
structure.

> +        /*
> +         * Appends 's' to 'buf', including a NUL teriminator.  Mutates
> +         * 'buf', 'ptr' and 'len' in scope.
> +         */
> +#define APPEND_STRING(s) ({                                     \

Please, there is no need for this to be a macro.  If you need it, make
it a function.  Yes, you'll have to pass some of its state to it.  But
that means you may (usefully) write down what its assumptions are.

Also, if you restructure this I think this macro will vanish.

> +                size_t sz = strlen(s) + 1;                      \
> +                ptr = realloc(buf, len + sz);                   \
> +                if (!ptr) { rc = ERROR_NOMEM; goto out; }       \

Please use the gc.  I guess you are going to tell me that you can't
use the gc because of the remus memory leak problem.  Do I need to go
and retrofit the per-iteration sub-ao myself ?

> +_hidden int libxl__save_emulator_xenstore_data(libxl__domain_suspend_state 
> *dss,
> +                                               uint8_t **buf, uint32_t *len);
> +_hidden int libxl__restore_emulator_xenstore_data(
> +    libxl__domain_create_state *dcs, const char *ptr, uint32_t size);

( in wrong place.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel