[Qemu-devel] [PATCH v4 0/16] parallels format support improvements

[Qemu-devel] [PATCH v4 0/16] 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.
- support for snaphosts in parallels format (read-only)
- some other improvements

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 v3:
- (patch 2) method of detection of Parallels XML is changed. xmlReadMemory
  fails if not entire file is specified
- (patches 8-11) get block status, backing store, reading performance
  improvements
- (patches 13-16) parallels snapshots support added

Changes from v2:
- (patch 1) changed libxml2 addition as suggested by Michael Tokarev
- (patch 2) changed API of xml_find/xml_get_text to avoid memcpy to variable
  on stack
- (patch 2) dropped predefined value for PARALLELS_XML/PARALLELS_IMAGE
- (patch 2) other minor changes (spelling, placement)
- (patch 3) quoted TEST_IMG as suggested by Jeff Cody
- (patches 4, 6) quoted TEST_IMG as suggested by Jeff Cody

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>
CC: Denis V. Lunev <den@openvz.org>

[Qemu-devel] [PATCH 01/16] 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>
CC: Michael Tokarev <mjt@tls.msk.ru>
---
 block/Makefile.objs   |  2 ++
 configure             | 28 ++++++++++++++++++++++++++++
 scripts/checkpatch.pl |  1 +
 3 files changed, 31 insertions(+)

diff --git a/block/Makefile.objs b/block/Makefile.objs
index 04b0e43..3040c33 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -38,3 +38,5 @@ ssh.o-libs         := $(LIBSSH2_LIBS)
 archipelago.o-libs := $(ARCHIPELAGO_LIBS)
 qcow.o-libs        := -lz
 linux-aio.o-libs   := -laio
+parallels.o-cflags := $(LIBXML2_CFLAGS)
+parallels.o-libs   := $(LIBXML2_LIBS)
diff --git a/configure b/configure
index cae588c..58b1f8d 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
@@ -4098,6 +4105,20 @@ 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)
+    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"
@@ -4341,6 +4362,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"
@@ -4847,6 +4869,12 @@ 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
+  echo "LIBXML2_CFLAGS=$libxml2_cflags" >> $config_host_mak
+  echo "LIBXML2_LIBS=$libxml2_libs" >> $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

[Qemu-devel] [PATCH 02/16] 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 | 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)
+{
+    int score = 0;
+#if CONFIG_LIBXML2
+    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);
 }
 
-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;
 }
 
+#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;
+    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 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", NULL);
+    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", NULL);
+        if (data != NULL && strcmp(data, "Compressed")) {
+            error_setg(errp, "Only compressed Parallels images are supported");
+            goto done;
+        }
+
+        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);
+    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;
-- 
1.9.1

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

[Kevin Wolf]

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

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

[Denis V. Lunev]

On 15/12/14 13:45, Kevin Wolf wrote:
> Am 15.12.2014 um 09:27 hat Denis V. Lunev geschrieben:
>

> 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?
Unfortunately, there is no formal spec in my hands.

The check itself is made to avoid "Plain" Parallels image.
I have consulted with the original author and from his
opinion the check was made "just to ensure". Images
without this element was never seen by him and by me.

As for the rest of your comments, "sure, will fix on next
submission".

Thank you for the prompt reply.

Regards,
     Den

[Qemu-devel] [PATCH 03/16] iotests, parallels: quote TEST_IMG in 076 test to be path-safe

[Denis V. Lunev]

suggested by Jeff Cody

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/076 | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/076 b/tests/qemu-iotests/076
index ed2be35..0139976 100755
--- a/tests/qemu-iotests/076
+++ b/tests/qemu-iotests/076
@@ -49,31 +49,31 @@ nb_sectors_offset=$((0x24))
 echo
 echo "== Read from a valid v1 image =="
 _use_sample_img parallels-v1.bz2
