Re: [Qemu-devel] [PATCH 02/16] block/parallels: allow to specify DiskDescriptor.xml instead of image file

[Kevin Wolf <kwolf@redhat.com>]

Am 15.12.2014 um 09:27 hat Denis V. Lunev geschrieben:
> Typically Parallels disk bundle consists of several images which are
> glued by XML disk descriptor. Also XML hides inside several important
> parameters which are not available in the image header.
> 
> This patch allows to specify this XML file as a filename for an image
> to open. It is allowed only to open Compressed images with the only
> snapshot inside. No additional options are parsed at the moment.
> 
> The code itself is dumb enough for a while. If XML file is specified,
> the file is parsed and the image is reopened as bs->file to keep the
> rest of the driver untouched. This would be changed later with more
> features added.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Acked-by: Roman Kagan <rkagan@parallels.com>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 209 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 205 insertions(+), 4 deletions(-)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index 4f9cd8d..c22b91b 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -27,6 +27,12 @@
>  #include "block/block_int.h"
>  #include "qemu/module.h"
>  
> +#if CONFIG_LIBXML2
> +#include <libxml/parser.h>
> +#include <libxml/tree.h>
> +#endif
> +#include <stdarg.h>
> +
>  /**************************************************************/
>  
>  #define HEADER_MAGIC "WithoutFreeSpace"
> @@ -59,6 +65,24 @@ typedef struct BDRVParallelsState {
>      unsigned int off_multiplier;
>  } BDRVParallelsState;
>  
> +
> +static int parallels_probe_xml(const uint8_t *__buf, int buf_size)

Identifiers starting in an underscore are reserved, please come up with
a different name here.

> +{
> +    int score = 0;
> +#if CONFIG_LIBXML2

You need to use #ifdef here (and in three other places).

block/parallels.c:32:5: warning: "CONFIG_LIBXML2" is not defined
block/parallels.c:86:5: warning: "CONFIG_LIBXML2" is not defined
block/parallels.c:189:5: warning: "CONFIG_LIBXML2" is not defined
block/parallels.c:442:5: warning: "CONFIG_LIBXML2" is not defined

> +    char *buf = g_malloc(buf_size + 1);
> +
> +    memcpy(buf, __buf, buf_size);
> +    buf[buf_size] = 0;
> +    if (strstr(buf, "<?xml version=\"1.0\"?>") &&
> +        strstr(buf, "<Parallels_disk_image>")) {
> +        score = 100;
> +    }
> +    g_free(buf);
> +#endif
> +    return score;
> +}
> +
>  static int parallels_probe(const uint8_t *buf, int buf_size, const char *filename)
>  {
>      const struct parallels_header *ph = (const void *)buf;
> @@ -71,11 +95,10 @@ static int parallels_probe(const uint8_t *buf, int buf_size, const char *filenam
>          (le32_to_cpu(ph->version) == HEADER_VERSION))
>          return 100;
>  
> -    return 0;
> +    return parallels_probe_xml(buf, buf_size);
>  }

The restriction buf_size >= HEADER_SIZE applies even for XML now. It's
not very like that XML turns out shorter, but technically there's no
reason for this.

Perhaps rename the existing parallels_probe and add a new wrapper that
calls both? Then the condition would only apply where it's really
needed. Bonus cleanup: Instead of duplicating the condition in
parallels_open(), you could just call the renamed function then.

> -static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
> -                          Error **errp)
> +static int parallels_open_image(BlockDriverState *bs, Error **errp)
>  {
>      BDRVParallelsState *s = bs->opaque;
>      int i;
> @@ -139,13 +162,191 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>      return 0;
>  
>  fail_format:
> -    error_setg(errp, "Image not in Parallels format");
> +    error_setg(errp, "Image is not in Parallels format");
>      ret = -EINVAL;
>  fail:
>      g_free(s->catalog_bitmap);
>      return ret;
>  }

This hunk is unrelated to the XML functionality. Make it a separate
patch if you want to make this a proper sentence.

