[libvirt] [PATCH 0/2] Add support for 'raw' block driver to Qemu JSON backend parsing code
The raw driver is not very interesting on its own but it can be layered on top of other block drivers. New parameters that allow using only part of the underlying file/device were recently added to Qemu. This is useful e.g. to directly access partitions inside a disk image or disks inside an archive (like OVA). This feature is utilised in OVA import in virt-v2v tool where we access the disks directly in OVA without needing to unpack the tar archive first. After implementing this in virt-v2v we noticed that it does not work when libguestfs uses libvirt as a backend because libvirt fails with error: internal error: missing parser implementation for JSON backing volume driver 'raw' Tomáš Golembiovský (2): util: storage: split function for JSON backing volume parsing in two util: storage: add JSON backing volume parser 'raw' block driver src/util/virstoragefile.c | 51 +++++++++++++++++++++++++++++++++++++---------- tests/virstoragetest.c | 6 ++++++ 2 files changed, 47 insertions(+), 10 deletions(-) -- 2.11.1
Split virStorageSourceParseBackingJSON into two functions so that the core can be reused by other functions. The new function called virStorageSourceParseBackingJSONInternal accepts virJSONValuePtr. Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> --- src/util/virstoragefile.c | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 3d4bda700..3698eeeda 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -3053,29 +3053,28 @@ virStorageSourceParseBackingJSONDeflatten(virJSONValuePtr json) static int -virStorageSourceParseBackingJSON(virStorageSourcePtr src, - const char *json) +virStorageSourceParseBackingJSONInternal(virStorageSourcePtr src, + virJSONValuePtr json) { - virJSONValuePtr root = NULL; virJSONValuePtr fixedroot = NULL; virJSONValuePtr file; const char *drvname; size_t i; int ret = -1; - if (!(root = virJSONValueFromString(json))) - return -1; - - if (!(file = virJSONValueObjectGetObject(root, "file"))) { - if (!(fixedroot = virStorageSourceParseBackingJSONDeflatten(root))) + if (!(file = virJSONValueObjectGetObject(json, "file"))) { + if (!(fixedroot = virStorageSourceParseBackingJSONDeflatten(json))) goto cleanup; file = fixedroot; } if (!(drvname = virJSONValueObjectGetString(file, "driver"))) { + char *str = virJSONValueToString(json, false); virReportError(VIR_ERR_INVALID_ARG, _("JSON backing volume defintion " - "'%s' lacks driver name"), json); + "'%s' lacks driver name"), + NULLSTR(str)); + VIR_FREE(str); goto cleanup; } @@ -3091,12 +3090,28 @@ virStorageSourceParseBackingJSON(virStorageSourcePtr src, "driver '%s'"), drvname); cleanup: - virJSONValueFree(root); virJSONValueFree(fixedroot); return ret; } +static int +virStorageSourceParseBackingJSON(virStorageSourcePtr src, + const char *json) +{ + virJSONValuePtr root = NULL; + int ret = -1; + + if (!(root = virJSONValueFromString(json))) + return -1; + + ret = virStorageSourceParseBackingJSONInternal(src, root); + + virJSONValueFree(root); + return ret; +} + + virStorageSourcePtr virStorageSourceNewFromBackingAbsolute(const char *path) { -- 2.11.1
On Mon, Feb 13, 2017 at 23:53:42 +0100, Tomáš Golembiovský wrote:
Split virStorageSourceParseBackingJSON into two functions so that the core can be reused by other functions. The new function called virStorageSourceParseBackingJSONInternal accepts virJSONValuePtr.
Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> --- src/util/virstoragefile.c | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 3d4bda700..3698eeeda 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -3053,29 +3053,28 @@ virStorageSourceParseBackingJSONDeflatten(virJSONValuePtr json)
static int -virStorageSourceParseBackingJSON(virStorageSourcePtr src, - const char *json) +virStorageSourceParseBackingJSONInternal(virStorageSourcePtr src, + virJSONValuePtr json) { - virJSONValuePtr root = NULL; virJSONValuePtr fixedroot = NULL; virJSONValuePtr file; const char *drvname; size_t i; int ret = -1;
- if (!(root = virJSONValueFromString(json))) - return -1; - - if (!(file = virJSONValueObjectGetObject(root, "file"))) { - if (!(fixedroot = virStorageSourceParseBackingJSONDeflatten(root))) + if (!(file = virJSONValueObjectGetObject(json, "file"))) { + if (!(fixedroot = virStorageSourceParseBackingJSONDeflatten(json))) goto cleanup;
file = fixedroot; }
if (!(drvname = virJSONValueObjectGetString(file, "driver"))) { + char *str = virJSONValueToString(json, false);
We usually declare variables at the top.
virReportError(VIR_ERR_INVALID_ARG, _("JSON backing volume defintion " - "'%s' lacks driver name"), json); + "'%s' lacks driver name"), + NULLSTR(str));
Broken formatting.
+ VIR_FREE(str); goto cleanup; }
@@ -3091,12 +3090,28 @@ virStorageSourceParseBackingJSON(virStorageSourcePtr src, "driver '%s'"), drvname);
cleanup: - virJSONValueFree(root); virJSONValueFree(fixedroot); return ret; }
+static int +virStorageSourceParseBackingJSON(virStorageSourcePtr src, + const char *json) +{ + virJSONValuePtr root = NULL; + int ret = -1; + + if (!(root = virJSONValueFromString(json))) + return -1; + + ret = virStorageSourceParseBackingJSONInternal(src, root); + + virJSONValueFree(root); + return ret; +} + + virStorageSourcePtr virStorageSourceNewFromBackingAbsolute(const char *path) {
ACK, I'll fix the stuff pointed out prior to pushing.
The 'raw' block driver in Qemu is not directly interesting from libvirt's perspective, but it can be layered above some other block drivers and this may be interesting for the user. The patch adds support for the 'raw' block driver. The driver is treated simply as a pass-through and child driver in JSON is queried to get the necessary information. Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> --- src/util/virstoragefile.c | 16 ++++++++++++++++ tests/virstoragetest.c | 6 ++++++ 2 files changed, 22 insertions(+) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 3698eeeda..0447016bf 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2648,6 +2648,11 @@ virStorageSourceParseBackingColon(virStorageSourcePtr src, static int +virStorageSourceParseBackingJSONInternal(virStorageSourcePtr src, + virJSONValuePtr json); + + +static int virStorageSourceParseBackingJSONPath(virStorageSourcePtr src, virJSONValuePtr json, int type) @@ -2963,6 +2968,16 @@ virStorageSourceParseBackingJSONRBD(virStorageSourcePtr src, return -1; } +static int +virStorageSourceParseBackingJSONRaw(virStorageSourcePtr src, + virJSONValuePtr json, + int opaque ATTRIBUTE_UNUSED) +{ + /* There are no interesting attributes in raw driver. + * Treat it as pass-through. + */ + return virStorageSourceParseBackingJSONInternal(src, json); +} struct virStorageSourceJSONDriverParser { const char *drvname; @@ -2985,6 +3000,7 @@ static const struct virStorageSourceJSONDriverParser jsonParsers[] = { {"sheepdog", virStorageSourceParseBackingJSONSheepdog, 0}, {"ssh", virStorageSourceParseBackingJSONSSH, 0}, {"rbd", virStorageSourceParseBackingJSONRBD, 0}, + {"raw", virStorageSourceParseBackingJSONRaw, 0}, }; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index f766df115..1bf490c43 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -1492,6 +1492,12 @@ mymain(void) "<source protocol='rbd' name='testshare'>\n" " <host name='example.com'/>\n" "</source>\n"); + TEST_BACKING_PARSE("json:{ \"file\": { " + "\"driver\": \"raw\"," + "\"file\": {" + "\"driver\": \"file\"," + "\"filename\": \"/path/to/file\" } } }", + "<source file='/path/to/file'/>\n"); cleanup: /* Final cleanup */ -- 2.11.1
The patches compile. I looked at both commits and they at least superficially seem sensible. I'm not intimately familiar enough with the original code to review this fully. However I want to try to test this using libguestfs. I believe the following test case should be sufficient: $ cd /var/tmp $ truncate -s 1M backing.img $ qemu-img create \ -b 'json:{"driver":"raw", "file":{"filename":"/var/tmp/backing.img"}}' \ -f qcow2 overlay.qcow2 $ guestfish -a /var/tmp/overlay.qcow2 run libguestfs: error: could not create appliance through libvirt. Try running qemu directly without libvirt using this environment variable: export LIBGUESTFS_BACKEND=direct Original error from libvirt: invalid argument: JSON backing volume defintion '{"driver":"raw", "file":{"filename":"/var/tmp/backing.img"}}' lacks driver name [code=8 int1=-1] But with libvirt built with your patches: $ killall libvirtd $ ../libvirt/run guestfish -a /var/tmp/overlay.qcow2 run libguestfs: error: could not create appliance through libvirt. Try running qemu directly without libvirt using this environment variable: export LIBGUESTFS_BACKEND=direct Original error from libvirt: invalid argument: JSON backing volume defintion '{"driver":"raw","file":{"filename":"/var/tmp/backing.img"}}' lacks driver name [code=8 int1=-1] It could be that my test case is wrong in some way. I enabled debugging and it does appear to be using the new version of libvirt, so I'm not sure what's up ... Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
Hi, On Tue, 14 Feb 2017 15:58:45 +0000 "Richard W.M. Jones" <rjones@redhat.com> wrote:
The patches compile.
I looked at both commits and they at least superficially seem sensible. I'm not intimately familiar enough with the original code to review this fully.
However I want to try to test this using libguestfs. I believe the following test case should be sufficient:
$ cd /var/tmp $ truncate -s 1M backing.img $ qemu-img create \ -b 'json:{"driver":"raw", "file":{"filename":"/var/tmp/backing.img"}}' \
The problem lies in the JSON here. Libvirt lacks the driver probing mechanism QEMU has (which makes sense). That means one has to be explicit about the drivers. Try with the following backing definition: json:{"driver":"raw", "file":{ "driver":"file", "filename":"/var/tmp/backing.img"}} Tomas
-f qcow2 overlay.qcow2 $ guestfish -a /var/tmp/overlay.qcow2 run libguestfs: error: could not create appliance through libvirt.
Try running qemu directly without libvirt using this environment variable: export LIBGUESTFS_BACKEND=direct
Original error from libvirt: invalid argument: JSON backing volume defintion '{"driver":"raw", "file":{"filename":"/var/tmp/backing.img"}}' lacks driver name [code=8 int1=-1]
But with libvirt built with your patches:
$ killall libvirtd $ ../libvirt/run guestfish -a /var/tmp/overlay.qcow2 run libguestfs: error: could not create appliance through libvirt.
Try running qemu directly without libvirt using this environment variable: export LIBGUESTFS_BACKEND=direct
Original error from libvirt: invalid argument: JSON backing volume defintion '{"driver":"raw","file":{"filename":"/var/tmp/backing.img"}}' lacks driver name [code=8 int1=-1]
It could be that my test case is wrong in some way. I enabled debugging and it does appear to be using the new version of libvirt, so I'm not sure what's up ...
Rich.
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
-- Tomáš Golembiovský <tgolembi@redhat.com>
On Tue, Feb 14, 2017 at 10:03:54PM +0100, Tomáš Golembiovský wrote:
Hi,
On Tue, 14 Feb 2017 15:58:45 +0000 "Richard W.M. Jones" <rjones@redhat.com> wrote:
The patches compile.
I looked at both commits and they at least superficially seem sensible. I'm not intimately familiar enough with the original code to review this fully.
However I want to try to test this using libguestfs. I believe the following test case should be sufficient:
$ cd /var/tmp $ truncate -s 1M backing.img $ qemu-img create \ -b 'json:{"driver":"raw", "file":{"filename":"/var/tmp/backing.img"}}' \
The problem lies in the JSON here. Libvirt lacks the driver probing mechanism QEMU has (which makes sense). That means one has to be explicit about the drivers. Try with the following backing definition:
json:{"driver":"raw", "file":{ "driver":"file", "filename":"/var/tmp/backing.img"}}
OK, that works. However it also works with the unpatched version of libvirt, so it's not proof that these patches fix any problem. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
On Wed, 15 Feb 2017 10:55:24 +0000 "Richard W.M. Jones" <rjones@redhat.com> wrote:
On Tue, Feb 14, 2017 at 10:03:54PM +0100, Tomáš Golembiovský wrote:
Hi,
On Tue, 14 Feb 2017 15:58:45 +0000 "Richard W.M. Jones" <rjones@redhat.com> wrote:
The patches compile.
I looked at both commits and they at least superficially seem sensible. I'm not intimately familiar enough with the original code to review this fully.
However I want to try to test this using libguestfs. I believe the following test case should be sufficient:
$ cd /var/tmp $ truncate -s 1M backing.img $ qemu-img create \ -b 'json:{"driver":"raw", "file":{"filename":"/var/tmp/backing.img"}}' \
The problem lies in the JSON here. Libvirt lacks the driver probing mechanism QEMU has (which makes sense). That means one has to be explicit about the drivers. Try with the following backing definition:
json:{"driver":"raw", "file":{ "driver":"file", "filename":"/var/tmp/backing.img"}}
OK, that works. However it also works with the unpatched version of libvirt, so it's not proof that these patches fix any problem.
Ah, sorry. I didn't notice your JSON was bad from the start and I just blindly extended it. The correct JSON should look like this: json: { "file": { "driver":"raw", "file": { "driver":"file", "filename":"/var/tmp/backing.img" } } } Tomas
Rich.
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
-- Tomáš Golembiovský <tgolembi@redhat.com>
On Wed, Feb 15, 2017 at 12:53:39PM +0100, Tomáš Golembiovský wrote:
On Wed, 15 Feb 2017 10:55:24 +0000 "Richard W.M. Jones" <rjones@redhat.com> wrote:
On Tue, Feb 14, 2017 at 10:03:54PM +0100, Tomáš Golembiovský wrote:
Hi,
On Tue, 14 Feb 2017 15:58:45 +0000 "Richard W.M. Jones" <rjones@redhat.com> wrote:
The patches compile.
I looked at both commits and they at least superficially seem sensible. I'm not intimately familiar enough with the original code to review this fully.
However I want to try to test this using libguestfs. I believe the following test case should be sufficient:
$ cd /var/tmp $ truncate -s 1M backing.img $ qemu-img create \ -b 'json:{"driver":"raw", "file":{"filename":"/var/tmp/backing.img"}}' \
The problem lies in the JSON here. Libvirt lacks the driver probing mechanism QEMU has (which makes sense). That means one has to be explicit about the drivers. Try with the following backing definition:
json:{"driver":"raw", "file":{ "driver":"file", "filename":"/var/tmp/backing.img"}}
OK, that works. However it also works with the unpatched version of libvirt, so it's not proof that these patches fix any problem.
Ah, sorry. I didn't notice your JSON was bad from the start and I just blindly extended it. The correct JSON should look like this:
json: { "file": { "driver":"raw", "file": { "driver":"file", "filename":"/var/tmp/backing.img" } } }
Ok that does now demonstrate the fix. For reference, here is the full test: $ cd /var/tmp $ rm -f backing.img $ truncate -s 10M backing.img $ qemu-img create -b 'json:{"file":{"driver":"raw", "file":{"driver":"file", "filename":"/var/tmp/backing.img"}}}' -f qcow2 overlay.qcow2 With old libvirt: $ guestfish -a /var/tmp/overlay.qcow2 run libguestfs: error: could not create appliance through libvirt. Try running qemu directly without libvirt using this environment variable: export LIBGUESTFS_BACKEND=direct Original error from libvirt: internal error: missing parser implementation for JSON backing volume driver 'raw' [code=1 int1=-1] With patched libvirt: $ killall libvirtd $ ~/d/libvirt/run guestfish -a /var/tmp/overlay.qcow2 run [no errors printed] Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On Mon, Feb 13, 2017 at 23:53:43 +0100, Tomáš Golembiovský wrote:
The 'raw' block driver in Qemu is not directly interesting from libvirt's perspective, but it can be layered above some other block drivers and this may be interesting for the user.
The patch adds support for the 'raw' block driver. The driver is treated simply as a pass-through and child driver in JSON is queried to get the necessary information.
Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> --- src/util/virstoragefile.c | 16 ++++++++++++++++ tests/virstoragetest.c | 6 ++++++ 2 files changed, 22 insertions(+)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 3698eeeda..0447016bf 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2648,6 +2648,11 @@ virStorageSourceParseBackingColon(virStorageSourcePtr src,
static int +virStorageSourceParseBackingJSONInternal(virStorageSourcePtr src, + virJSONValuePtr json);
We try to avoid forward declaration as much as possible. It's better to move the code.
+ + +static int virStorageSourceParseBackingJSONPath(virStorageSourcePtr src, virJSONValuePtr json, int type) @@ -2963,6 +2968,16 @@ virStorageSourceParseBackingJSONRBD(virStorageSourcePtr src, return -1; }
+static int +virStorageSourceParseBackingJSONRaw(virStorageSourcePtr src, + virJSONValuePtr json, + int opaque ATTRIBUTE_UNUSED) +{ + /* There are no interesting attributes in raw driver. + * Treat it as pass-through. + */
In fact, we may need to start supporting other format drivers explicitly as well in the future. It may be necessary to add format validation and other stuff, but this should be okay for now. ACK, but I'll tweak the coding style a bit prior to pushing. Peter
On Wed, Feb 22, 2017 at 10:34:12 +0100, Peter Krempa wrote:
On Mon, Feb 13, 2017 at 23:53:43 +0100, Tomáš Golembiovský wrote:
The 'raw' block driver in Qemu is not directly interesting from libvirt's perspective, but it can be layered above some other block drivers and this may be interesting for the user.
The patch adds support for the 'raw' block driver. The driver is treated simply as a pass-through and child driver in JSON is queried to get the necessary information.
Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> --- src/util/virstoragefile.c | 16 ++++++++++++++++ tests/virstoragetest.c | 6 ++++++ 2 files changed, 22 insertions(+)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 3698eeeda..0447016bf 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2648,6 +2648,11 @@ virStorageSourceParseBackingColon(virStorageSourcePtr src,
static int +virStorageSourceParseBackingJSONInternal(virStorageSourcePtr src, + virJSONValuePtr json);
We try to avoid forward declaration as much as possible. It's better to move the code.
And this one can't be avoided ... :)
Hi, gentle reminder. Thanks, Tomas On Mon, 13 Feb 2017 23:53:41 +0100 Tomáš Golembiovský <tgolembi@redhat.com> wrote:
The raw driver is not very interesting on its own but it can be layered on top of other block drivers. New parameters that allow using only part of the underlying file/device were recently added to Qemu. This is useful e.g. to directly access partitions inside a disk image or disks inside an archive (like OVA).
This feature is utilised in OVA import in virt-v2v tool where we access the disks directly in OVA without needing to unpack the tar archive first. After implementing this in virt-v2v we noticed that it does not work when libguestfs uses libvirt as a backend because libvirt fails with error:
internal error: missing parser implementation for JSON backing volume driver 'raw'
Tomáš Golembiovský (2): util: storage: split function for JSON backing volume parsing in two util: storage: add JSON backing volume parser 'raw' block driver
src/util/virstoragefile.c | 51 +++++++++++++++++++++++++++++++++++++---------- tests/virstoragetest.c | 6 ++++++ 2 files changed, 47 insertions(+), 10 deletions(-)
-- 2.11.1
-- Tomáš Golembiovský <tgolembi@redhat.com>
participants (3)
-
Peter Krempa -
Richard W.M. Jones -
Tomáš Golembiovský