-{ $QEMU_IO -c "read -P 0x11 0 64k" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0x11 0 64k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
 
 echo
 echo "== Negative catalog size =="
 _use_sample_img parallels-v1.bz2
 poke_file "$TEST_IMG" "$catalog_entries_offset" "\xff\xff\xff\xff"
-{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read 0 512" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
 
 echo
 echo "== Overflow in catalog allocation =="
 _use_sample_img parallels-v1.bz2
 poke_file "$TEST_IMG" "$nb_sectors_offset" "\xff\xff\xff\xff"
 poke_file "$TEST_IMG" "$catalog_entries_offset" "\x01\x00\x00\x40"
-{ $QEMU_IO -c "read 64M 64M" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read 64M 64M" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
 
 echo
 echo "== Zero sectors per track =="
 _use_sample_img parallels-v1.bz2
 poke_file "$TEST_IMG" "$tracks_offset" "\x00\x00\x00\x00"
-{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read 0 512" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
 
 echo
 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
+{ $QEMU_IO -c "read -P 0x11 0 64k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
 
 # success, all done
 echo "*** done"
-- 
1.9.1

[Qemu-devel] [PATCH 04/16] 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>
Reviewed-by: 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 0139976..636fc58 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 04/16] iotests: simple parallels XML disk descriptor file test added

[Kevin Wolf]

Am 15.12.2014 um 09:27 hat Denis V. Lunev geschrieben:
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Acked-by: Roman Kagan <rkagan@parallels.com>
> Reviewed-by: 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 0139976..636fc58 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

_sample_img isn't made to be used twice in a single test case. It will
forget to clean up the previously used decompressed images.

Should be easy enough to modify it so that it saves a list of sample
files instead of just the last one, but it needs to be done before we
can do this.

> +{ $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

Kevin

[Qemu-devel] [PATCH 05/16] 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>
Reviewed-by: 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 c22b91b..fedb009 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -63,6 +63,7 @@ typedef struct BDRVParallelsState {
     unsigned int tracks;
 
     unsigned int off_multiplier;
+    unsigned int padding;
 } BDRVParallelsState;
 
 
@@ -235,6 +236,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) {
@@ -264,6 +266,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", NULL);
+    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", NULL);
     data = ""; /* make gcc happy */
     for (size = 0; image != NULL; image = image->next) {
@@ -365,6 +380,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 05/16] block/parallels: support padded Parallels images

[Kevin Wolf]

Am 15.12.2014 um 09:27 hat Denis V. Lunev geschrieben:
> 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>
> Reviewed-by: 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 c22b91b..fedb009 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -63,6 +63,7 @@ typedef struct BDRVParallelsState {
>      unsigned int tracks;
>  
>      unsigned int off_multiplier;
> +    unsigned int padding;
>  } BDRVParallelsState;
>  
>  
> @@ -235,6 +236,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) {
> @@ -264,6 +266,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", NULL);
> +    if (data != NULL) {
> +        char *endptr;
> +        unsigned long pad;
> +
> +        pad = strtoul(data, &endptr, 0);
> +        if ((endptr != NULL && *endptr != '\0') || pad > UINT_MAX) {

Can endptr even be NULL? Also, shouldn't you set errno = 0 before and
check it here?

> +            goto fail;
> +        }
> +        s->padding = (uint32_t)pad;

s->padding is unsigned int, pad is unsigned long. Why the cast to
uint32_t here, which is different from both?

> +    }
> +
>      image = xml_seek(root, "StorageData", "Storage", "Image", NULL);
>      data = ""; /* make gcc happy */
>      for (size = 0; image != NULL; image = image->next) {
> @@ -365,6 +380,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;

No check needed here? bdrv_check_request() has checked sector_num and
nb_sectors against the image size, but now you can't rely on the result
any more.

>      while (nb_sectors > 0) {
>          int64_t position = seek_to_sector(bs, sector_num);
>          if (position >= 0) {

Kevin

Re: [Qemu-devel] [PATCH 05/16] block/parallels: support padded Parallels images

[Denis V. Lunev]

On 15/12/14 14:05, Kevin Wolf wrote:
> Am 15.12.2014 um 09:27 hat Denis V. Lunev geschrieben:
>> 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>
>> Reviewed-by: 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 c22b91b..fedb009 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -63,6 +63,7 @@ typedef struct BDRVParallelsState {
>>       unsigned int tracks;
>>   
>>       unsigned int off_multiplier;
>> +    unsigned int padding;
>>   } BDRVParallelsState;
>>   
>>   
>> @@ -235,6 +236,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) {
>> @@ -264,6 +266,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", NULL);
>> +    if (data != NULL) {
>> +        char *endptr;
>> +        unsigned long pad;
>> +
>> +        pad = strtoul(data, &endptr, 0);
>> +        if ((endptr != NULL && *endptr != '\0') || pad > UINT_MAX) {
> Can endptr even be NULL? Also, shouldn't you set errno = 0 before and
> check it here?
got it, I think you are right
>> +            goto fail;
>> +        }
>> +        s->padding = (uint32_t)pad;
> s->padding is unsigned int, pad is unsigned long. Why the cast to
> uint32_t here, which is different from both?
ok
>> +    }
>> +
>>       image = xml_seek(root, "StorageData", "Storage", "Image", NULL);
>>       data = ""; /* make gcc happy */
>>       for (size = 0; image != NULL; image = image->next) {
>> @@ -365,6 +380,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;
> No check needed here? bdrv_check_request() has checked sector_num and
> nb_sectors against the image size, but now you can't rely on the result
> any more.
you are perfectly correct. I'll recheck the situation.
I have seen some dances with image size. Allowed
image size should be extended with 1 block, but
I am not quite sure.

Thank you for pointing this out.

>>       while (nb_sectors > 0) {
>>           int64_t position = seek_to_sector(bs, sector_num);
>>           if (position >= 0) {
> Kevin

[Qemu-devel] [PATCH 06/16] 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>
Reviewed-by: 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 636fc58..766b359 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

[Qemu-devel] [PATCH 07/16] 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>
Reviewed-by: 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 fedb009..5fcede8 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 07/16] parallels: change copyright information in the image header

[Kevin Wolf]

Am 15.12.2014 um 09:27 hat Denis V. Lunev geschrieben:
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Acked-by: Roman Kagan <rkagan@parallels.com>
> Reviewed-by: 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 fedb009..5fcede8 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.

On what is it based now? (Link to a spec would be great, if it exists.)

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

Kevin

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

[Denis V. Lunev]

On 15/12/14 14:06, Kevin Wolf wrote:
> Am 15.12.2014 um 09:27 hat Denis V. Lunev geschrieben:
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Acked-by: Roman Kagan <rkagan@parallels.com>
>> Reviewed-by: 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 fedb009..5fcede8 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.
> On what is it based now? (Link to a spec would be great, if it exists.)

It is based now on http://git.openvz.org/?p=ploop;a=summary
Unfortunately, there are no publicly available documents about.

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

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

[Denis V. Lunev]

On 15/12/14 14:06, Kevin Wolf wrote:
> Am 15.12.2014 um 09:27 hat Denis V. Lunev geschrieben:
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Acked-by: Roman Kagan <rkagan@parallels.com>
>> Reviewed-by: 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 fedb009..5fcede8 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.
> On what is it based now? (Link to a spec would be great, if it exists.)
I was wrong :)

Pls find disk descriptor XML XSD scheme under this link

https://github.com/CloudServer/parallels-sdk/blob/master/Sources/XmlModel/Schema/DiskDescriptor.xsd

[Qemu-devel] [PATCH 08/16] block/parallels: switch to bdrv_read

[Denis V. Lunev]

From: Roman Kagan <rkagan@parallels.com>

Switch the .bdrv_read method implementation from using bdrv_pread() to
bdrv_read() on the underlying file, since the latter is subject to i/o
throttling while the former is not.

Besides, since bdrv_read() operates in sectors rather than bytes, adjust
the helper functions to do so too.

Signed-off-by: Roman Kagan <rkagan@parallels.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 5fcede8..de4f39f 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -364,9 +364,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
 }
 
 
-static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
+static int64_t seek_to_sector(BDRVParallelsState *s, int64_t sector_num)
 {
-    BDRVParallelsState *s = bs->opaque;
     uint32_t index, offset;
 
     index = sector_num / s->tracks;
@@ -375,8 +374,7 @@ static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
     /* not allocated */
     if ((index >= s->catalog_size) || (s->catalog_bitmap[index] == 0))
         return -1;
-    return
-        ((uint64_t)s->catalog_bitmap[index] * s->off_multiplier + offset) * 512;
+    return (uint64_t)s->catalog_bitmap[index] * s->off_multiplier + offset;
 }
 
 static int parallels_read(BlockDriverState *bs, int64_t sector_num,
@@ -387,16 +385,18 @@ static int parallels_read(BlockDriverState *bs, int64_t sector_num,
     sector_num += s->padding;
 
     while (nb_sectors > 0) {
-        int64_t position = seek_to_sector(bs, sector_num);
+        int64_t position = seek_to_sector(s, sector_num);
         if (position >= 0) {
-            if (bdrv_pread(bs->file, position, buf, 512) != 512)
-                return -1;
+            int ret = bdrv_read(bs->file, position, buf, 1);
+            if (ret < 0) {
+                return ret;
+            }
         } else {
-            memset(buf, 0, 512);
+            memset(buf, 0, BDRV_SECTOR_SIZE);
         }
         nb_sectors--;
         sector_num++;
-        buf += 512;
+        buf += BDRV_SECTOR_SIZE;
     }
     return 0;
 }
-- 
1.9.1

[Qemu-devel] [PATCH 09/16] block/parallels: read up to cluster end in one go

[Denis V. Lunev]

From: Roman Kagan <rkagan@parallels.com>

Teach parallels_read() to do reads in coarser granularity than just a
single sector: if requested, read up the cluster end in one go.

Signed-off-by: Roman Kagan <rkagan@parallels.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index de4f39f..a05bf39 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -377,6 +377,13 @@ static int64_t seek_to_sector(BDRVParallelsState *s, int64_t sector_num)
     return (uint64_t)s->catalog_bitmap[index] * s->off_multiplier + offset;
 }
 
+static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num,
+        int nb_sectors)
+{
+    int ret = s->tracks - sector_num % s->tracks;
+    return MIN(nb_sectors, ret);
+}
+
 static int parallels_read(BlockDriverState *bs, int64_t sector_num,
                     uint8_t *buf, int nb_sectors)
 {
@@ -386,17 +393,18 @@ static int parallels_read(BlockDriverState *bs, int64_t sector_num,
 
     while (nb_sectors > 0) {
         int64_t position = seek_to_sector(s, sector_num);
+        int n = cluster_remainder(s, sector_num, nb_sectors);
         if (position >= 0) {
-            int ret = bdrv_read(bs->file, position, buf, 1);
+            int ret = bdrv_read(bs->file, position, buf, n);
             if (ret < 0) {
                 return ret;
             }
         } else {
-            memset(buf, 0, BDRV_SECTOR_SIZE);
+            memset(buf, 0, n << BDRV_SECTOR_BITS);
         }
-        nb_sectors--;
-        sector_num++;
-        buf += BDRV_SECTOR_SIZE;
+        nb_sectors -= n;
+        sector_num += n;
+        buf += n << BDRV_SECTOR_BITS;
     }
     return 0;
 }
-- 
1.9.1

[Qemu-devel] [PATCH 10/16] block/parallels: add get_block_status

[Denis V. Lunev]

From: Roman Kagan <rkagan@parallels.com>

Implement VFS method for get_block_status to Parallels format driver.

Signed-off-by: Roman Kagan <rkagan@parallels.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index a05bf39..2d3e962 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -384,6 +384,26 @@ static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num,
     return MIN(nb_sectors, ret);
 }
 
+static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
+        int64_t sector_num, int nb_sectors, int *pnum)
+{
+    BDRVParallelsState *s = bs->opaque;
+    int64_t offset;
+
+    qemu_co_mutex_lock(&s->lock);
+    offset = seek_to_sector(s, sector_num);
+    qemu_co_mutex_unlock(&s->lock);
+
+    *pnum = cluster_remainder(s, sector_num, nb_sectors);
+
+    if (offset < 0) {
+        return 0;
+    }
+
+    return (offset << BDRV_SECTOR_BITS) |
+        BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
+}
+
 static int parallels_read(BlockDriverState *bs, int64_t sector_num,
                     uint8_t *buf, int nb_sectors)
 {
@@ -433,6 +453,7 @@ static BlockDriver bdrv_parallels = {
     .bdrv_open		= parallels_open,
     .bdrv_read          = parallels_co_read,
     .bdrv_close		= parallels_close,
+    .bdrv_co_get_block_status = parallels_co_get_block_status,
 };
 
 static void bdrv_parallels_init(void)
-- 
1.9.1

Re: [Qemu-devel] [PATCH 10/16] block/parallels: add get_block_status

[Denis V. Lunev]

On 15/12/14 11:27, Denis V. Lunev wrote:
> From: Roman Kagan <rkagan@parallels.com>
>
> Implement VFS method for get_block_status to Parallels format driver.
>
> Signed-off-by: Roman Kagan <rkagan@parallels.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   block/parallels.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index a05bf39..2d3e962 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -384,6 +384,26 @@ static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num,
>       return MIN(nb_sectors, ret);
>   }
>   
> +static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
> +        int64_t sector_num, int nb_sectors, int *pnum)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    int64_t offset;
> +
> +    qemu_co_mutex_lock(&s->lock);
> +    offset = seek_to_sector(s, sector_num);
I have mistaken here at porting Roman's changes
on top of my changes. "padding" is not applied here
while it should.

Shame on me :(

Something safe should be invented to avoid this
mess.

> +    qemu_co_mutex_unlock(&s->lock);
> +
> +    *pnum = cluster_remainder(s, sector_num, nb_sectors);
> +
> +    if (offset < 0) {
> +        return 0;
> +    }
> +
> +    return (offset << BDRV_SECTOR_BITS) |
> +        BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
> +}
> +
>   static int parallels_read(BlockDriverState *bs, int64_t sector_num,
>                       uint8_t *buf, int nb_sectors)
>   {
> @@ -433,6 +453,7 @@ static BlockDriver bdrv_parallels = {
>       .bdrv_open		= parallels_open,
>       .bdrv_read          = parallels_co_read,
>       .bdrv_close		= parallels_close,
> +    .bdrv_co_get_block_status = parallels_co_get_block_status,
>   };
>   
>   static void bdrv_parallels_init(void)

Re: [Qemu-devel] [PATCH 10/16] block/parallels: add get_block_status

[Kevin Wolf]

Am 15.12.2014 um 12:52 hat Denis V. Lunev geschrieben:
> On 15/12/14 11:27, Denis V. Lunev wrote:
> >From: Roman Kagan <rkagan@parallels.com>
> >
> >Implement VFS method for get_block_status to Parallels format driver.
> >
> >Signed-off-by: Roman Kagan <rkagan@parallels.com>
> >Signed-off-by: Denis V. Lunev <den@openvz.org>
> >CC: Jeff Cody <jcody@redhat.com>
> >CC: Kevin Wolf <kwolf@redhat.com>
> >CC: Stefan Hajnoczi <stefanha@redhat.com>
> >---
> >  block/parallels.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >
> >diff --git a/block/parallels.c b/block/parallels.c
> >index a05bf39..2d3e962 100644
> >--- a/block/parallels.c
> >+++ b/block/parallels.c
> >@@ -384,6 +384,26 @@ static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num,
> >      return MIN(nb_sectors, ret);
> >  }
> >+static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
> >+        int64_t sector_num, int nb_sectors, int *pnum)
> >+{
> >+    BDRVParallelsState *s = bs->opaque;
> >+    int64_t offset;
> >+
> >+    qemu_co_mutex_lock(&s->lock);
> >+    offset = seek_to_sector(s, sector_num);
> I have mistaken here at porting Roman's changes
> on top of my changes. "padding" is not applied here
> while it should.
> 
> Shame on me :(
> 
> Something safe should be invented to avoid this
> mess.

Perhaps the application of padding should be moved into
seek_to_sector(). In this case, the following changes to parallels_read()
would follow:

* Reading from the backing file would not apply the padding. I think the
  padding is an internal property of this specific image file, so this
  is what actually should happen. In other words, a bug fix that you
  would have to do anyway.

* n = cluster_remainder(...) must be changed to be based on position
  (i.e. the position in the image file) rather than sector_num (i.e. the
  virtual offset as seen by the guest)

Kevin

[Qemu-devel] [PATCH 11/16] block/parallels: add support for backing files

[Denis V. Lunev]

From: Roman Kagan <rkagan@parallels.com>

Add backing file support to Parallels format driver.

That said, I think backing file operations should end up in the generic
block layer, but that's a longer story...

Signed-off-by: Roman Kagan <rkagan@parallels.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index 2d3e962..718274b 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -419,6 +419,11 @@ static int parallels_read(BlockDriverState *bs, int64_t sector_num,
             if (ret < 0) {
                 return ret;
             }
+        } else if (bs->backing_hd) {
+            int ret = bdrv_read(bs->backing_hd, sector_num, buf, n);
+            if (ret < 0) {
+                return ret;
+            }
         } else {
             memset(buf, 0, n << BDRV_SECTOR_BITS);
         }
@@ -454,6 +459,7 @@ static BlockDriver bdrv_parallels = {
     .bdrv_read          = parallels_co_read,
     .bdrv_close		= parallels_close,
     .bdrv_co_get_block_status = parallels_co_get_block_status,
+    .supports_backing  = true,
 };
 
 static void bdrv_parallels_init(void)
-- 
1.9.1

Re: [Qemu-devel] [PATCH 11/16] block/parallels: add support for backing files

[Kevin Wolf]

Am 15.12.2014 um 09:27 hat Denis V. Lunev geschrieben:
> From: Roman Kagan <rkagan@parallels.com>
> 
> Add backing file support to Parallels format driver.
> 
> That said, I think backing file operations should end up in the generic
> block layer, but that's a longer story...
> 
> Signed-off-by: Roman Kagan <rkagan@parallels.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>

How are users supposed to make use of this? bs->backing_file isn't set
during parallels_open(), so you generally dont' get a backing file.

Users might manually add -drive backing=..., but is there really no
support for storing the backing file in the image format? If so, perhaps
it's better not to support backing files at all here.

Kevin

Re: [Qemu-devel] [PATCH 11/16] block/parallels: add support for backing files

[Roman Kagan]

On Mon, Dec 15, 2014 at 01:30:03PM +0100, Kevin Wolf wrote:
> Am 15.12.2014 um 09:27 hat Denis V. Lunev geschrieben:
> > From: Roman Kagan <rkagan@parallels.com>
> > 
> > Add backing file support to Parallels format driver.
> > 
> > That said, I think backing file operations should end up in the generic
> > block layer, but that's a longer story...
> > 
> > Signed-off-by: Roman Kagan <rkagan@parallels.com>
> > Signed-off-by: Denis V. Lunev <den@openvz.org>
> > CC: Jeff Cody <jcody@redhat.com>
> > CC: Kevin Wolf <kwolf@redhat.com>
> > CC: Stefan Hajnoczi <stefanha@redhat.com>
> 
> How are users supposed to make use of this? bs->backing_file isn't set
> during parallels_open(), so you generally dont' get a backing file.
> 
> Users might manually add -drive backing=..., but is there really no
> support for storing the backing file in the image format? If so, perhaps
> it's better not to support backing files at all here.

Parallels virtual disks (as also used by ploop, http://openvz.org/Ploop)
consist of a descriptor xml file and one or more data files.  The
data files may have delta/backing relationships; those relationships are
only stored in the descriptor file.

The original parallels driver in qemu used to support direct access to
the data files.  This patch makes it possible for such a data file to be
a delta WRT a backing file, specified externally (see e.g. the patch to
the tests).

The code for parsing the descriptor xml and filling in the delta/backing
relations isn't there yet; we haven't yet figured out where it fits
best.  Still it makes up a perfectly valid usecase to figure out those
relations by some external means (e.g. eye inspection of the descriptor
file :) and construct appropriate JSON filename for use in any scenario
where a readonly image can be used in qemu.

Roman.

[Qemu-devel] [PATCH 12/16] iotests: testcase for backing in parallels format

[Denis V. Lunev]

From: Roman Kagan <rkagan@parallels.com>

This patch adds a testcase for reading from a backing file when both the
current image and the backing one are in parallels format.

Signed-off-by: Roman Kagan <rkagan@parallels.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/076     | 21 +++++++++++++++++++++
 tests/qemu-iotests/076.out |  4 ++++
 2 files changed, 25 insertions(+)

diff --git a/tests/qemu-iotests/076 b/tests/qemu-iotests/076
index 766b359..04b359b 100755
--- a/tests/qemu-iotests/076
+++ b/tests/qemu-iotests/076
@@ -31,6 +31,11 @@ status=1	# failure is the default!
 _cleanup()
 {
 	_cleanup_test_img
+
+   if [ -n "$TEST_IMG_BASE" ]
+   then
+       rm -f "$TEST_IMG_BASE"
+   fi
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
@@ -87,6 +92,22 @@ _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
 
+echo
+echo "== Read from an image with a backing image =="
+TEST_IMG_BASE="$TEST_IMG.base"
+_use_sample_img parallels-v1.bz2
+mv -f "$TEST_IMG" "$TEST_IMG_BASE"
+dd if="$TEST_IMG_BASE" of="$TEST_IMG" bs=64 count=1 &>/dev/null
+dd if=/dev/zero of="$TEST_IMG" bs=1M count=0 seek=1 conv=notrunc &>/dev/null
+{ $QEMU_IO -c "read -P 0x11 0 64k" \
+    "json:{
+        \"file\": {\"filename\": \"$TEST_IMG\"},
+        \"backing\":{
+            \"file\":{\"filename\":\"$TEST_IMG_BASE\"},
+            \"driver\":\"parallels\"
+        }
+    }"; } 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 46680d8..0cce00c 100644
--- a/tests/qemu-iotests/076.out
+++ b/tests/qemu-iotests/076.out
@@ -27,4 +27,8 @@ read 65536/65536 bytes at offset 0
 == 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)
+
+== Read from an image with a backing image ==
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 *** done
-- 
1.9.1

[Qemu-devel] [PATCH 13/16] block/parallels: read disk size from XML if DiskDescriptor.xml is passed

[Denis V. Lunev]

as an image filename. This is preparational commit to read snapshot
information from XML. This is the only global parameter which will be
kept in BDRVParallelsState, the rest should be layered down into
snapshots information structure.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 718274b..0c0e669 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -107,6 +107,7 @@ static int parallels_open_image(BlockDriverState *bs, Error **errp)
     int i;
     struct parallels_header ph;
     int ret;
+    int64_t total_sectors;
 
     bs->read_only = 1; // no write support yet
 
@@ -115,20 +116,35 @@ static int parallels_open_image(BlockDriverState *bs, Error **errp)
         goto fail;
     }
 
-    bs->total_sectors = le64_to_cpu(ph.nb_sectors);
+    total_sectors = le64_to_cpu(ph.nb_sectors);
 
     if (le32_to_cpu(ph.version) != HEADER_VERSION) {
         goto fail_format;
     }
     if (!memcmp(ph.magic, HEADER_MAGIC, 16)) {
         s->off_multiplier = 1;
-        bs->total_sectors = 0xffffffff & bs->total_sectors;
+        total_sectors = 0xffffffff & total_sectors;
     } else if (!memcmp(ph.magic, HEADER_MAGIC2, 16)) {
         s->off_multiplier = le32_to_cpu(ph.tracks);
     } else {
         goto fail_format;
     }
 
+    if (total_sectors == 0) {
+        error_setg(errp, "Invalid image: zero total sectors");
+        ret = -EINVAL;
+        goto fail;
+    }
+    if (bs->total_sectors == 0) {
+        /* no descriptor file, standalone image opened */
+        bs->total_sectors = total_sectors;
+    }
+    if (bs->total_sectors != total_sectors) {
+        error_setg(errp, "Invalid image: wrong total sectors");
+        ret = -EINVAL;
+        goto fail;
+    }
+
     s->tracks = le32_to_cpu(ph.tracks);
     if (s->tracks == 0) {
         error_setg(errp, "Invalid image: Zero sectors per track");
@@ -234,7 +250,7 @@ static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp)
     int size, ret;
     xmlDoc *doc = NULL;
     xmlNode *root, *image;
-    char *xml = NULL;
+    char *xml = NULL, *endptr;
     const char *data;
     char image_path[PATH_MAX];
     Error *local_err = NULL;
@@ -271,7 +287,6 @@ static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp)
 
     data = xml_get_text(root, "Disk_Parameters", "Padding", NULL);
     if (data != NULL) {
-        char *endptr;
         unsigned long pad;
 
         pad = strtoul(data, &endptr, 0);
@@ -281,6 +296,16 @@ static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp)
         s->padding = (uint32_t)pad;
     }
 
+    data = xml_get_text(root, "Disk_Parameters", "Disk_size", NULL);
+    if (data == NULL) {
+        goto fail;
+    } else {
+        bs->total_sectors = strtoull(data, &endptr, 0);
+        if (endptr != NULL && *endptr != '\0') {
+            goto fail;
+        }
+    }
+
     image = xml_seek(root, "StorageData", "Storage", "Image", NULL);
     data = ""; /* make gcc happy */
     for (size = 0; image != NULL; image = image->next) {
-- 
1.9.1

Re: [Qemu-devel] [PATCH 13/16] block/parallels: read disk size from XML if DiskDescriptor.xml is passed

[Kevin Wolf]

Am 15.12.2014 um 09:27 hat Denis V. Lunev geschrieben:
> as an image filename. This is preparational commit to read snapshot
> information from XML. This is the only global parameter which will be
> kept in BDRVParallelsState, the rest should be layered down into
> snapshots information structure.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 33 +++++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index 718274b..0c0e669 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -107,6 +107,7 @@ static int parallels_open_image(BlockDriverState *bs, Error **errp)
>      int i;
>      struct parallels_header ph;
>      int ret;
> +    int64_t total_sectors;
>  
>      bs->read_only = 1; // no write support yet
>  
> @@ -115,20 +116,35 @@ static int parallels_open_image(BlockDriverState *bs, Error **errp)
>          goto fail;
>      }
>  
> -    bs->total_sectors = le64_to_cpu(ph.nb_sectors);
> +    total_sectors = le64_to_cpu(ph.nb_sectors);
>  
>      if (le32_to_cpu(ph.version) != HEADER_VERSION) {
>          goto fail_format;
>      }
>      if (!memcmp(ph.magic, HEADER_MAGIC, 16)) {
>          s->off_multiplier = 1;
> -        bs->total_sectors = 0xffffffff & bs->total_sectors;
> +        total_sectors = 0xffffffff & total_sectors;
>      } else if (!memcmp(ph.magic, HEADER_MAGIC2, 16)) {
>          s->off_multiplier = le32_to_cpu(ph.tracks);
>      } else {
>          goto fail_format;
>      }
>  
> +    if (total_sectors == 0) {
> +        error_setg(errp, "Invalid image: zero total sectors");
> +        ret = -EINVAL;
> +        goto fail;
> +    }
> +    if (bs->total_sectors == 0) {
> +        /* no descriptor file, standalone image opened */
> +        bs->total_sectors = total_sectors;
> +    }
> +    if (bs->total_sectors != total_sectors) {
> +        error_setg(errp, "Invalid image: wrong total sectors");
> +        ret = -EINVAL;
> +        goto fail;
> +    }
> +
>      s->tracks = le32_to_cpu(ph.tracks);
>      if (s->tracks == 0) {
>          error_setg(errp, "Invalid image: Zero sectors per track");
> @@ -234,7 +250,7 @@ static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp)
>      int size, ret;
>      xmlDoc *doc = NULL;
>      xmlNode *root, *image;
> -    char *xml = NULL;
> +    char *xml = NULL, *endptr;
>      const char *data;
>      char image_path[PATH_MAX];
>      Error *local_err = NULL;
> @@ -271,7 +287,6 @@ static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp)
>  
>      data = xml_get_text(root, "Disk_Parameters", "Padding", NULL);
>      if (data != NULL) {
> -        char *endptr;
>          unsigned long pad;
>  
>          pad = strtoul(data, &endptr, 0);
> @@ -281,6 +296,16 @@ static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp)
>          s->padding = (uint32_t)pad;
>      }
>  
> +    data = xml_get_text(root, "Disk_Parameters", "Disk_size", NULL);
> +    if (data == NULL) {
> +        goto fail;
> +    } else {
> +        bs->total_sectors = strtoull(data, &endptr, 0);

Same comment about strtoull() error checks as in a previous patch.

> +        if (endptr != NULL && *endptr != '\0') {
> +            goto fail;
> +        }
> +    }
> +
>      image = xml_seek(root, "StorageData", "Storage", "Image", NULL);
>      data = ""; /* make gcc happy */
>      for (size = 0; image != NULL; image = image->next) {

Kevin

[Qemu-devel] [PATCH 14/16] block/parallels: introduce ParallelsSnapshot data structure

[Denis V. Lunev]

In order to support snapshots of parallels images we should maintain
snapshots list in BDRVParallelsState. Snapshots actually forms tree.
Though, in read-only case, we do need to traverse only from current
snapshot leaf to the snapshot tree root. Thus interesting for us
snapshots forms old good linked list.

This patch just introduces the structure for this and fills it with
a signle image as was done before.

True parsing be done in the next patch.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 110 ++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 73 insertions(+), 37 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 0c0e669..d1b52f3 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -56,15 +56,22 @@ struct parallels_header {
     char padding[12];
 } QEMU_PACKED;
 
-typedef struct BDRVParallelsState {
-    CoMutex lock;
+typedef struct ParallelsSnapshot {
+    BlockDriverState *file;
 
     uint32_t *catalog_bitmap;
     unsigned int catalog_size;
 
     unsigned int tracks;
-
     unsigned int off_multiplier;
+} ParallelsSnapshot;
+
+typedef struct BDRVParallelsState {
+    CoMutex lock;
+
+    ParallelsSnapshot *snaps;
+    unsigned int snaps_count;
+
     unsigned int padding;
 } BDRVParallelsState;
 
@@ -101,49 +108,37 @@ static int parallels_probe(const uint8_t *buf, int buf_size, const char *filenam
     return parallels_probe_xml(buf, buf_size);
 }
 
-static int parallels_open_image(BlockDriverState *bs, Error **errp)
+static int parallels_open_image(ParallelsSnapshot *s, int64_t *total_sectors,
+                                Error **errp)
 {
-    BDRVParallelsState *s = bs->opaque;
     int i;
     struct parallels_header ph;
     int ret;
-    int64_t total_sectors;
-
-    bs->read_only = 1; // no write support yet
 
-    ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph));
+    ret = bdrv_pread(s->file, 0, &ph, sizeof(ph));
     if (ret < 0) {
-        goto fail;
+        return ret;
     }
 
-    total_sectors = le64_to_cpu(ph.nb_sectors);
+    *total_sectors = le64_to_cpu(ph.nb_sectors);
 
     if (le32_to_cpu(ph.version) != HEADER_VERSION) {
         goto fail_format;
     }
     if (!memcmp(ph.magic, HEADER_MAGIC, 16)) {
         s->off_multiplier = 1;
-        total_sectors = 0xffffffff & total_sectors;
+        *total_sectors = 0xffffffff & *total_sectors;
     } else if (!memcmp(ph.magic, HEADER_MAGIC2, 16)) {
         s->off_multiplier = le32_to_cpu(ph.tracks);
     } else {
         goto fail_format;
     }
 
-    if (total_sectors == 0) {
+    if (*total_sectors == 0) {
         error_setg(errp, "Invalid image: zero total sectors");
         ret = -EINVAL;
         goto fail;
     }
-    if (bs->total_sectors == 0) {
-        /* no descriptor file, standalone image opened */
-        bs->total_sectors = total_sectors;
-    }
-    if (bs->total_sectors != total_sectors) {
-        error_setg(errp, "Invalid image: wrong total sectors");
-        ret = -EINVAL;
-        goto fail;
-    }
 
     s->tracks = le32_to_cpu(ph.tracks);
     if (s->tracks == 0) {
@@ -169,7 +164,7 @@ static int parallels_open_image(BlockDriverState *bs, Error **errp)
         goto fail;
     }
 
-    ret = bdrv_pread(bs->file, 64, s->catalog_bitmap, s->catalog_size * 4);
+    ret = bdrv_pread(s->file, 64, s->catalog_bitmap, s->catalog_size * 4);
     if (ret < 0) {
         goto fail;
     }
@@ -177,7 +172,6 @@ static int parallels_open_image(BlockDriverState *bs, Error **errp)
     for (i = 0; i < s->catalog_size; i++)
         le32_to_cpus(&s->catalog_bitmap[i]);
 
-    qemu_co_mutex_init(&s->lock);
     return 0;
 
 fail_format:
@@ -331,19 +325,25 @@ static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp)
         goto done;
     }
 
+    s->snaps_count = size;
+    s->snaps = g_malloc0(sizeof(ParallelsSnapshot) * s->snaps_count);
+
     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);
