* [Qemu-devel] [PATCH v4 0/16] parallels format support improvements
@ 2014-12-15 8:27 Denis V. Lunev
2014-12-15 8:27 ` [Qemu-devel] [PATCH 01/16] configure: add dependency from libxml2 Denis V. Lunev
` (15 more replies)
0 siblings, 16 replies; 33+ messages in thread
From: Denis V. Lunev @ 2014-12-15 8:27 UTC (permalink / raw
Cc: Kevin Wolf, Roman Kagan, Jeff Cody, qemu-devel, Stefan Hajnoczi,
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>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 01/16] configure: add dependency from libxml2
2014-12-15 8:27 [Qemu-devel] [PATCH v4 0/16] parallels format support improvements Denis V. Lunev
@ 2014-12-15 8:27 ` Denis V. Lunev
2014-12-15 8:27 ` [Qemu-devel] [PATCH 02/16] block/parallels: allow to specify DiskDescriptor.xml instead of image file Denis V. Lunev
` (14 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Denis V. Lunev @ 2014-12-15 8:27 UTC (permalink / raw
Cc: Kevin Wolf, Jeff Cody, Michael Tokarev, qemu-devel,
Stefan Hajnoczi, Paolo Bonzini, 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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 02/16] block/parallels: allow to specify DiskDescriptor.xml instead of image file
2014-12-15 8:27 [Qemu-devel] [PATCH v4 0/16] parallels format support improvements Denis V. Lunev
2014-12-15 8:27 ` [Qemu-devel] [PATCH 01/16] configure: add dependency from libxml2 Denis V. Lunev
@ 2014-12-15 8:27 ` Denis V. Lunev
2014-12-15 10:45 ` Kevin Wolf
2014-12-15 8:27 ` [Qemu-devel] [PATCH 03/16] iotests, parallels: quote TEST_IMG in 076 test to be path-safe Denis V. Lunev
` (13 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Denis V. Lunev @ 2014-12-15 8:27 UTC (permalink / raw
Cc: Kevin Wolf, Denis V. Lunev, Jeff Cody, qemu-devel,
Stefan Hajnoczi
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 03/16] iotests, parallels: quote TEST_IMG in 076 test to be path-safe
2014-12-15 8:27 [Qemu-devel] [PATCH v4 0/16] parallels format support improvements Denis V. Lunev
2014-12-15 8:27 ` [Qemu-devel] [PATCH 01/16] configure: add dependency from libxml2 Denis V. Lunev
2014-12-15 8:27 ` [Qemu-devel] [PATCH 02/16] block/parallels: allow to specify DiskDescriptor.xml instead of image file Denis V. Lunev
@ 2014-12-15 8:27 ` Denis V. Lunev
2014-12-15 8:27 ` [Qemu-devel] [PATCH 04/16] iotests: simple parallels XML disk descriptor file test added Denis V. Lunev
` (12 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Denis V. Lunev @ 2014-12-15 8:27 UTC (permalink / raw
Cc: Kevin Wolf, Denis V. Lunev, Jeff Cody, qemu-devel,
Stefan Hajnoczi
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 04/16] iotests: simple parallels XML disk descriptor file test added
2014-12-15 8:27 [Qemu-devel] [PATCH v4 0/16] parallels format support improvements Denis V. Lunev
` (2 preceding siblings ...)
2014-12-15 8:27 ` [Qemu-devel] [PATCH 03/16] iotests, parallels: quote TEST_IMG in 076 test to be path-safe Denis V. Lunev
@ 2014-12-15 8:27 ` Denis V. Lunev
2014-12-15 10:49 ` Kevin Wolf
2014-12-15 8:27 ` [Qemu-devel] [PATCH 05/16] block/parallels: support padded Parallels images Denis V. Lunev
` (11 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Denis V. Lunev @ 2014-12-15 8:27 UTC (permalink / raw
Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 05/16] block/parallels: support padded Parallels images
2014-12-15 8:27 [Qemu-devel] [PATCH v4 0/16] parallels format support improvements Denis V. Lunev
` (3 preceding siblings ...)
2014-12-15 8:27 ` [Qemu-devel] [PATCH 04/16] iotests: simple parallels XML disk descriptor file test added Denis V. Lunev
@ 2014-12-15 8:27 ` Denis V. Lunev
2014-12-15 11:05 ` Kevin Wolf
2014-12-15 8:27 ` [Qemu-devel] [PATCH 06/16] iotests: padded parallels image test Denis V. Lunev
` (10 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Denis V. Lunev @ 2014-12-15 8:27 UTC (permalink / raw
Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 06/16] iotests: padded parallels image test
2014-12-15 8:27 [Qemu-devel] [PATCH v4 0/16] parallels format support improvements Denis V. Lunev
` (4 preceding siblings ...)
2014-12-15 8:27 ` [Qemu-devel] [PATCH 05/16] block/parallels: support padded Parallels images Denis V. Lunev
@ 2014-12-15 8:27 ` Denis V. Lunev
2014-12-15 8:27 ` [Qemu-devel] [PATCH 07/16] parallels: change copyright information in the image header Denis V. Lunev
` (9 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Denis V. Lunev @ 2014-12-15 8:27 UTC (permalink / raw
Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 07/16] parallels: change copyright information in the image header
2014-12-15 8:27 [Qemu-devel] [PATCH v4 0/16] parallels format support improvements Denis V. Lunev
` (5 preceding siblings ...)
2014-12-15 8:27 ` [Qemu-devel] [PATCH 06/16] iotests: padded parallels image test Denis V. Lunev
@ 2014-12-15 8:27 ` Denis V. Lunev
2014-12-15 11:06 ` Kevin Wolf
2014-12-15 8:27 ` [Qemu-devel] [PATCH 08/16] block/parallels: switch to bdrv_read Denis V. Lunev
` (8 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Denis V. Lunev @ 2014-12-15 8:27 UTC (permalink / raw
Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 08/16] block/parallels: switch to bdrv_read
2014-12-15 8:27 [Qemu-devel] [PATCH v4 0/16] parallels format support improvements Denis V. Lunev
` (6 preceding siblings ...)
2014-12-15 8:27 ` [Qemu-devel] [PATCH 07/16] parallels: change copyright information in the image header Denis V. Lunev
@ 2014-12-15 8:27 ` Denis V. Lunev
2014-12-15 8:27 ` [Qemu-devel] [PATCH 09/16] block/parallels: read up to cluster end in one go Denis V. Lunev
` (7 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Denis V. Lunev @ 2014-12-15 8:27 UTC (permalink / raw
Cc: Kevin Wolf, Roman Kagan, Jeff Cody, qemu-devel, Stefan Hajnoczi,
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 09/16] block/parallels: read up to cluster end in one go
2014-12-15 8:27 [Qemu-devel] [PATCH v4 0/16] parallels format support improvements Denis V. Lunev
` (7 preceding siblings ...)
2014-12-15 8:27 ` [Qemu-devel] [PATCH 08/16] block/parallels: switch to bdrv_read Denis V. Lunev
@ 2014-12-15 8:27 ` Denis V. Lunev
2014-12-15 8:27 ` [Qemu-devel] [PATCH 10/16] block/parallels: add get_block_status Denis V. Lunev
` (6 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Denis V. Lunev @ 2014-12-15 8:27 UTC (permalink / raw
Cc: Kevin Wolf, Roman Kagan, Jeff Cody, qemu-devel, Stefan Hajnoczi,
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 10/16] block/parallels: add get_block_status
2014-12-15 8:27 [Qemu-devel] [PATCH v4 0/16] parallels format support improvements Denis V. Lunev
` (8 preceding siblings ...)
2014-12-15 8:27 ` [Qemu-devel] [PATCH 09/16] block/parallels: read up to cluster end in one go Denis V. Lunev
@ 2014-12-15 8:27 ` Denis V. Lunev
2014-12-15 11:52 ` Denis V. Lunev
2014-12-15 8:27 ` [Qemu-devel] [PATCH 11/16] block/parallels: add support for backing files Denis V. Lunev
` (5 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Denis V. Lunev @ 2014-12-15 8:27 UTC (permalink / raw
Cc: Kevin Wolf, Roman Kagan, Jeff Cody, qemu-devel, Stefan Hajnoczi,
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 11/16] block/parallels: add support for backing files
2014-12-15 8:27 [Qemu-devel] [PATCH v4 0/16] parallels format support improvements Denis V. Lunev
` (9 preceding siblings ...)
2014-12-15 8:27 ` [Qemu-devel] [PATCH 10/16] block/parallels: add get_block_status Denis V. Lunev
@ 2014-12-15 8:27 ` Denis V. Lunev
2014-12-15 12:30 ` Kevin Wolf
2014-12-15 8:27 ` [Qemu-devel] [PATCH 12/16] iotests: testcase for backing in parallels format Denis V. Lunev
` (4 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Denis V. Lunev @ 2014-12-15 8:27 UTC (permalink / raw
Cc: Kevin Wolf, Roman Kagan, Jeff Cody, qemu-devel, Stefan Hajnoczi,
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 12/16] iotests: testcase for backing in parallels format
2014-12-15 8:27 [Qemu-devel] [PATCH v4 0/16] parallels format support improvements Denis V. Lunev
` (10 preceding siblings ...)
2014-12-15 8:27 ` [Qemu-devel] [PATCH 11/16] block/parallels: add support for backing files Denis V. Lunev
@ 2014-12-15 8:27 ` Denis V. Lunev
2014-12-15 8:27 ` [Qemu-devel] [PATCH 13/16] block/parallels: read disk size from XML if DiskDescriptor.xml is passed Denis V. Lunev
` (3 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Denis V. Lunev @ 2014-12-15 8:27 UTC (permalink / raw
Cc: Kevin Wolf, Roman Kagan, Jeff Cody, qemu-devel, Stefan Hajnoczi,
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 13/16] block/parallels: read disk size from XML if DiskDescriptor.xml is passed
2014-12-15 8:27 [Qemu-devel] [PATCH v4 0/16] parallels format support improvements Denis V. Lunev
` (11 preceding siblings ...)
2014-12-15 8:27 ` [Qemu-devel] [PATCH 12/16] iotests: testcase for backing in parallels format Denis V. Lunev
@ 2014-12-15 8:27 ` Denis V. Lunev
2014-12-15 12:38 ` Kevin Wolf
2014-12-15 8:27 ` [Qemu-devel] [PATCH 14/16] block/parallels: introduce ParallelsSnapshot data structure Denis V. Lunev
` (2 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Denis V. Lunev @ 2014-12-15 8:27 UTC (permalink / raw
Cc: Kevin Wolf, Denis V. Lunev, Jeff Cody, qemu-devel,
Stefan Hajnoczi
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 14/16] block/parallels: introduce ParallelsSnapshot data structure
2014-12-15 8:27 [Qemu-devel] [PATCH v4 0/16] parallels format support improvements Denis V. Lunev
` (12 preceding siblings ...)
2014-12-15 8:27 ` [Qemu-devel] [PATCH 13/16] block/parallels: read disk size from XML if DiskDescriptor.xml is passed Denis V. Lunev
@ 2014-12-15 8:27 ` Denis V. Lunev
2014-12-15 12:45 ` Kevin Wolf
2014-12-15 8:28 ` [Qemu-devel] [PATCH 15/16] block/parallels: support read-only parallels snapshots Denis V. Lunev
2014-12-15 8:28 ` [Qemu-devel] [PATCH 16/16] iotests: testcase parallels image with snapshots Denis V. Lunev
15 siblings, 1 reply; 33+ messages in thread
From: Denis V. Lunev @ 2014-12-15 8:27 UTC (permalink / raw
Cc: Kevin Wolf, Denis V. Lunev, Jeff Cody, qemu-devel,
Stefan Hajnoczi
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 15/16] block/parallels: support read-only parallels snapshots
2014-12-15 8:27 [Qemu-devel] [PATCH v4 0/16] parallels format support improvements Denis V. Lunev
` (13 preceding siblings ...)
2014-12-15 8:27 ` [Qemu-devel] [PATCH 14/16] block/parallels: introduce ParallelsSnapshot data structure Denis V. Lunev
@ 2014-12-15 8:28 ` Denis V. Lunev
2014-12-15 8:28 ` [Qemu-devel] [PATCH 16/16] iotests: testcase parallels image with snapshots Denis V. Lunev
15 siblings, 0 replies; 33+ messages in thread
From: Denis V. Lunev @ 2014-12-15 8:28 UTC (permalink / raw
Cc: Kevin Wolf, Denis V. Lunev, Jeff Cody, qemu-devel,
Stefan Hajnoczi
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 16/16] iotests: testcase parallels image with snapshots
2014-12-15 8:27 [Qemu-devel] [PATCH v4 0/16] parallels format support improvements Denis V. Lunev
` (14 preceding siblings ...)
2014-12-15 8:28 ` [Qemu-devel] [PATCH 15/16] block/parallels: support read-only parallels snapshots Denis V. Lunev
@ 2014-12-15 8:28 ` Denis V. Lunev
15 siblings, 0 replies; 33+ messages in thread
From: Denis V. Lunev @ 2014-12-15 8:28 UTC (permalink / raw
Cc: Kevin Wolf, Denis V. Lunev, Jeff Cody, qemu-devel,
Stefan Hajnoczi
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 02/16] block/parallels: allow to specify DiskDescriptor.xml instead of image file
2014-12-15 8:27 ` [Qemu-devel] [PATCH 02/16] block/parallels: allow to specify DiskDescriptor.xml instead of image file Denis V. Lunev
@ 2014-12-15 10:45 ` Kevin Wolf
2014-12-15 11:51 ` Denis V. Lunev
0 siblings, 1 reply; 33+ messages in thread
From: Kevin Wolf @ 2014-12-15 10:45 UTC (permalink / raw
To: Denis V. Lunev; +Cc: Jeff Cody, qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 04/16] iotests: simple parallels XML disk descriptor file test added
2014-12-15 8:27 ` [Qemu-devel] [PATCH 04/16] iotests: simple parallels XML disk descriptor file test added Denis V. Lunev
@ 2014-12-15 10:49 ` Kevin Wolf
0 siblings, 0 replies; 33+ messages in thread
From: Kevin Wolf @ 2014-12-15 10:49 UTC (permalink / raw
To: Denis V. Lunev; +Cc: qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 05/16] block/parallels: support padded Parallels images
2014-12-15 8:27 ` [Qemu-devel] [PATCH 05/16] block/parallels: support padded Parallels images Denis V. Lunev
@ 2014-12-15 11:05 ` Kevin Wolf
2014-12-15 11:33 ` Denis V. Lunev
0 siblings, 1 reply; 33+ messages in thread
From: Kevin Wolf @ 2014-12-15 11:05 UTC (permalink / raw
To: Denis V. Lunev; +Cc: qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 07/16] parallels: change copyright information in the image header
2014-12-15 8:27 ` [Qemu-devel] [PATCH 07/16] parallels: change copyright information in the image header Denis V. Lunev
@ 2014-12-15 11:06 ` Kevin Wolf
2014-12-15 11:52 ` Denis V. Lunev
2014-12-16 16:29 ` Denis V. Lunev
0 siblings, 2 replies; 33+ messages in thread
From: Kevin Wolf @ 2014-12-15 11:06 UTC (permalink / raw
To: Denis V. Lunev; +Cc: qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 05/16] block/parallels: support padded Parallels images
2014-12-15 11:05 ` Kevin Wolf
@ 2014-12-15 11:33 ` Denis V. Lunev
0 siblings, 0 replies; 33+ messages in thread
From: Denis V. Lunev @ 2014-12-15 11:33 UTC (permalink / raw
To: Kevin Wolf; +Cc: qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 02/16] block/parallels: allow to specify DiskDescriptor.xml instead of image file
2014-12-15 10:45 ` Kevin Wolf
@ 2014-12-15 11:51 ` Denis V. Lunev
0 siblings, 0 replies; 33+ messages in thread
From: Denis V. Lunev @ 2014-12-15 11:51 UTC (permalink / raw
To: Kevin Wolf; +Cc: Jeff Cody, qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 07/16] parallels: change copyright information in the image header
2014-12-15 11:06 ` Kevin Wolf
@ 2014-12-15 11:52 ` Denis V. Lunev
2014-12-16 16:29 ` Denis V. Lunev
1 sibling, 0 replies; 33+ messages in thread
From: Denis V. Lunev @ 2014-12-15 11:52 UTC (permalink / raw
To: Kevin Wolf; +Cc: qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 10/16] block/parallels: add get_block_status
2014-12-15 8:27 ` [Qemu-devel] [PATCH 10/16] block/parallels: add get_block_status Denis V. Lunev
@ 2014-12-15 11:52 ` Denis V. Lunev
2014-12-15 12:18 ` Kevin Wolf
0 siblings, 1 reply; 33+ messages in thread
From: Denis V. Lunev @ 2014-12-15 11:52 UTC (permalink / raw
Cc: Kevin Wolf, Jeff Cody, qemu-devel, Stefan Hajnoczi, Roman Kagan
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)
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 10/16] block/parallels: add get_block_status
2014-12-15 11:52 ` Denis V. Lunev
@ 2014-12-15 12:18 ` Kevin Wolf
0 siblings, 0 replies; 33+ messages in thread
From: Kevin Wolf @ 2014-12-15 12:18 UTC (permalink / raw
To: Denis V. Lunev; +Cc: Jeff Cody, qemu-devel, Stefan Hajnoczi, Roman Kagan
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 11/16] block/parallels: add support for backing files
2014-12-15 8:27 ` [Qemu-devel] [PATCH 11/16] block/parallels: add support for backing files Denis V. Lunev
@ 2014-12-15 12:30 ` Kevin Wolf
2014-12-15 13:08 ` Roman Kagan
0 siblings, 1 reply; 33+ messages in thread
From: Kevin Wolf @ 2014-12-15 12:30 UTC (permalink / raw
To: Denis V. Lunev; +Cc: Jeff Cody, qemu-devel, Stefan Hajnoczi, Roman Kagan
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 13/16] block/parallels: read disk size from XML if DiskDescriptor.xml is passed
2014-12-15 8:27 ` [Qemu-devel] [PATCH 13/16] block/parallels: read disk size from XML if DiskDescriptor.xml is passed Denis V. Lunev
@ 2014-12-15 12:38 ` Kevin Wolf
0 siblings, 0 replies; 33+ messages in thread
From: Kevin Wolf @ 2014-12-15 12:38 UTC (permalink / raw
To: Denis V. Lunev; +Cc: Jeff Cody, qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 14/16] block/parallels: introduce ParallelsSnapshot data structure
2014-12-15 8:27 ` [Qemu-devel] [PATCH 14/16] block/parallels: introduce ParallelsSnapshot data structure Denis V. Lunev
@ 2014-12-15 12:45 ` Kevin Wolf
2014-12-15 13:32 ` Denis V. Lunev
2014-12-17 16:15 ` [Qemu-devel] [RFC PATCH 1/1] block/parallels: new concept for DiskDescriptor.xml Denis V. Lunev
0 siblings, 2 replies; 33+ messages in thread
From: Kevin Wolf @ 2014-12-15 12:45 UTC (permalink / raw
To: Denis V. Lunev; +Cc: Jeff Cody, qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 11/16] block/parallels: add support for backing files
2014-12-15 12:30 ` Kevin Wolf
@ 2014-12-15 13:08 ` Roman Kagan
0 siblings, 0 replies; 33+ messages in thread
From: Roman Kagan @ 2014-12-15 13:08 UTC (permalink / raw
To: Kevin Wolf; +Cc: Denis V. Lunev, Jeff Cody, qemu-devel, Stefan Hajnoczi
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.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 14/16] block/parallels: introduce ParallelsSnapshot data structure
2014-12-15 12:45 ` Kevin Wolf
@ 2014-12-15 13:32 ` Denis V. Lunev
2014-12-17 16:15 ` [Qemu-devel] [RFC PATCH 1/1] block/parallels: new concept for DiskDescriptor.xml Denis V. Lunev
1 sibling, 0 replies; 33+ messages in thread
From: Denis V. Lunev @ 2014-12-15 13:32 UTC (permalink / raw
To: Kevin Wolf; +Cc: Jeff Cody, qemu-devel, Stefan Hajnoczi
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 */
}
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 07/16] parallels: change copyright information in the image header
2014-12-15 11:06 ` Kevin Wolf
2014-12-15 11:52 ` Denis V. Lunev
@ 2014-12-16 16:29 ` Denis V. Lunev
1 sibling, 0 replies; 33+ messages in thread
From: Denis V. Lunev @ 2014-12-16 16:29 UTC (permalink / raw
To: Kevin Wolf; +Cc: qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [RFC PATCH 1/1] block/parallels: new concept for DiskDescriptor.xml
2014-12-15 12:45 ` Kevin Wolf
2014-12-15 13:32 ` Denis V. Lunev
@ 2014-12-17 16:15 ` Denis V. Lunev
1 sibling, 0 replies; 33+ messages in thread
From: Denis V. Lunev @ 2014-12-17 16:15 UTC (permalink / raw
Cc: Kevin Wolf, Roman Kagan, Jeff Cody, qemu-devel, Stefan Hajnoczi,
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
end of thread, other threads:[~2014-12-17 16:15 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-12-15 8:27 [Qemu-devel] [PATCH v4 0/16] parallels format support improvements Denis V. Lunev
2014-12-15 8:27 ` [Qemu-devel] [PATCH 01/16] configure: add dependency from libxml2 Denis V. Lunev
2014-12-15 8:27 ` [Qemu-devel] [PATCH 02/16] block/parallels: allow to specify DiskDescriptor.xml instead of image file Denis V. Lunev
2014-12-15 10:45 ` Kevin Wolf
2014-12-15 11:51 ` Denis V. Lunev
2014-12-15 8:27 ` [Qemu-devel] [PATCH 03/16] iotests, parallels: quote TEST_IMG in 076 test to be path-safe Denis V. Lunev
2014-12-15 8:27 ` [Qemu-devel] [PATCH 04/16] iotests: simple parallels XML disk descriptor file test added Denis V. Lunev
2014-12-15 10:49 ` Kevin Wolf
2014-12-15 8:27 ` [Qemu-devel] [PATCH 05/16] block/parallels: support padded Parallels images Denis V. Lunev
2014-12-15 11:05 ` Kevin Wolf
2014-12-15 11:33 ` Denis V. Lunev
2014-12-15 8:27 ` [Qemu-devel] [PATCH 06/16] iotests: padded parallels image test Denis V. Lunev
2014-12-15 8:27 ` [Qemu-devel] [PATCH 07/16] parallels: change copyright information in the image header Denis V. Lunev
2014-12-15 11:06 ` Kevin Wolf
2014-12-15 11:52 ` Denis V. Lunev
2014-12-16 16:29 ` Denis V. Lunev
2014-12-15 8:27 ` [Qemu-devel] [PATCH 08/16] block/parallels: switch to bdrv_read Denis V. Lunev
2014-12-15 8:27 ` [Qemu-devel] [PATCH 09/16] block/parallels: read up to cluster end in one go Denis V. Lunev
2014-12-15 8:27 ` [Qemu-devel] [PATCH 10/16] block/parallels: add get_block_status Denis V. Lunev
2014-12-15 11:52 ` Denis V. Lunev
2014-12-15 12:18 ` Kevin Wolf
2014-12-15 8:27 ` [Qemu-devel] [PATCH 11/16] block/parallels: add support for backing files Denis V. Lunev
2014-12-15 12:30 ` Kevin Wolf
2014-12-15 13:08 ` Roman Kagan
2014-12-15 8:27 ` [Qemu-devel] [PATCH 12/16] iotests: testcase for backing in parallels format Denis V. Lunev
2014-12-15 8:27 ` [Qemu-devel] [PATCH 13/16] block/parallels: read disk size from XML if DiskDescriptor.xml is passed Denis V. Lunev
2014-12-15 12:38 ` Kevin Wolf
2014-12-15 8:27 ` [Qemu-devel] [PATCH 14/16] block/parallels: introduce ParallelsSnapshot data structure Denis V. Lunev
2014-12-15 12:45 ` Kevin Wolf
2014-12-15 13:32 ` Denis V. Lunev
2014-12-17 16:15 ` [Qemu-devel] [RFC PATCH 1/1] block/parallels: new concept for DiskDescriptor.xml Denis V. Lunev
2014-12-15 8:28 ` [Qemu-devel] [PATCH 15/16] block/parallels: support read-only parallels snapshots Denis V. Lunev
2014-12-15 8:28 ` [Qemu-devel] [PATCH 16/16] iotests: testcase parallels image with snapshots Denis V. Lunev
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.