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

[Jeff Cody <jcody@redhat.com>]

On Wed, Oct 29, 2014 at 04:38:07PM +0300, Denis V. Lunev wrote:
> 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 | 231 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 226 insertions(+), 5 deletions(-)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index 4f9cd8d..201c0f1 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -27,6 +27,11 @@
>  #include "block/block_int.h"
>  #include "qemu/module.h"
>  
> +#if CONFIG_LIBXML2
> +#include <libxml/parser.h>
> +#include <libxml/tree.h>
> +#endif
> +
>  /**************************************************************/
>  
>  #define HEADER_MAGIC "WithoutFreeSpace"
> @@ -34,6 +39,10 @@
>  #define HEADER_VERSION 2
>  #define HEADER_SIZE 64
>  
> +#define PARALLELS_XML       100
> +#define PARALLELS_IMAGE     101
> +
> +
>  // always little-endian
>  struct parallels_header {
>      char magic[16]; // "WithoutFreeSpace"
> @@ -59,6 +68,194 @@ typedef struct BDRVParallelsState {
>      unsigned int off_multiplier;
>  } BDRVParallelsState;
>  
> +static int parallels_open_image(BlockDriverState *bs, Error **errp);

You shouldn't need this forward declaration, if you put your new
function parallels_open_xml() after parallels_open_image().

> +
> +#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(xmlNode *root, const char *elem)
> +{
> +    xmlNode *child = root;
> +    const char *path;
> +    char nodename[128];
> +    int last = 0;
> +
> +    path = elem;
> +    if (path[0] == '/') {
> +        path++;
> +    }
> +    if (path[0] == 0) {
> +        return NULL;
> +    }
> +    while (!last) {
> +        const char *p = strchr(path, '/');
> +        int length;
> +        if (p == NULL) {
> +            length = strlen(path);
> +            last = 1;
> +        } else {
> +            length = p - path;
> +        }
> +        memcpy(nodename, path, length);
> +        nodename[length] = 0;

It looks like "elem" is always controlled by us, and not passed by the
user - will this always be the case?

If not, this doesn't seem safe, with nodename being a local array of
128 bytes.  How about using g_strdup() or g_strndup() here?

> +        child = xml_find(child, nodename);
> +        if (child == NULL) {
> +            return NULL;
> +        }
> +        path = ++p;
> +    }
> +    return child;
> +}
> +
> +static const char *xml_get_text(xmlNode *node, const char *name)
> +{
> +    xmlNode *child;
> +
> +    node = xml_seek(node, name);
> +    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;
> +    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;
> +    }
> +    /* XML file size should be resonable */

s/resonable/reasonable

> +    ret = -EFBIG;
> +    if (size > 65536) {
> +        goto fail;
> +    }
> +
> +    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");
> +    data = ""; /* make gcc happy */

What gcc warning are you trying to suppress here?

> +    for (size = 0; image != NULL; image = image->next) {
> +        if (image->type != XML_ELEMENT_NODE) {
> +            continue;
> +        }
> +
> +        size++;
> +        data = xml_get_text(image, "Type");
> +        if (data != NULL && strcmp(data, "Compressed")) {
> +            error_setg(errp, "Only compressed Parallels images are supported");
> +            goto done;
> +        }
> +
> +        data = xml_get_text(image, "File");
> +        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);
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret, "Could not open '%s': %s",
> +                         image_path, error_get_pretty(local_err));
> +        error_free(local_err);

error_get_pretty() is not NULL friendly - but I don't think it is an
issue, because if (ret < 0) is true, then (local_err) should be set as
well. Just an observation, I don't think you need to do anything here.

> +    } 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_probe_xml(const uint8_t *buf, int buf_size)
> +{
> +    int score = 0;
> +#if CONFIG_LIBXML2
> +    xmlDoc *doc;
> +    xmlNode *root;
> +
> +    doc = xmlReadMemory((const char *)buf, buf_size, NULL, NULL,
> +                        XML_PARSE_NOERROR | XML_PARSE_NOWARNING);
> +    if (doc == NULL) {
> +        return 0;   /* This is not an XML, we don't care */
> +    }
> +
> +    root = xmlDocGetRootElement(doc);
> +    if (root == NULL) {
> +        goto done;
> +    }
> +    if (!xmlStrcmp(root->name, (const xmlChar *)"Parallels_disk_image")) {
> +        score = PARALLELS_XML;
> +    }
> +
> +done:
> +    xmlFreeDoc(doc);
> +#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;
> @@ -69,13 +266,12 @@ static int parallels_probe(const uint8_t *buf, int buf_size, const char *filenam
>      if ((!memcmp(ph->magic, HEADER_MAGIC, 16) ||
>          !memcmp(ph->magic, HEADER_MAGIC2, 16)) &&
>          (le32_to_cpu(ph->version) == HEADER_VERSION))
> -        return 100;
> +        return PARALLELS_IMAGE;
>  
> -    return 0;
> +    return parallels_probe_xml(buf, buf_size);
>  }
>

Hmm.  So PARALLELS_XML is 100, and PARALLELS_IMAGE is 101 - and, this
is used later in this patch, to differentiate between XML and non-XML
images.  This is somewhat abusing the .bdrv_probe(), I think - the
intention of .bdrv_probe() is to return a confidence score between
0-100.

I think you'd be better off just checking the magic in
parallels_open() rather than overloading the .bdrv_probe() function.


> -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 +335,38 @@ 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;
>  }
>  
> +static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
> +                          Error **errp)
> +{
> +    uint8_t buf[1024];
> +    int score;
> +
> +    score = bdrv_pread(bs->file, 0, buf, sizeof(buf));
> +    if (score < 0) {
> +        return score;
> +    }
> +
> +    score = parallels_probe(buf, (unsigned)score, NULL);
> +    if (score == PARALLELS_XML) {
> +#if CONFIG_LIBXML2
> +        return parallels_open_xml(bs, flags, errp);
> +#endif
> +    } else if (score == PARALLELS_IMAGE) {
> +        return parallels_open_image(bs, errp);
> +    }
> +
> +    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;
> -- 
> 1.9.1
>