Re: [libvirt RFC 2/2] qemu: Handle 'offset' and 'size' parameters of 'raw' driver

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.

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