On Thu, Jan 16, 2020 at 05:56:40PM +0100, Peter Krempa wrote:

Prior to using -blockdev if a user created an overlay file with some json properties we'd not put them on the command line at all. This meant that qemu then obeyed them.

Now since we do configure all known backing files, we must also pass in all of the parameters. Otherwise we'd configure something else than the user expected.

The most prominent one now is the 'offset' and 'size' of the raw driver which we completely ignored.

This is an RFC to implement properly the parameters above to see whether upstream accepts it or not.

This patch is not meant to be pushed like this but to start discussion.

Another option is to detect when we know that the image can't be passed through (we do that partially for images with authentication) and possibly just skip formatting them and let qemu handle it. But that really seems like a step back now that we've put this much effort into blockdev.

Obviously it's missing the parser bits and schema and docs but I'd like to know whether the design of the XML makes any sense.

So you're suggesting <disk type="file" media="disk"> ... <source file='/tmp/testfle'> <options offset='10752' size='4063232'/> </source> ... </disk> First a very minor question is whether to have a general <options> element vs a more specific <slice offset="nnn" size="nnn"/>. I would probably have gone for the latter myself, so I feel like more general options elements end up being a dumping ground. Now the more important question... The offset/size applies to the 'raw' driver in QEMU, which IIUC, is something that can be plugged into the block layer driver stack at any point - either above or below a format driver, or a above a protocol protocol driver IIUC this lets us conceptually support many things 1. Guest -> raw + offet/set -> file 2. Guest -> qcow2 -> raw + offet/set -> file 3. Guest -> raw + offset/size -> qcow2 -> file 4. Guest -> raw + offset/size -> qcow2 -> raw + offset/set -> file Add in protocol drivers (iscsi) 5. Guest -> raw + offet/set -> iscsi 6. Guest -> qcow2 -> raw + offet/set -> iscsi 7. Guest -> raw + offset/size -> qcow2 -> iscsi 8. Guest -> raw + offset/size -> qcow2 -> raw + offset/set -> iscsi Add in backing files 9. Guest -> qcow2 -> backing raw + offset/size -> file 10. Guest -> qcow2 -> backing qcow2 -> raw + offset/size -> file 11. Guest -> raw + offset/size qcow2 -> backing raw + offset/size -> file 12. Guest -> raw + offset/size qcow2 -> backing qcow2 -> raw + offet/size -> file Some of these combinations look much more likely to be used than others. ie, i'd expect offset+size on top of the protocol driver to be much more common than offset+size on top of a format driver. So the question is do we need to cope with both of those possibilities, or can we restrict ourselves to only worry about offset+size on top of the prootocol driver ? With the syntax you're suggesting we can only cope with it on top of the protocol driver, or on top of the format driver, but not both. So we have pick one, or decide on a syntax that can cope with both. One way we can support both is to say that size/offset as a child of <source> refers to the protocol driver, and that size/offset at same level as <source> refers to the format driver. eg <disk type='file' device='disk'> <driver name='qemu' type='qcow2' queues='4'/> <options size="nnn" offset="nnn"/> <source file='/var/lib/libvirt/images/domain.qcow'> <options size="nnn" offset="nnn"/> </source> <backingStore type='file'> <options size="nnn" offset="nnn"/> <format type='qcow2'/> <source file='/var/lib/libvirt/images/snapshot.qcow'> <options size="nnn" offset="nnn"/> </source> <backingStore type='block'> <format type='raw'/> <options size="nnn" offset="nnn"/> <source dev='/dev/mapper/base'> <options size="nnn" offset="nnn"/> </source> <backingStore/> </backingStore> </backingStore> <target dev='vdd' bus='virtio'/> </disk> If we think this is a viable approach, then it has the nice characteristic that we can be lazy and only implement the protocol driver size/offset as a child of <source> for now. We'll know that we have the design flexiblility to add format driver size/offset in future, if anyone ever turns up and requests it.