+    ret = bdrv_open(&s->snaps[0].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);
+        int64_t total_sectors;
+        ret = parallels_open_image(s->snaps, &total_sectors, errp);
+        if (bs->total_sectors != total_sectors) {
+            error_setg(errp, "Invalid image: wrong total sectors");
+            ret = -EINVAL;
+            goto done;
+        }
     }
 
 done:
@@ -367,17 +367,25 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     uint8_t buf[MAX(1024, HEADER_SIZE)];
     int size;
     const struct parallels_header *ph;
+    BDRVParallelsState *s = bs->opaque;
 
     size = bdrv_pread(bs->file, 0, buf, sizeof(buf));
     if (size < 0) {
         return size;
     }
 
+    qemu_co_mutex_init(&s->lock);
+    bs->read_only = 1;
+
     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);
+        s->snaps_count = 1;
+        s->snaps = g_malloc0(sizeof(ParallelsSnapshot));
+        s->snaps->file = bs->file;
+        bdrv_ref(bs->file);
+        return parallels_open_image(s->snaps, &bs->total_sectors, errp);
     } else if (parallels_probe_xml(buf, (unsigned)size)) {
 #if CONFIG_LIBXML2
         return parallels_open_xml(bs, flags, errp);
@@ -389,10 +397,14 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
 }
 
 