> +#if CONFIG_LIBXML2
> +static xmlNodePtr xml_find(xmlNode *node, const char *elem)
> +{
> +    xmlNode *child;
> +
> +    for (child = node->xmlChildrenNode; child != NULL; child = child->next) {
> +        if (!xmlStrcmp(child->name, (const xmlChar *)elem) &&
> +                child->type == XML_ELEMENT_NODE) {
> +            return child;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static xmlNodePtr xml_seek_va(xmlNode *root, va_list args)
> +{
> +    const char *elem;
> +
> +    while ((elem = va_arg(args, const char *)) != NULL) {
> +        root = xml_find(root, elem);
> +        if (root == NULL) {
> +            return NULL;
> +        }
> +    }
> +    return root;
> +}
> +
> +static xmlNodePtr xml_seek(xmlNode *root, ...)
> +{
> +    va_list args;
> +    va_start(args, root);
> +    root = xml_seek_va(root, args);
> +    va_end(args);
> +    return root;
> +}
> +
> +static const char *xml_get_text(xmlNode *node, ...)
> +{
> +    xmlNode *child;
> +    va_list args;
> +
> +    va_start(args, node);
> +    node = xml_seek_va(node, args);
> +    va_end(args);
> +
> +    if (node == NULL) {
> +        return NULL;
> +    }
> +
> +    for (child = node->xmlChildrenNode; child; child = child->next) {
> +        if (child->type == XML_TEXT_NODE) {
> +            return (const char *)child->content;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp)
> +{
> +    int size, ret;

size needs to be int64_t to avoid truncation of the bdrv_getlength()
return value.

> +    xmlDoc *doc = NULL;
> +    xmlNode *root, *image;
> +    char *xml = NULL;
> +    const char *data;
> +    char image_path[PATH_MAX];
> +    Error *local_err = NULL;
> +
> +    ret = size = bdrv_getlength(bs->file);
> +    if (ret < 0) {
> +        goto fail;
> +    }

Multiple assignments in a single line are generally avoided in qemu. I'd
suggest to write it like this:

size = bdrv_getlength();
if (size < 0) {
    ret = size;
    goto fail;
}

> +    /* XML file size should be reasonable */
> +    ret = -EFBIG;
> +    if (size > 65536) {
> +        goto fail;
> +    }

Here the convention is to set ret = -EFBIG inside the if block.

> +    xml = g_malloc(size + 1);
> +
> +    ret = bdrv_pread(bs->file, 0, xml, size);
> +    if (ret != size) {
> +        goto fail;
> +    }
> +    xml[size] = 0;
> +
> +    ret = -EINVAL;
> +    doc = xmlReadMemory(xml, size, NULL, NULL,
> +                        XML_PARSE_NOERROR | XML_PARSE_NOWARNING);
> +    if (doc == NULL) {
> +        goto fail;
> +    }
> +    root = xmlDocGetRootElement(doc);
> +    if (root == NULL) {
> +        goto fail;
> +    }
> +    image = xml_seek(root, "StorageData", "Storage", "Image", NULL);
> +    data = ""; /* make gcc happy */

Does this comment imply that you think that an assert(image != NULL)
would be correct? I don't think so.

An explicit error check should make gcc happy and would also improve the
error message for corrupt images without such an element (currently
"Parallels images with snapshots are not supported").

> +    for (size = 0; image != NULL; image = image->next) {
> +        if (image->type != XML_ELEMENT_NODE) {
> +            continue;
> +        }
> +
> +        size++;
> +        data = xml_get_text(image, "Type", NULL);
> +        if (data != NULL && strcmp(data, "Compressed")) {
> +            error_setg(errp, "Only compressed Parallels images are supported");
> +            goto done;
> +        }

Does a spec for the format exist that I could check the code against?

This seems to imply that the default, if no "Type" element exists, is
compressed images. Correct?

> +        data = xml_get_text(image, "File", NULL);
> +        if (data == NULL) {
> +            goto fail;
> +        }
> +    }
> +    /* Images with more than 1 snapshots are not supported at the moment */
> +    if (size != 1) {
> +        error_setg(errp, "Parallels images with snapshots are not supported");
> +        goto done;
> +    }
> +
> +    path_combine(image_path, sizeof(image_path), bs->file->filename, data);
> +    /* the caller (top layer bdrv_open) will close file for us if bs->file
> +       is changed. */
> +    bs->file = NULL;
> +
> +    ret = bdrv_open(&bs->file, image_path, NULL, NULL, flags | BDRV_O_PROTOCOL,
> +                    NULL, &local_err);

Please try to make this compatible with the way bs->file is opened in
bdrv_open():

    ret = bdrv_open_image(&file, filename, options, "file",
                          bdrv_inherited_flags(flags),
                          true, &local_err);

Otherwise you might get different flags (e.g. no BDRV_O_CACHE_WB and
BDRV_O_UNMAP) and options passed in the QDict won't be respected.

> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret, "Could not open '%s': %s",
> +                         image_path, error_get_pretty(local_err));
> +        error_free(local_err);
> +    } else {
> +        ret = parallels_open_image(bs, errp);
> +    }
> +
> +done:
> +    if (doc != NULL) {
> +        xmlFreeDoc(doc);
> +    }
> +    if (xml != NULL) {
> +        g_free(xml);
> +    }
> +    return ret;
> +
> +fail:
> +    error_setg(errp, "Failed to parse Parallels disk descriptor XML");
> +    goto done;
> +}
> +#endif
> +
> +static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
> +                          Error **errp)
> +{
> +    uint8_t buf[MAX(1024, HEADER_SIZE)];
> +    int size;
> +    const struct parallels_header *ph;
> +
> +    size = bdrv_pread(bs->file, 0, buf, sizeof(buf));
> +    if (size < 0) {
> +        return size;
> +    }
> +
> +    ph = (const struct parallels_header *)buf;
> +    if ((!memcmp(ph->magic, HEADER_MAGIC, 16) ||
> +         !memcmp(ph->magic, HEADER_MAGIC2, 16)) &&
> +        le32_to_cpu(ph->version) == HEADER_VERSION) {
> +        return parallels_open_image(bs, errp);
> +    } else if (parallels_probe_xml(buf, (unsigned)size)) {
> +#if CONFIG_LIBXML2
> +        return parallels_open_xml(bs, flags, errp);
> +#endif
> +    }
> +
> +    error_setg(errp, "Image is not in Parallels format");
> +    return -EINVAL;
> +}
> +
> +
>  static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
>  {
>      BDRVParallelsState *s = bs->opaque;

Kevin