Re: [PATCH v6 2/3] libxl: save/restore qemu's physmap

[Ian Jackson <Ian.Jackson@eu.citrix.com>]

Stefano Stabellini writes ("[Xen-devel] [PATCH v6 2/3] libxl: save/restore qemu's physmap"):
> Read Qemu's physmap from xenstore and save it using toolstack_save.
> Restore Qemu's physmap using toolstack_restore.

> +static int libxl__toolstack_restore(uint32_t domid, uint8_t *buf,
> +        uint32_t size, void *data)
...
> +    if (size < sizeof(version) + sizeof(count))
> +        return -1;
> +
> +    memcpy(&version, ptr, sizeof(version));
> +    ptr += sizeof(version);
> +
> +    if (version != TOOLSTACK_SAVE_VERSION)
> +        return -1;

Surely these error exits need log messages.

> +    for (i = 0; i < count; i++) {
> +        pi = (struct libxl__physmap_info*) ptr;
> +        ptr += sizeof(struct libxl__physmap_info) + pi->namelen;
> +
> +        ret = libxl__xs_write(gc, 0, libxl__sprintf(gc,
> +                    "/local/domain/0/device-model/%d/physmap/%"PRIx64"/start_addr",

Long line.

> +                    domid, pi->phys_offset), "%"PRIx64, pi->start_addr);
> +        if (ret)
> +            return -1;
> +        ret = libxl__xs_write(gc, 0, libxl__sprintf(gc,
> +                    "/local/domain/0/device-model/%d/physmap/%"PRIx64"/size",
> +                    domid, pi->phys_offset), "%"PRIx64, pi->size);

This whole thing contains a lot of repetitive code.  Can you perhaps
break the xs_write into a helper function and then you'd make the
repetition more explicit by writing something like:

           helper(gc, domid, "start_addr", "%"PRIx64, pi->start_addr);
           helper(gc, domid, "name",       "%"PRIx64, pi->size);
           if (pi->namelen)
                helper(gc, domid, "name",       "%s",      pi->name);

> +static int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
> +        uint32_t *len, void *data)
> +{
...
> +    *buf = calloc(1, *len);

Surely this should come from the gc.

> +        start_addr = libxl__xs_read(gc, 0, libxl__sprintf(gc,
> +                    "/local/domain/0/device-model/%d/physmap/%s/start_addr",
> +                    domid, phys_offset));
> +        size = libxl__xs_read(gc, 0, libxl__sprintf(gc,
> +                    "/local/domain/0/device-model/%d/physmap/%s/size",
> +                    domid, phys_offset));
> +        name = libxl__xs_read(gc, 0, libxl__sprintf(gc,
> +                    "/local/domain/0/device-model/%d/physmap/%s/name",
> +                    domid, phys_offset));

Again, quite a lot of repetition which obscures the similarities
between the different calls.

> +        if (start_addr == NULL || size == NULL || phys_offset == NULL)
> +            return -1;

This seems a rather odd condition.  Surely it is an error if only some
of these parameters are in xenstore ?

> +        if (name == NULL)
> +            namelen = 0;
> +        else
> +            namelen = strlen(name) + 1;
> +        *len += namelen + sizeof(struct libxl__physmap_info);
> +        offset = ptr - (*buf);
> +        *buf = realloc(*buf, *len);

Shouldn't this come from the gc ?

Ian.