-static int64_t seek_to_sector(BDRVParallelsState *s, int64_t sector_num)
+static int64_t seek_to_sector(ParallelsSnapshot *s, int64_t sector_num)
 {
     uint32_t index, offset;
 
+    if (s == NULL) {
+        return -1;
+    }
+
     index = sector_num / s->tracks;
     offset = sector_num % s->tracks;
 
@@ -405,10 +417,27 @@ static int64_t seek_to_sector(BDRVParallelsState *s, int64_t sector_num)
 static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num,
         int nb_sectors)
 {
-    int ret = s->tracks - sector_num % s->tracks;
+    int ret = s->snaps->tracks - sector_num % s->snaps->tracks;
     return MIN(nb_sectors, ret);
 }
 
+static ParallelsSnapshot *find_snap(BDRVParallelsState *s, int64_t sector_num)
+{
+    int snap_idx;
+    uint32_t index = sector_num / s->snaps->tracks;
+
+    for (snap_idx = 0; snap_idx < s->snaps_count; snap_idx++) {
+        ParallelsSnapshot *snap = s->snaps + snap_idx;
+
+        if (index >= snap->catalog_size || snap->catalog_bitmap[index] == 0) {
+            continue;
+        }
+        return snap;
+    }
+
+    return NULL;
+}
+
 static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, int *pnum)
 {
@@ -416,7 +445,7 @@ static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
     int64_t offset;
 
     qemu_co_mutex_lock(&s->lock);
-    offset = seek_to_sector(s, sector_num);
+    offset = seek_to_sector(find_snap(s, sector_num), sector_num);
     qemu_co_mutex_unlock(&s->lock);
 
     *pnum = cluster_remainder(s, sector_num, nb_sectors);
@@ -437,10 +466,11 @@ static int parallels_read(BlockDriverState *bs, int64_t sector_num,
     sector_num += s->padding;
 
     while (nb_sectors > 0) {
-        int64_t position = seek_to_sector(s, sector_num);
+        ParallelsSnapshot *snap = find_snap(s, sector_num);
+        int64_t position = seek_to_sector(snap, sector_num);
         int n = cluster_remainder(s, sector_num, nb_sectors);
         if (position >= 0) {
-            int ret = bdrv_read(bs->file, position, buf, n);
+            int ret = bdrv_read(snap->file, position, buf, n);
             if (ret < 0) {
                 return ret;
             }
@@ -472,8 +502,14 @@ static coroutine_fn int parallels_co_read(BlockDriverState *bs, int64_t sector_n
 
 static void parallels_close(BlockDriverState *bs)
 {
+    int i;
     BDRVParallelsState *s = bs->opaque;
-    g_free(s->catalog_bitmap);
+
+    for (i = 0; i < s->snaps_count; i++) {
+        ParallelsSnapshot *snap = s->snaps + i;
+        g_free(snap->catalog_bitmap);
+        bdrv_unref(snap->file);
+    }
 }
 
 static BlockDriver bdrv_parallels = {
-- 
1.9.1

Re: [Qemu-devel] [PATCH 14/16] block/parallels: introduce ParallelsSnapshot data structure

[Kevin Wolf]

Am 15.12.2014 um 09:27 hat Denis V. Lunev geschrieben:
> In order to support snapshots of parallels images we should maintain
> snapshots list in BDRVParallelsState. Snapshots actually forms tree.
> Though, in read-only case, we do need to traverse only from current
> snapshot leaf to the snapshot tree root. Thus interesting for us
> snapshots forms old good linked list.
> 
> This patch just introduces the structure for this and fills it with
> a signle image as was done before.

s/signle/single/

> True parsing be done in the next patch.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>

If I understand correctly, this is what actually describes the backing
file relationship. We should probably use the normal infrastructure for
this.

The challenge here seems to be that the single descriptor XML file
describes the complete chain of backing files. This is different from
the image formats that we support until now.

I think we need some design discussion here first before we even look at
code. Did you consider making the snapshots regular backing files, and
if so, what were the reasons that let you prefer a purely internal
solution?

Kevin

Re: [Qemu-devel] [PATCH 14/16] block/parallels: introduce ParallelsSnapshot data structure

[Denis V. Lunev]

On 15/12/14 15:45, Kevin Wolf wrote:
> Am 15.12.2014 um 09:27 hat Denis V. Lunev geschrieben:
>> In order to support snapshots of parallels images we should maintain
>> snapshots list in BDRVParallelsState. Snapshots actually forms tree.
>> Though, in read-only case, we do need to traverse only from current
>> snapshot leaf to the snapshot tree root. Thus interesting for us
>> snapshots forms old good linked list.
>>
>> This patch just introduces the structure for this and fills it with
>> a signle image as was done before.
> s/signle/single/
>
>> True parsing be done in the next patch.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Jeff Cody <jcody@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
> If I understand correctly, this is what actually describes the backing
> file relationship. We should probably use the normal infrastructure for
> this.
>
> The challenge here seems to be that the single descriptor XML file
> describes the complete chain of backing files. This is different from
> the image formats that we support until now.
>
> I think we need some design discussion here first before we even look at
> code. Did you consider making the snapshots regular backing files, and
> if so, what were the reasons that let you prefer a purely internal
> solution?
>
> Kevin

This implementation is borrowed from the current VMDK support.
The idea is exactly the same, see below. I have taken this as
a source of architecture approach as format is quite similar.

Anyway, I am very open to a discussion and solid architecture
approach here would be very good.

Regards,
     Den

static int vmdk_probe(const uint8_t *buf, int buf_size, const char 
*filename)
{
     magic = be32_to_cpu(*(uint32_t *)buf);
     if (magic == VMDK3_MAGIC ||
         magic == VMDK4_MAGIC) {
         return 100;
     } else {
         /* test descriptor parsing */
     }
}


static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
                      Error **errp)
{
     char *buf;
     int ret;
     BDRVVmdkState *s = bs->opaque;
     uint32_t magic;

     buf = vmdk_read_desc(bs->file, 0, errp);
     if (!buf) {
         return -EINVAL;
     }

     magic = ldl_be_p(buf);
     switch (magic) {
         case VMDK3_MAGIC:
         case VMDK4_MAGIC:
             ret = vmdk_open_sparse(bs, bs->file, flags, buf, errp);
             s->desc_offset = 0x200;
             break;
         default:
             ret = vmdk_open_desc_file(bs, flags, buf, errp);
             break;
     }
     if (ret) {
         goto fail;
     }
     ...


static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
                                Error **errp)
{
    ....
    ret = vmdk_parse_extents(buf, bs, bs->file->filename, errp);
    ....
}

static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
                               const char *desc_file_path, Error **errp)
{
     ....
     while (*p) {
         ....

         path_combine(extent_path, sizeof(extent_path),
                 desc_file_path, fname);
         extent_file = NULL;
         ret = bdrv_open(&extent_file, extent_path, NULL, NULL,
                         bs->open_flags | BDRV_O_PROTOCOL, NULL, errp);
         ...
         /* save to extents array */
}

[Qemu-devel] [RFC PATCH 1/1] block/parallels: new concept for DiskDescriptor.xml

[Denis V. Lunev]

Actually we have 2 major options without intruduction of the new concept:
- follow VMDK approach (original approach in v4 patchset)
- chain backing stores in XML parsing code in additional block driver

This is very rough but working conceptual code with a new approach.
The patch should be applied on top of patch 1 of the original patchset.

The idea is to parse XML file in new "XML" block driver and open
real image as a backing store inside XML parsing code. The first image
will be read-write, other underlying ones will be readnly. Padding
will be passed through options dictionary.

I have not addressed any other things from the review.

Can you pls comment this?

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Roman Kagan <rkagan@parallels.com>
---
 block/parallels.c | 232 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 232 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index 4f9cd8d..bc0d683 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -27,6 +27,12 @@
 #include "block/block_int.h"
 #include "qemu/module.h"
 
+#if defined(CONFIG_LIBXML2)
+#include <libxml/parser.h>
+#include <libxml/tree.h>
+#endif
+#include <stdarg.h>
+
 /**************************************************************/
 
 #define HEADER_MAGIC "WithoutFreeSpace"
@@ -211,3 +217,229 @@ static void bdrv_parallels_init(void)
 }
 
 block_init(bdrv_parallels_init);
+
+
+#if defined(CONFIG_LIBXML2)
+
+typedef struct BDRVPrlXmlState {
+    xmlDoc *xml;
+    BlockDriverState *bi;
+} BDRVPrlXmlState;
+
+
+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 prl_probe_xml(const uint8_t *data, int buf_size, const char *fn)
+{
+    int score = 0;
+    char *buf = g_malloc(buf_size + 1);
+
+    memcpy(buf, data, buf_size);
+    buf[buf_size] = 0;
+    if (strstr(buf, "<?xml version=\"1.0\"?>") &&
+        strstr(buf, "<Parallels_disk_image>")) {
+        score = 100;
+    }
+    g_free(buf);
+    return score;
+}
+
+static int prl_open_xml(BlockDriverState *bs, QDict *opts, int fl, Error **errp)
+{
+    int64_t size;
+    int ret;
+    xmlDoc *doc = NULL;
+    xmlNode *root, *image;
+    char *xml = NULL;
+    const char *data;
+    char image_path[PATH_MAX];
+    Error *local_err = NULL;
+    BDRVPrlXmlState *s = bs->opaque;
+    BlockDriverState *backing_hd = NULL;
+
+    size = bdrv_getlength(bs->file);
+    if (size < 0) {
+        ret = (int)size;
+        goto fail;
+    }
+    /* XML file size should be reasonable */
+    if (size > 65536) {
+        ret = -EFBIG;
+        goto fail;
+    }
+
+    xml = g_malloc(size + 1);
+
+    ret = bdrv_pread(bs->file, 0, xml, size);
+    if (ret != size) {
+        g_free(xml);
+        goto fail;
+    }
+    xml[size] = 0;
+
+    ret = -EINVAL;
+    doc = xmlReadMemory(xml, size, NULL, NULL,
+                        XML_PARSE_NOERROR | XML_PARSE_NOWARNING);
+    g_free(xml);
+    if (doc == NULL) {
+        goto fail;
+    }
+    root = xmlDocGetRootElement(doc);
+    if (root == NULL) {
+        goto fail;
+    }
+
+    data = xml_get_text(root, "Disk_Parameters", "Disk_size", NULL);
+    if (data == NULL) {
+        goto fail;
+    } else {
+        char *endptr;
+        bs->total_sectors = strtoull(data, &endptr, 0);
+        if (*endptr != '\0') {
+            goto fail;
+        }
+    }
+
+    image = xml_seek(root, "StorageData", "Storage", "Image", NULL);
+    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", NULL);
+        if (data != NULL && strcmp(data, "Compressed")) {
+            error_setg(errp, "Only compressed Parallels images are supported");
+            goto done;
+        }
+
+        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);
+
+    bs->open_flags &= ~BDRV_O_NO_BACKING;
+
+    fl &= ~(BDRV_O_RDWR | BDRV_O_COPY_ON_READ | BDRV_O_SNAPSHOT |
+            BDRV_O_TEMPORARY);
+    backing_hd = bdrv_new();
+    ret = bdrv_open(&backing_hd, image_path, NULL, NULL,
+                    fl, NULL, &local_err);
+    if (ret < 0) {
+        bdrv_unref(backing_hd);
+        backing_hd = NULL;
+        bs->open_flags |= BDRV_O_NO_BACKING;
+        error_setg(errp, "Could not open backing file: %s",
+                   error_get_pretty(local_err));
+        error_free(local_err);
+        goto done;
+    }
+    bdrv_set_backing_hd(bs, backing_hd);
+
+    s->xml = doc;
+    doc = NULL;
+
+done:
+    if (doc != NULL) {
+        xmlFreeDoc(doc);
+    }
+    return ret;
+
+fail:
+    error_setg(errp, "Failed to parse Parallels disk descriptor XML");
+    goto done;
+}
+
+static coroutine_fn int
+prl_co_read(BlockDriverState *bs, int64_t sect, uint8_t *buf, int n)
+{
+    return bdrv_read(bs->backing_hd, sect, buf, n);
+}
+
+static void prl_close_xml(BlockDriverState *bs)
+{
+    BDRVPrlXmlState *s = bs->opaque;
+    xmlFreeDoc(s->xml);
+}
+
+static BlockDriver bdrv_prl_xml = {
+    .format_name    = "prl",
+    .instance_size  = sizeof(BDRVPrlXmlState),
+    .bdrv_probe     = prl_probe_xml,
+    .bdrv_open      = prl_open_xml,
+    .bdrv_read      = prl_co_read,
+    .bdrv_close     = prl_close_xml,
+    .supports_backing  = true,
+};
+
+static void bdrv_prl_init_xml(void)
+{
+    bdrv_register(&bdrv_prl_xml);
+}
+
+block_init(bdrv_prl_init_xml);
+
+#endif
-- 
1.9.1

[Qemu-devel] [PATCH 15/16] block/parallels: support read-only parallels snapshots

[Denis V. Lunev]

This patch reads snapshot information from the XML descriptor file.

The lastest snapshot (read-write one) could be located using the
following rules:
GUID could be used as a source to locate leaf in the snapshot tree:
- TopGUID tag is specified in the XML file
- {704718E1-2314-44c8-9087-D78ED36B0F4E} snapshot is present. Used for
  backups by Parallels
- {5FBAABE3-6958-40ff-92A7-860E329AAB41} snapshot

Without one of the snapshots above disk image is invalid.

Snapshots are read from top to bottom placing them into linear array,
which is already properly handled at the moment.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 102 +++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 77 insertions(+), 25 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index d1b52f3..93a8498 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -42,6 +42,10 @@
 #define HEADER_VERSION 2
 #define HEADER_SIZE 64
 
+#define CURRENT_SNAP_ID "{5FBAABE3-6958-40ff-92A7-860E329AAB41}"
+#define BACKUP_SNAP_ID  "{704718E1-2314-44c8-9087-D78ED36B0F4E}"
+
+
 // always little-endian
 struct parallels_header {
     char magic[16]; // "WithoutFreeSpace"
@@ -239,13 +243,34 @@ static const char *xml_get_text(xmlNode *node, ...)
     return NULL;
 }
 
+static xmlNode *xml_find_snapshot(xmlNode *snaps, const char *guid)
+{
+    if (guid == NULL) {
+        return NULL;
+    }
+
+    for (snaps = snaps->children; snaps != NULL; snaps = snaps->next) {
+        const char *id;
+        if (snaps->type != XML_ELEMENT_NODE) {
+            continue;
+        }
+        id = xml_get_text(snaps, "GUID", NULL);
+
+        if (!xmlStrcasecmp((const xmlChar *)guid, (const xmlChar *)id)) {
+            return snaps;
+        }
+    }
+    return NULL;
+}
+
 static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp)
 {
-    int size, ret;
+    int ret, size, i;
     xmlDoc *doc = NULL;
-    xmlNode *root, *image;
+    xmlNode *root, *storage, *snaps;
     char *xml = NULL, *endptr;
     const char *data;
+    const char *leaf_snap;
     char image_path[PATH_MAX];
     Error *local_err = NULL;
     BDRVParallelsState *s = bs->opaque;
@@ -300,14 +325,47 @@ static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp)
         }
     }
 
