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

["Denis V. Lunev" <den@openvz.org>]

On 05/11/14 05:12, Jeff Cody wrote:
> 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().
ok, fair enough. This is a rudiment from my previous internal version
which has been implemented in the other way.

>> +
>> +#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?
yes, this constraint is used now and will be used in the future.
XML structure is known and it is fixed. Here we just shortcut
the navigation in the tree in a convenient way to pass entire
path is one call.

I think that we can switch to interface like
    static xmlNodePtr xml_seek(xmlNode *root, ...)
and call it like
     xml_seek(root, "StorageData", "Storage", "Image", NULL);
in this case we will be able to drop a lot of processing and
parsing inside including strchr, memcpy, strlen etc


>> +        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
ok
>> +    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?

use of uninitialized variable in the line below.
     path_combine(image_path, sizeof(image_path), bs->file->filename, data);
which can not happen due to check size == 1 a line above.

>
>> +    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.
thank you for pointing this out

>> +    } 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.
>
you are right. I have missed this thing. Need some thinking on this,
but, anyway, this should be fixed.

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