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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/07/15 12:49, Ian Jackson wrote:
> 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.

What do you recommend instead?

>
>> +    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.

I am replicating the behaviour of the old libxl__toolstack_save(), not
redesigning the logic from scratch.  (In an ideal world, all this cruft
would be limited to qemu alone, which is the sole producer and consumer,
rather than split across qemu, xenstore and libxl, but that boat has
long since sailed.)

>
>> +        /*
>> +         * 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 ?

Again, because that what older toolstack_save() did, but without the
buggy use of "ptr = realloc(ptr, ...".

On a separate thread in the 4.7 timeframe, we need to discuss the
current libxl__$FOOalloc() infrastructure.  What it currently does is
inappropriate in any daemonised context, or in any language with its own
garbage collector.

~Andrew

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