-    image = xml_seek(root, "StorageData", "Storage", "Image", NULL);
-    data = ""; /* make gcc happy */
-    for (size = 0; image != NULL; image = image->next) {
-        if (image->type != XML_ELEMENT_NODE) {
-            continue;
+    snaps = xml_seek(root, "Snapshots", NULL);
+
+    leaf_snap = xml_get_text(snaps, "TopGUID", NULL);
+    if (leaf_snap != NULL && xml_find_snapshot(snaps, leaf_snap) == NULL) {
+        goto fail;
+    } else if (xml_find_snapshot(snaps, BACKUP_SNAP_ID) != NULL) {
+        leaf_snap = BACKUP_SNAP_ID;
+    } else if (xml_find_snapshot(snaps, CURRENT_SNAP_ID) != NULL) {
+        leaf_snap = CURRENT_SNAP_ID;
+    } else {
+        goto fail;
+    }
+
+    data = leaf_snap;
+    for (s->snaps_count = 0; data != NULL; s->snaps_count++) {
+        xmlNode *node = xml_find_snapshot(snaps, data);
+        if (node == NULL) {
+            break;
+        }
+        data = xml_get_text(node, "ParentGUID", NULL);
+    }
+    if (s->snaps_count == 0) {
+        goto fail;
+    }
+    s->snaps = g_malloc0(sizeof(ParallelsSnapshot) * s->snaps_count);
+
+    storage = xml_seek(root, "StorageData", "Storage", NULL);
+    if (storage == NULL) {
+        goto fail;
+    }
+
+    data = leaf_snap;
+    for (i = 0; i < s->snaps_count; i++) {
+        int64_t total_sectors;
+        xmlNode *image = xml_find_snapshot(storage, data);
+        xmlNode *snap = xml_find_snapshot(snaps, data);
+
+        if (image == NULL || snap == NULL) {
+            goto fail;
         }
 
-        size++;
         data = xml_get_text(image, "Type", NULL);
         if (data != NULL && strcmp(data, "Compressed")) {
             error_setg(errp, "Only compressed Parallels images are supported");
@@ -318,32 +376,26 @@ static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp)
         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;
-    }
 
-    s->snaps_count = size;
-    s->snaps = g_malloc0(sizeof(ParallelsSnapshot) * s->snaps_count);
+        path_combine(image_path, sizeof(image_path), bs->file->filename, data);
 
-    path_combine(image_path, sizeof(image_path), bs->file->filename, data);
+        ret = bdrv_open(&s->snaps[i].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);
+            goto done;
+        }
 
-    ret = bdrv_open(&s->snaps[0].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 {
-        int64_t total_sectors;
         ret = parallels_open_image(s->snaps, &total_sectors, errp);
         if (bs->total_sectors != total_sectors) {
             error_setg(errp, "Invalid image: wrong total sectors");
             ret = -EINVAL;
             goto done;
         }
+
+        data = xml_get_text(snap, "ParentGUID", NULL);
     }
 
 done:
-- 
1.9.1

[Qemu-devel] [PATCH 16/16] iotests: testcase parallels image with snapshots

[Denis V. Lunev]

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/076                                    |   7 +++++++
 tests/qemu-iotests/076.out                                |   4 ++++
 .../qemu-iotests/sample_images/parallels-snapshot.xml.bz2 | Bin 0 -> 458 bytes
 tests/qemu-iotests/sample_images/parallels-v2-empty.bz2   | Bin 0 -> 93 bytes
 4 files changed, 11 insertions(+)
 create mode 100644 tests/qemu-iotests/sample_images/parallels-snapshot.xml.bz2
 create mode 100644 tests/qemu-iotests/sample_images/parallels-v2-empty.bz2

diff --git a/tests/qemu-iotests/076 b/tests/qemu-iotests/076
index 04b359b..6077c5b 100755
--- a/tests/qemu-iotests/076
+++ b/tests/qemu-iotests/076
@@ -108,6 +108,13 @@ dd if=/dev/zero of="$TEST_IMG" bs=1M count=0 seek=1 conv=notrunc &>/dev/null
         }
     }"; } 2>&1 | _filter_qemu_io | _filter_testdir
 
