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