[Qemu-devel] [PATCH v2 0/6] parallels format support improvements

[Qemu-devel] [PATCH v2 0/6] parallels format support improvements

[Denis V. Lunev]

The patchset implements additional compatibility bits for Parallels
format:
- initial support of parsing of Parallels DiskDeskriptor.xml
  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.
- support for padded Parallels images.
  For the time being Parallels was created an optimization for such OSes
  in its desktop product. Desktop users are not qualified enough to create
  properly aligned installations. Thus Parallels makes a blind guess
  on a customer behalf and creates so-called "padded" images if guest
  OS type is specified as WinXP, Win2k and Win2k3.

The code uses approach from VMDK support, either image or XML descriptor
could be used. Though there is temporary hack in the opening code:
BlockDriverState->file is being reopened inside parallels_open. I prefer
to keep this code in this state till proper Parallels snapshots support
in order to minimize current changes.

Changes from v1:
- dropped already merged part (original patches 1-3)

CC: Jeff Cody <jcody@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Roman Kagan <rkagan@parallels.com>

[Qemu-devel] [PATCH 1/6] configure: add dependency from libxml2

[Denis V. Lunev]

This dependency is required for adequate Parallels images support.
Typically the disk 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.

The patch also adds clause to checkpatch.pl to understand libxml2 types.
In the other case there is the following false positive:
    ERROR: need consistent spacing around '*' (ctx:WxB)
    #210: FILE: block/parallels.c:232:
    +   !xmlStrcmp(root->name, (const xmlChar *)"Parallels_disk_image"))

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>
CC: Paolo Bonzini <pbonzini@redhat.com>
---
 configure             | 29 +++++++++++++++++++++++++++++
 scripts/checkpatch.pl |  1 +
 2 files changed, 30 insertions(+)

diff --git a/configure b/configure
index a9e4d49..5426752 100755
--- a/configure
+++ b/configure
@@ -335,6 +335,7 @@ libssh2=""
 vhdx=""
 quorum=""
 numa=""
+libxml2=""
 
 # parse CC options first
 for opt do
@@ -1129,6 +1130,10 @@ for opt do
   ;;
   --enable-numa) numa="yes"
   ;;
+  --disable-libxml2) libxml2="no"
+  ;;
+  --enable-libxml2) libxml2="yes"
+  ;;
   *)
       echo "ERROR: unknown option $opt"
       echo "Try '$0 --help' for more information"
@@ -1400,6 +1405,8 @@ Advanced options (experts only):
   --enable-quorum          enable quorum block filter support
   --disable-numa           disable libnuma support
   --enable-numa            enable libnuma support
+  --disable-libxml2        disable libxml2 (for Parallels image format)
+  --enable-libxml2         enable libxml2 (for Parallels image format)
 
 NOTE: The object files are built at the place where configure is launched
 EOF
@@ -4097,6 +4104,23 @@ if test -z "$zero_malloc" ; then
     fi
 fi
 
+# check for libxml2
+if test "$libxml2" != "no" ; then
+    if $pkg_config --exists libxml-2.0; then
+        libxml2="yes"
+        libxml2_cflags=$($pkg_config --cflags libxml-2.0)
+        libxml2_libs=$($pkg_config --libs libxml-2.0)
+        QEMU_CFLAGS="$QEMU_CFLAGS $libxml2_cflags"
+        libs_softmmu="$libs_softmmu $libxml2_libs"
+        libs_tools="$libs_tools $libxml2_libs"
+    else
+        if test "$libxml2" = "yes"; then
+            feature_not_found "libxml2" "Install libxml2 devel"
+        fi
+        libxml2="no"
+    fi
+fi
+
 # Now we've finished running tests it's OK to add -Werror to the compiler flags
 if test "$werror" = "yes"; then
     QEMU_CFLAGS="-Werror $QEMU_CFLAGS"
@@ -4340,6 +4364,7 @@ echo "Quorum            $quorum"
 echo "lzo support       $lzo"
 echo "snappy support    $snappy"
 echo "NUMA host support $numa"