+echo
+echo "== Read from a valid parallels bundle with snapshots =="
+_use_sample_img parallels-v2.bz2
+_use_sample_img parallels-v2-empty.bz2
+_use_sample_img parallels-snapshot.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 0cce00c..f4c95ad 100644
--- a/tests/qemu-iotests/076.out
+++ b/tests/qemu-iotests/076.out
@@ -31,4 +31,8 @@ read 65536/65536 bytes at offset 0
 == Read from an image with a backing 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 parallels bundle with snapshots ==
+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-snapshot.xml.bz2 b/tests/qemu-iotests/sample_images/parallels-snapshot.xml.bz2
new file mode 100644
index 0000000000000000000000000000000000000000..2cee3ad35e1096eddd08a52fbb61ed92d8f84e33
GIT binary patch
literal 458
zcmV;*0X6<YT4*^jL0KkKSs#6@iU0uU-+)w5Py_$xpWsdazwh08KmppUb4`Sj2_w@?
zp!GjU$sV8?7>yY;4?!uC5=W!~pwmqNdVmIiG#gT<seuDc8UO$Q000W2lTAmdr>W^Q
z1JVEh4H_9e5;5=-G8)1JrvQZjB$*s+y$l6XVp3s-T2`gi^h(f5gh_-+f3QV8WGgLB
zS!qx%Ua8~MBvOuN#w0f!5gh`NFy&BuVj_7U!;s}g>ZKJ*y+uJ{OtMcE%%Un6Au^o0
z1OKoG-eSH5vlyz~Jqul0r<_S9#YQ-zZBY5Z;4_Ep<eC@O8Bsuu6D6!XD_NkfY%Yl8
z=D}qf@**rb5=cg&`5@$!IeG|&F(G@1Vo3^RDVa>AnGvC4PH4*oZpiJ91ZP^(zEde2
zjEo|;3(6~xSeWsPqV<L*>{OwJ#K6Y9S6&|joZSN<!-tPP&%)9Hs=$>j+g@fQy_HeC
z$c2TCCi>NiZcvp7XIZGI%*1GAJPMSx^RUh}>WWRJ>>8INS}S`kF}D*)WE`e4Qbd@{
z(Y<?3l4LOxzuGO5HB0KDWeN(&j2TIZ?>=_(*jN;VlJd~agPp`($rRy2Kz;VID8>iI
AmH+?%

literal 0
HcmV?d00001

diff --git a/tests/qemu-iotests/sample_images/parallels-v2-empty.bz2 b/tests/qemu-iotests/sample_images/parallels-v2-empty.bz2
new file mode 100644
index 0000000000000000000000000000000000000000..47ce6984d64bf8e740e5cbbebff7f619471cb7cc
GIT binary patch
literal 93
zcmV-j0HXgwT4*^jL0KkKS&I=ybpQepcf`>^Kmb4h0{{qsDqz+?KmY&;00<xeK$3*@
z00HTgC^Tr$WCo1YkN^m+sMgBKBmka>=Kan6jQ)3diJ9;~0r<O;DZ+$=Sc)^Kk}4$Q

literal 0
HcmV?d00001

-- 
1.9.1