The bug was reported by Rich Jones here: https://bugzilla.redhat.com/show_bug.cgi?id=1791788 --- src/conf/domain_conf.c | 19 +++++++++++++++++++ src/qemu/qemu_block.c | 10 ++++------ src/util/virstoragefile.c | 21 +++++++++++++++++---- src/util/virstoragefile.h | 4 ++++ tests/virstoragetest.c | 7 ++++++- 5 files changed, 50 insertions(+), 11 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2a8a04cacb..62872dfb2f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -24183,6 +24183,23 @@ virDomainDiskSourceFormatPrivateData(virBufferPtr buf, }

+static void +virDomainDiskSourceFormatOptions(virBufferPtr buf, + virStorageSourcePtr src) +{ + g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; + + if (src->format == VIR_STORAGE_FILE_RAW) { + if (src->offset > 0) + virBufferAsprintf(&attrBuf, " offset='%llu'", src->offset); + if (src->size > 0) + virBufferAsprintf(&attrBuf, " size='%llu'", src->size); + } + + virXMLFormatElement(buf, "options", &attrBuf, NULL); +} + + /** * virDomainDiskSourceFormat: * @buf: output buffer @@ -24253,6 +24270,8 @@ virDomainDiskSourceFormat(virBufferPtr buf, return -1; }

+ virDomainDiskSourceFormatOptions(&childBuf, src); + if (src->type != VIR_STORAGE_TYPE_NETWORK) virDomainSourceDefFormatSeclabel(&childBuf, src->nseclabels, src->seclabels, flags); diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index eab21bc107..21977a319e 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1190,14 +1190,12 @@ qemuBlockStorageSourceGetFormatRawProps(virStorageSourcePtr src, secretalias = srcPriv->encinfo->s.aes.alias; }

- /* currently unhandled properties for the 'raw' driver: - * 'offset' - * 'size' - */ - if (virJSONValueObjectAdd(props, "s:driver", driver, - "S:key-secret", secretalias, NULL) < 0) + "S:key-secret", secretalias, + "P:offset", src->offset, + "P:size", src->size, + NULL) < 0) return -1;

return 0; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 1397f532fd..22a243a2eb 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2270,6 +2270,8 @@ virStorageSourceCopy(const virStorageSource *src, def->type = src->type; def->protocol = src->protocol; def->format = src->format; + def->offset = src->offset; + def->size = src->size; def->capacity = src->capacity; def->allocation = src->allocation; def->has_allocation = src->has_allocation; @@ -3511,10 +3513,21 @@ virStorageSourceParseBackingJSONRaw(virStorageSourcePtr src, virJSONValuePtr json, int opaque G_GNUC_UNUSED) { - /* There are no interesting attributes in raw driver. - * Treat it as pass-through. - */ - return virStorageSourceParseBackingJSONInternal(src, json); + int rc; + + if ((rc = virStorageSourceParseBackingJSONInternal(src, json)) < 0) + return rc; + + if ((virJSONValueObjectHasKey(json, "offset") && + virJSONValueObjectGetNumberUlong(json, "offset", &src->offset) < 0) || + (virJSONValueObjectHasKey(json, "size") && + virJSONValueObjectGetNumberUlong(json, "size", &src->size) < 0)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("malformed 'size' or 'offset' property of 'raw' driver")); + return -1; + } + + return rc; }

diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 39e50a989d..5a86f32e95 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -286,6 +286,10 @@ struct _virStorageSource { bool nocow; bool sparse;

+ /* properties of the raw driver. Unspecified if 0. */ + unsigned long long offset; + unsigned long long size; + virStoragePermsPtr perms; virStorageTimestampsPtr timestamps; unsigned long long capacity; /* in bytes, 0 if unknown */ diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 58e15c0cc4..d9af2628d0 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -606,6 +606,9 @@ testBackingParse(const void *args) if (!src) return 0;

+ /* allow tests pass format */ + src->format = VIR_STORAGE_FILE_RAW; + if (src && !data->expect) { fprintf(stderr, "parsing of backing store string '%s' should " "have failed\n", data->backing); @@ -1567,7 +1570,9 @@ mymain(void) "}" "}" "}", - "<source file='/tmp/testfle'/>\n", 0); + "<source file='/tmp/testfle'>\n" + " <options offset='10752' size='4063232'/>\n" + "</source>\n", 0);

#endif /* WITH_YAJL */

-- 2.24.1

Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|