+echo "libxml2           $libxml2"
 
 if test "$sdl_too_old" = "yes"; then
 echo "-> Your SDL version is too old - please upgrade to have SDL support"
@@ -4846,6 +4871,10 @@ if test "$rdma" = "yes" ; then
   echo "CONFIG_RDMA=y" >> $config_host_mak
 fi
 
+if test "$libxml2" = "yes" ; then
+  echo "CONFIG_LIBXML2=y" >> $config_host_mak
+fi
+
 # Hold two types of flag:
 #   CONFIG_THREAD_SETNAME_BYTHREAD  - we've got a way of setting the name on
 #                                     a thread we have a handle to
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 053e432..3ddb097 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -242,6 +242,7 @@ our @typeList = (
 	qr{${Ident}_t},
 	qr{${Ident}_handler},
 	qr{${Ident}_handler_fn},
+	qr{xml${Ident}},
 );
 our @modifierList = (
 	qr{fastcall},
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/6] configure: add dependency from libxml2

[Michael Tokarev]

29.10.2014 16:38, Denis V. Lunev wrote:
> This dependency is required for adequate Parallels images support.
> Typically the disk 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.
[]
> +# check for libxml2
> +if test "$libxml2" != "no" ; then
> +    if $pkg_config --exists libxml-2.0; then
> +        libxml2="yes"
> +        libxml2_cflags=$($pkg_config --cflags libxml-2.0)
> +        libxml2_libs=$($pkg_config --libs libxml-2.0)
> +        QEMU_CFLAGS="$QEMU_CFLAGS $libxml2_cflags"
> +        libs_softmmu="$libs_softmmu $libxml2_libs"
> +        libs_tools="$libs_tools $libxml2_libs"

Please NO.  NO NO NO NO :)

Create a separate make variable, $LIBXML or something,
and add it as a parallels.o dependency, not libs_softmmu
etc.  Ditto for the cflags.  See examples -- libiscsi,
libcurl, librbd - in block/Makefile.objs.

After that, I think we'll move parallels.o to be a module ;)

(Cc'ing Fam)

Thanks,

/mjt

Re: [Qemu-devel] [PATCH 1/6] configure: add dependency from libxml2

[Denis V. Lunev]

On 05/11/14 10:32, Michael Tokarev wrote:
> 29.10.2014 16:38, Denis V. Lunev wrote:
>> This dependency is required for adequate Parallels images support.
>> Typically the disk 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.
> []
>> +# check for libxml2
>> +if test "$libxml2" != "no" ; then
>> +    if $pkg_config --exists libxml-2.0; then
>> +        libxml2="yes"
>> +        libxml2_cflags=$($pkg_config --cflags libxml-2.0)
>> +        libxml2_libs=$($pkg_config --libs libxml-2.0)
>> +        QEMU_CFLAGS="$QEMU_CFLAGS $libxml2_cflags"
>> +        libs_softmmu="$libs_softmmu $libxml2_libs"
>> +        libs_tools="$libs_tools $libxml2_libs"
> Please NO.  NO NO NO NO :)
>
> Create a separate make variable, $LIBXML or something,
> and add it as a parallels.o dependency, not libs_softmmu
> etc.  Ditto for the cflags.  See examples -- libiscsi,
> libcurl, librbd - in block/Makefile.objs.
no prob
> After that, I think we'll move parallels.o to be a module ;)
>
> (Cc'ing Fam)
what does it mean for me? Could you point out the place
to read/understand the difference.

Thank you in advance,
     Den

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

[Denis V. Lunev]

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);
+
+#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;
+        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 */
+    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 */
+    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);
+    } 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);
 }
 
-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

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

[Jeff Cody]

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
> 

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

[Denis V. Lunev]

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

[Qemu-devel] [PATCH 3/6] iotests: simple parallels XML disk descriptor file test added

[Denis V. Lunev]

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>
---
 tests/qemu-iotests/076                                    |   6 ++++++
 tests/qemu-iotests/076.out                                |   4 ++++
 tests/qemu-iotests/sample_images/parallels-simple.xml.bz2 | Bin 0 -> 374 bytes
 3 files changed, 10 insertions(+)
 create mode 100644 tests/qemu-iotests/sample_images/parallels-simple.xml.bz2

diff --git a/tests/qemu-iotests/076 b/tests/qemu-iotests/076
index ed2be35..242d0c3 100755
--- a/tests/qemu-iotests/076
+++ b/tests/qemu-iotests/076
@@ -75,6 +75,12 @@ echo "== Read from a valid v2 image =="
 _use_sample_img parallels-v2.bz2
 { $QEMU_IO -c "read -P 0x11 0 64k" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
 
+echo
+echo "== Read from a valid v2 image opened through xml =="
+_use_sample_img parallels-v2.bz2
+_use_sample_img parallels-simple.xml.bz2
+{ $QEMU_IO -c "read -P 0x11 0 64k" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/076.out b/tests/qemu-iotests/076.out
index 32ade08..628d9bf 100644
--- a/tests/qemu-iotests/076.out
+++ b/tests/qemu-iotests/076.out
@@ -19,4 +19,8 @@ no file open, try 'help open'
 == Read from a valid v2 image ==
 read 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== Read from a valid v2 image opened through xml ==
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 *** done
diff --git a/tests/qemu-iotests/sample_images/parallels-simple.xml.bz2 b/tests/qemu-iotests/sample_images/parallels-simple.xml.bz2
new file mode 100644
index 0000000000000000000000000000000000000000..13760bc9f30246ba01f43cdca3d3d90780430f61
GIT binary patch
literal 374
zcmV-+0g3)XT4*^jL0KkKSx`RPoB#ls-+)w5Py_$xpWsdazwh08Faddp1^@s613&-(
z00UFVg-_KD8fY4NVup`X5ukcZBTOVwCXAT_OhRY?0}wJ9sXCPa+IAr*0!sLV#k$+r
zFRIp6TJ>A%(u7Qkn2MJtX_(TWtce9REA{g&qLoa&l#2??q)aKM7Qx|d6glC9JSZw~
z-{Dy+9yLxWKg_2Xm02klNIFQqySl|-EJ?WkLWzOBm`Xui7_);kgut`D<Jy*x1cG2m
zCYzWh49n~Qks#g*ByEgsjlH07DHv)~9ZRui7r}`9{%wsd23x|ItiZh!U&QMH5t<C-
zN>^IdcIOOiAVM)WoC{=C8L4k%Ybsug63k@+f`frkh@_Ij10W;xr->@K!oLkGw3Lv$
zS&-sh3*_e*@-+(JnDfSD#7GS;nh3VYlxtRDgM<i`;3IE%@(Ot{k5NIKyfDy8eeT(5
UX+T&T%b)nWk}1N3f`Rtj;5{a#bN~PV

literal 0
HcmV?d00001

-- 
1.9.1

Re: [Qemu-devel] [PATCH 3/6] iotests: simple parallels XML disk descriptor file test added

[Jeff Cody]

On Wed, Oct 29, 2014 at 04:38:08PM +0300, Denis V. Lunev wrote:
> 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>
> ---
>  tests/qemu-iotests/076                                    |   6 ++++++
>  tests/qemu-iotests/076.out                                |   4 ++++
>  tests/qemu-iotests/sample_images/parallels-simple.xml.bz2 | Bin 0 -> 374 bytes
>  3 files changed, 10 insertions(+)
>  create mode 100644 tests/qemu-iotests/sample_images/parallels-simple.xml.bz2
> 
> diff --git a/tests/qemu-iotests/076 b/tests/qemu-iotests/076
> index ed2be35..242d0c3 100755
> --- a/tests/qemu-iotests/076
> +++ b/tests/qemu-iotests/076
> @@ -75,6 +75,12 @@ echo "== Read from a valid v2 image =="
>  _use_sample_img parallels-v2.bz2
>  { $QEMU_IO -c "read -P 0x11 0 64k" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
>  
> +echo
> +echo "== Read from a valid v2 image opened through xml =="
> +_use_sample_img parallels-v2.bz2
> +_use_sample_img parallels-simple.xml.bz2
> +{ $QEMU_IO -c "read -P 0x11 0 64k" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
> +

$TEST_IMG should be quoted, to be path-safe ("$TEST_IMG") - although,
looks like 076 has some other instances of unquoted $TEST_IMG, as
well (not your fault).  I don't care if you fix it here or not, we can
fix them all up in a later patch.

>  # success, all done
>  echo "*** done"
>  rm -f $seq.full
> diff --git a/tests/qemu-iotests/076.out b/tests/qemu-iotests/076.out
> index 32ade08..628d9bf 100644
> --- a/tests/qemu-iotests/076.out
> +++ b/tests/qemu-iotests/076.out
> @@ -19,4 +19,8 @@ no file open, try 'help open'
>  == Read from a valid v2 image ==
>  read 65536/65536 bytes at offset 0
>  64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +== Read from a valid v2 image opened through xml ==
> +read 65536/65536 bytes at offset 0
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  *** done
> diff --git a/tests/qemu-iotests/sample_images/parallels-simple.xml.bz2 b/tests/qemu-iotests/sample_images/parallels-simple.xml.bz2
> new file mode 100644
> index 0000000000000000000000000000000000000000..13760bc9f30246ba01f43cdca3d3d90780430f61
> GIT binary patch
> literal 374
> zcmV-+0g3)XT4*^jL0KkKSx`RPoB#ls-+)w5Py_$xpWsdazwh08Faddp1^@s613&-(
> z00UFVg-_KD8fY4NVup`X5ukcZBTOVwCXAT_OhRY?0}wJ9sXCPa+IAr*0!sLV#k$+r
> zFRIp6TJ>A%(u7Qkn2MJtX_(TWtce9REA{g&qLoa&l#2??q)aKM7Qx|d6glC9JSZw~
> z-{Dy+9yLxWKg_2Xm02klNIFQqySl|-EJ?WkLWzOBm`Xui7_);kgut`D<Jy*x1cG2m
> zCYzWh49n~Qks#g*ByEgsjlH07DHv)~9ZRui7r}`9{%wsd23x|ItiZh!U&QMH5t<C-
> zN>^IdcIOOiAVM)WoC{=C8L4k%Ybsug63k@+f`frkh@_Ij10W;xr->@K!oLkGw3Lv$
> zS&-sh3*_e*@-+(JnDfSD#7GS;nh3VYlxtRDgM<i`;3IE%@(Ot{k5NIKyfDy8eeT(5
> UX+T&T%b)nWk}1N3f`Rtj;5{a#bN~PV
> 
> literal 0
> HcmV?d00001
> 
> -- 
> 1.9.1
>

Reviewed-by: Jeff Cody <jcody@redhat.com>

[Qemu-devel] [PATCH 4/6] block/parallels: support padded Parallels images

[Denis V. Lunev]

Unfortunately, old guest OSes do not align partitions to page size by
default. This is true for Windows 2003 and Windows XP.

For the time being Parallels was created an optimization for such OSes
in its desktop product. Desktop users are not qualified enough to create
properly aligned installations. Thus Parallels makes a blind guess
on a customer behalf and creates so-called "padded" images if guest
OS type is specified as WinXP, Win2k and Win2k3.

"Padding" is a value which should be added to guest LBA to obtain
sector number inside the image. This results in a shifted images.
   0123        offset inside image (in 512 byte sectors)
  +-------
  +.012        guest data (512 byte sectors)
  +-------
The information about this is available in DiskDescriptor.xml ONLY. There
is no such data in the image header.

There share of such images could be evaluated as 6-8% according to the
statistics in my hands.

This patch obtains proper value from XML and applies it on reading.

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 | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index 201c0f1..d07e4f7 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -66,6 +66,7 @@ typedef struct BDRVParallelsState {
     unsigned int tracks;
 
     unsigned int off_multiplier;
+    unsigned int padding;
 } BDRVParallelsState;
 
 static int parallels_open_image(BlockDriverState *bs, Error **errp);
@@ -144,6 +145,7 @@ static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp)
     const char *data;
     char image_path[PATH_MAX];
     Error *local_err = NULL;
+    BDRVParallelsState *s = bs->opaque;
 
     ret = size = bdrv_getlength(bs->file);
     if (ret < 0) {
@@ -173,6 +175,19 @@ static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp)
     if (root == NULL) {
         goto fail;
     }
+
+    data = xml_get_text(root, "/Disk_Parameters/Padding");
+    if (data != NULL) {
+        char *endptr;
+        unsigned long pad;
+
+        pad = strtoul(data, &endptr, 0);
+        if ((endptr != NULL && *endptr != '\0') || pad > UINT_MAX) {
+            goto fail;
+        }
+        s->padding = (uint32_t)pad;
+    }
+
     image = xml_seek(root, "/StorageData/Storage/Image");
     data = ""; /* make gcc happy */
     for (size = 0; image != NULL; image = image->next) {
@@ -385,6 +400,10 @@ static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
 static int parallels_read(BlockDriverState *bs, int64_t sector_num,
                     uint8_t *buf, int nb_sectors)
 {
+    BDRVParallelsState *s = bs->opaque;
+
+    sector_num += s->padding;
+
     while (nb_sectors > 0) {
         int64_t position = seek_to_sector(bs, sector_num);
         if (position >= 0) {
-- 
1.9.1

Re: [Qemu-devel] [PATCH 4/6] block/parallels: support padded Parallels images

[Jeff Cody]

On Wed, Oct 29, 2014 at 04:38:09PM +0300, Denis V. Lunev wrote:
> Unfortunately, old guest OSes do not align partitions to page size by
> default. This is true for Windows 2003 and Windows XP.
> 
> For the time being Parallels was created an optimization for such OSes
> in its desktop product. Desktop users are not qualified enough to create
> properly aligned installations. Thus Parallels makes a blind guess
> on a customer behalf and creates so-called "padded" images if guest
> OS type is specified as WinXP, Win2k and Win2k3.
> 
> "Padding" is a value which should be added to guest LBA to obtain
> sector number inside the image. This results in a shifted images.
>    0123        offset inside image (in 512 byte sectors)
>   +-------
>   +.012        guest data (512 byte sectors)
>   +-------
> The information about this is available in DiskDescriptor.xml ONLY. There
> is no such data in the image header.
> 
> There share of such images could be evaluated as 6-8% according to the
> statistics in my hands.
> 
> This patch obtains proper value from XML and applies it on reading.
> 
> 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 | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index 201c0f1..d07e4f7 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -66,6 +66,7 @@ typedef struct BDRVParallelsState {
>      unsigned int tracks;
>  
>      unsigned int off_multiplier;
> +    unsigned int padding;
>  } BDRVParallelsState;
>  
>  static int parallels_open_image(BlockDriverState *bs, Error **errp);
> @@ -144,6 +145,7 @@ static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp)
>      const char *data;
>      char image_path[PATH_MAX];
>      Error *local_err = NULL;
> +    BDRVParallelsState *s = bs->opaque;
>  
>      ret = size = bdrv_getlength(bs->file);
>      if (ret < 0) {
> @@ -173,6 +175,19 @@ static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp)
>      if (root == NULL) {
>          goto fail;
>      }
> +
> +    data = xml_get_text(root, "/Disk_Parameters/Padding");
> +    if (data != NULL) {
> +        char *endptr;
> +        unsigned long pad;
> +
> +        pad = strtoul(data, &endptr, 0);
> +        if ((endptr != NULL && *endptr != '\0') || pad > UINT_MAX) {
> +            goto fail;
> +        }
> +        s->padding = (uint32_t)pad;
> +    }
> +
>      image = xml_seek(root, "/StorageData/Storage/Image");
>      data = ""; /* make gcc happy */
>      for (size = 0; image != NULL; image = image->next) {
> @@ -385,6 +400,10 @@ static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
>  static int parallels_read(BlockDriverState *bs, int64_t sector_num,
>                      uint8_t *buf, int nb_sectors)
>  {
> +    BDRVParallelsState *s = bs->opaque;
> +
> +    sector_num += s->padding;
> +
>      while (nb_sectors > 0) {
>          int64_t position = seek_to_sector(bs, sector_num);
>          if (position >= 0) {
> -- 
> 1.9.1
> 

Reviewed-by: Jeff Cody <jcody@redhat.com>

[Qemu-devel] [PATCH 5/6] iotests: padded parallels image test

[Denis V. Lunev]

Unfortunately, old guest OSes do not align partitions to page size by
default. This is true for Windows 2003 and Windows XP.

"Padding" is a value which should be added to guest LBA to obtain
sector number inside the image. This results in a shifted images.
   0123        offset inside image (in 512 byte sectors)
  +-------
  +.012        guest data (512 byte sectors)
  +-------
The information about this is available in DiskDescriptor.xml ONLY. There
is no such data in the image header.

This patch contains very simple image with padding and corresponding
XML disk descriptor created in authentic way.

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>
---
 tests/qemu-iotests/076                                    |   6 ++++++
 tests/qemu-iotests/076.out                                |   4 ++++
 tests/qemu-iotests/sample_images/parallels-padded.xml.bz2 | Bin 0 -> 377 bytes
 tests/qemu-iotests/sample_images/parallels-v2-padded.bz2  | Bin 0 -> 139 bytes
 4 files changed, 10 insertions(+)
 create mode 100644 tests/qemu-iotests/sample_images/parallels-padded.xml.bz2
 create mode 100644 tests/qemu-iotests/sample_images/parallels-v2-padded.bz2

diff --git a/tests/qemu-iotests/076 b/tests/qemu-iotests/076
index 242d0c3..c83b953 100755
--- a/tests/qemu-iotests/076
+++ b/tests/qemu-iotests/076
@@ -81,6 +81,12 @@ _use_sample_img parallels-v2.bz2
 _use_sample_img parallels-simple.xml.bz2
 { $QEMU_IO -c "read -P 0x11 0 64k" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
 
+echo
+echo "== Read from a valid v2 image opened through xml with padding =="
+_use_sample_img parallels-v2-padded.bz2
+_use_sample_img parallels-padded.xml.bz2
+{ $QEMU_IO -c "read -P 0x11 0 64k" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/076.out b/tests/qemu-iotests/076.out
index 628d9bf..46680d8 100644
--- a/tests/qemu-iotests/076.out
+++ b/tests/qemu-iotests/076.out
@@ -23,4 +23,8 @@ read 65536/65536 bytes at offset 0
 == Read from a valid v2 image opened through xml ==
 read 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== Read from a valid v2 image opened through xml with padding ==
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 *** done
diff --git a/tests/qemu-iotests/sample_images/parallels-padded.xml.bz2 b/tests/qemu-iotests/sample_images/parallels-padded.xml.bz2
new file mode 100644
index 0000000000000000000000000000000000000000..90e481b88e1f4d65256d0511a8509114c6358052
GIT binary patch
literal 377
zcmV-<0fzoUT4*^jL0KkKS+JP`&Hw<KUw~9lPy_$xpWsdazwh08FaddH8X6Fz^phqa
z0kr@C000`P{z){A0000Upa9SS5-9{U$c>4nhyVtdfq>MRN`P%Vs7e5mydg1cvP<`s
zYo%7Ss&>_C5i%-bDqP)#HmEBiP+TU)8WyoiurHMw!l_ZF1>8_3m3N^^QwEd8=8hkN
zLO3;VtVoT0C3B518KN#oIV4@)7DfOSbkchYBQVO$9`K;#(G?m(FfikRxR&q(!eB`z
zPfQaAWzYbTAj%3PZH#S=y<l!A7-v`x<>0Cm|7Ih97TD6vNg%Klm)F8;i7;OxS)eka
zB8A?w-Lb<P2oQ=RX}Frn6(&gA)fHtcO2o1qAWlwhDKQig<sdQwI23XvR~T0E)#6G<
zUDR|Oi30I4$}VPcTnCQ(CKJjW0g+;mFwDaQlohzp(E|mg47s2j;G&NR^b?hm3@s%x
XY}H|5IAssg=l(9_ig2MJVKN1r8+(|=

literal 0
HcmV?d00001

diff --git a/tests/qemu-iotests/sample_images/parallels-v2-padded.bz2 b/tests/qemu-iotests/sample_images/parallels-v2-padded.bz2
new file mode 100644
index 0000000000000000000000000000000000000000..80948ce689e2c5c2335d6e6b36965fe0b2305613
GIT binary patch
literal 139
zcmV;60CfLCT4*^jL0KkKSt&WeumB1p|H%DFU;t162mk{B2!JYJ)<8f2AOHXuAOMPl
zQl^8-X^;SIHmDUv6HN?Ehp1#2<6B`Us=32n6t4O~g)zWI0C`4`x5NN3Kna)uT+{)Q
t08GFDLI4D2paA;U<m>wRX=_?<Gd@%RJirP-3P26T+>uTcBnnP&Yyj78F)siB

literal 0
HcmV?d00001

-- 
1.9.1

Re: [Qemu-devel] [PATCH 5/6] iotests: padded parallels image test

[Jeff Cody]

On Wed, Oct 29, 2014 at 04:38:10PM +0300, Denis V. Lunev wrote:
> Unfortunately, old guest OSes do not align partitions to page size by
> default. This is true for Windows 2003 and Windows XP.
> 
> "Padding" is a value which should be added to guest LBA to obtain
> sector number inside the image. This results in a shifted images.
>    0123        offset inside image (in 512 byte sectors)
>   +-------
>   +.012        guest data (512 byte sectors)
>   +-------
> The information about this is available in DiskDescriptor.xml ONLY. There
> is no such data in the image header.
> 
> This patch contains very simple image with padding and corresponding
> XML disk descriptor created in authentic way.
> 
> 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>
> ---
>  tests/qemu-iotests/076                                    |   6 ++++++
>  tests/qemu-iotests/076.out                                |   4 ++++
>  tests/qemu-iotests/sample_images/parallels-padded.xml.bz2 | Bin 0 -> 377 bytes
>  tests/qemu-iotests/sample_images/parallels-v2-padded.bz2  | Bin 0 -> 139 bytes
>  4 files changed, 10 insertions(+)
>  create mode 100644 tests/qemu-iotests/sample_images/parallels-padded.xml.bz2
>  create mode 100644 tests/qemu-iotests/sample_images/parallels-v2-padded.bz2
> 
> diff --git a/tests/qemu-iotests/076 b/tests/qemu-iotests/076
> index 242d0c3..c83b953 100755
> --- a/tests/qemu-iotests/076
> +++ b/tests/qemu-iotests/076
> @@ -81,6 +81,12 @@ _use_sample_img parallels-v2.bz2
>  _use_sample_img parallels-simple.xml.bz2
>  { $QEMU_IO -c "read -P 0x11 0 64k" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
>  
> +echo
> +echo "== Read from a valid v2 image opened through xml with padding =="
> +_use_sample_img parallels-v2-padded.bz2
> +_use_sample_img parallels-padded.xml.bz2
> +{ $QEMU_IO -c "read -P 0x11 0 64k" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
> +

Same comment as before on $TEST_IMG needing quoting, and as before,
can wait until we clean up the other instances in this same file.


Reviewed-by: Jeff Cody <jcody@redhat.com>


>  # success, all done
>  echo "*** done"
>  rm -f $seq.full
> diff --git a/tests/qemu-iotests/076.out b/tests/qemu-iotests/076.out
> index 628d9bf..46680d8 100644
> --- a/tests/qemu-iotests/076.out
> +++ b/tests/qemu-iotests/076.out
> @@ -23,4 +23,8 @@ read 65536/65536 bytes at offset 0
>  == Read from a valid v2 image opened through xml ==
>  read 65536/65536 bytes at offset 0
>  64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +== Read from a valid v2 image opened through xml with padding ==
> +read 65536/65536 bytes at offset 0
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  *** done
> diff --git a/tests/qemu-iotests/sample_images/parallels-padded.xml.bz2 b/tests/qemu-iotests/sample_images/parallels-padded.xml.bz2
> new file mode 100644
> index 0000000000000000000000000000000000000000..90e481b88e1f4d65256d0511a8509114c6358052
> GIT binary patch
> literal 377
> zcmV-<0fzoUT4*^jL0KkKS+JP`&Hw<KUw~9lPy_$xpWsdazwh08FaddH8X6Fz^phqa
> z0kr@C000`P{z){A0000Upa9SS5-9{U$c>4nhyVtdfq>MRN`P%Vs7e5mydg1cvP<`s
> zYo%7Ss&>_C5i%-bDqP)#HmEBiP+TU)8WyoiurHMw!l_ZF1>8_3m3N^^QwEd8=8hkN
> zLO3;VtVoT0C3B518KN#oIV4@)7DfOSbkchYBQVO$9`K;#(G?m(FfikRxR&q(!eB`z
> zPfQaAWzYbTAj%3PZH#S=y<l!A7-v`x<>0Cm|7Ih97TD6vNg%Klm)F8;i7;OxS)eka
> zB8A?w-Lb<P2oQ=RX}Frn6(&gA)fHtcO2o1qAWlwhDKQig<sdQwI23XvR~T0E)#6G<
> zUDR|Oi30I4$}VPcTnCQ(CKJjW0g+;mFwDaQlohzp(E|mg47s2j;G&NR^b?hm3@s%x
> XY}H|5IAssg=l(9_ig2MJVKN1r8+(|=
> 
> literal 0
> HcmV?d00001
> 
> diff --git a/tests/qemu-iotests/sample_images/parallels-v2-padded.bz2 b/tests/qemu-iotests/sample_images/parallels-v2-padded.bz2
> new file mode 100644
> index 0000000000000000000000000000000000000000..80948ce689e2c5c2335d6e6b36965fe0b2305613
> GIT binary patch
> literal 139
> zcmV;60CfLCT4*^jL0KkKSt&WeumB1p|H%DFU;t162mk{B2!JYJ)<8f2AOHXuAOMPl
> zQl^8-X^;SIHmDUv6HN?Ehp1#2<6B`Us=32n6t4O~g)zWI0C`4`x5NN3Kna)uT+{)Q
> t08GFDLI4D2paA;U<m>wRX=_?<Gd@%RJirP-3P26T+>uTcBnnP&Yyj78F)siB
> 
> literal 0
> HcmV?d00001
> 
> -- 
> 1.9.1
> 

[Qemu-devel] [PATCH 6/6] parallels: change copyright information in the image header

[Denis V. Lunev]

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 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/parallels.c b/block/parallels.c
index d07e4f7..1491211 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -2,8 +2,10 @@
  * Block driver for Parallels disk image format
  *
  * Copyright (c) 2007 Alex Beregszaszi
+ * Copyright (c) 2014 Denis V. Lunev <den@openvz.org>
  *
- * This code is based on comparing different disk images created by Parallels.
+ * This code was originally based on comparing different disk images created
+ * by Parallels.
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
-- 
1.9.1

Re: [Qemu-devel] [PATCH 6/6] parallels: change copyright information in the image header

[Jeff Cody]

On Wed, Oct 29, 2014 at 04:38:11PM +0300, Denis V. Lunev wrote:
> 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 | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index d07e4f7..1491211 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -2,8 +2,10 @@
>   * Block driver for Parallels disk image format
>   *
>   * Copyright (c) 2007 Alex Beregszaszi
> + * Copyright (c) 2014 Denis V. Lunev <den@openvz.org>
>   *
> - * This code is based on comparing different disk images created by Parallels.
> + * This code was originally based on comparing different disk images created
> + * by Parallels.
>   *
>   * Permission is hereby granted, free of charge, to any person obtaining a copy
>   * of this software and associated documentation files (the "Software"), to deal
> -- 
> 1.9.1
>

Reviewed-by: Jeff Cody <jcody@redhat.com>