[v5,12/22] block: Allow recursive "file"s

It should be possible to use a format as a driver for a file which in
turn requires another file, i.e., nesting file formats.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> It should be possible to use a format as a driver for a file which in
> turn requires another file, i.e., nesting file formats.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Hm, does this do what I think it does?

$ ./qemu-img convert -O qcow2 /home/kwolf/images/hd.img /tmp/hd.qcow2
$ ./qemu-img convert -f raw -O qcow2 /tmp/hd.qcow2 /tmp/hd.qcow2.qcow2
$ x86_64-softmmu/qemu-system-x86_64 -drive driver=qcow2,file.driver=qcow2,file.file.driver=file,file.file.filename=/tmp/hd.qcow2.qcow2

I can't decide whether this is awesomeness or insanity, but in any case
it works with this patch. :-)

Worth a qemu-iotests case, I think.

>  block.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 9659eb5..9222669 100644
> --- a/block.c
> +++ b/block.c
> @@ -948,14 +948,19 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>          goto fail;
>      }
>  
> -    ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err);
> +    if (!drv->bdrv_file_open) {
> +        ret = bdrv_open(bs, filename, options, flags, drv, &local_err);
> +        options = NULL;
> +    } else {
> +        ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err);
> +    }
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          goto fail;
>      }
>  
>      /* Check if any unknown options were used */
> -    if (qdict_size(options) != 0) {
> +    if (options && (qdict_size(options) != 0)) {
>          const QDictEntry *entry = qdict_first(options);
>          error_setg(errp, "Block protocol '%s' doesn't support the option '%s'",
>                     drv->format_name, entry->key);
> @@ -970,10 +975,12 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>  
>  fail:
>      QDECREF(options);
> -    if (!bs->drv) {
> -        QDECREF(bs->options);
> +    if (bs) {
> +        if (!bs->drv) {
> +            QDECREF(bs->options);
> +        }
> +        bdrv_unref(bs);
>      }
> -    bdrv_unref(bs);
>      return ret;
>  }

Not sure why this hunk is needed, but anyway:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

On 12/13/2013 01:19 PM, Kevin Wolf wrote:
> Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
>> It should be possible to use a format as a driver for a file which in
>> turn requires another file, i.e., nesting file formats.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
> Hm, does this do what I think it does?

That depends on what you think it's doing :)

> 
> $ ./qemu-img convert -O qcow2 /home/kwolf/images/hd.img /tmp/hd.qcow2
> $ ./qemu-img convert -f raw -O qcow2 /tmp/hd.qcow2 /tmp/hd.qcow2.qcow2
> $ x86_64-softmmu/qemu-system-x86_64 -drive driver=qcow2,file.driver=qcow2,file.file.driver=file,file.file.filename=/tmp/hd.qcow2.qcow2
> 
> I can't decide whether this is awesomeness or insanity, but in any case
> it works with this patch. :-)

So if I understood your example, you just created a qcow2 file, where
the non-metadata contents of that file are themselves a qcow2 data format.

Oh my.  This could get nasty if someone ever wanted to try to get
libvirt to feed the nested contents to a guest (right now, libvirt has a
hard-coded assumption that qcow2 files only ever wrap raw data in the
non-metadata portion; there's no easy way to represent in the libvirt
storage volume XML a file where you want the nested guest contents of
qcow2 data wrapped inside the non-metadata portion of an outermost qcow2
file).

I guess it's not much different from a raw block device that has a
partition table, then in the first partition, you recursively add
another partition table (which is what you get when you hand one
partition of a host block device to the guest, and the guest then
partitions what it thinks is a full disk).  And libguestfs is able to
see through nested partition tables, so on that front, the functionality
is similar.

Of course, the recursion has its use for blkverify and blkdebug, but
maybe (for our sanity) it would be worth preventing nesting across any
format that has metadata wrapping contents.

> 
> Worth a qemu-iotests case, I think.

That's for sure :)

Am 13.12.2013 um 21:36 hat Eric Blake geschrieben:
> On 12/13/2013 01:19 PM, Kevin Wolf wrote:
> > Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> >> It should be possible to use a format as a driver for a file which in
> >> turn requires another file, i.e., nesting file formats.
> >>
> >> Signed-off-by: Max Reitz <mreitz@redhat.com>
> > 
> > Hm, does this do what I think it does?
> 
> That depends on what you think it's doing :)
> 
> > 
> > $ ./qemu-img convert -O qcow2 /home/kwolf/images/hd.img /tmp/hd.qcow2
> > $ ./qemu-img convert -f raw -O qcow2 /tmp/hd.qcow2 /tmp/hd.qcow2.qcow2
> > $ x86_64-softmmu/qemu-system-x86_64 -drive driver=qcow2,file.driver=qcow2,file.file.driver=file,file.file.filename=/tmp/hd.qcow2.qcow2
> > 
> > I can't decide whether this is awesomeness or insanity, but in any case
> > it works with this patch. :-)
> 
> So if I understood your example, you just created a qcow2 file, where
> the non-metadata contents of that file are themselves a qcow2 data format.
> 
> Oh my.

Hehe, that was exactly my first thought as well.

But if we're serious about the direction that we're taking with
blockdev-add - that is, letting the user build his graph of block
drivers - then this is something we have to allow. It's just a proof
that we do provide the flexibility we had in mind.

> This could get nasty if someone ever wanted to try to get
> libvirt to feed the nested contents to a guest (right now, libvirt has a
> hard-coded assumption that qcow2 files only ever wrap raw data in the
> non-metadata portion; there's no easy way to represent in the libvirt
> storage volume XML a file where you want the nested guest contents of
> qcow2 data wrapped inside the non-metadata portion of an outermost qcow2
> file).

I don't think there's a good use case for nested qcow2, so I wouldn't
expect libvirt to implement this. Until the first user comes up with a
use case, of course. And like with everything scary that we assume
nobody will ever do, someone will turn up eventually...

> I guess it's not much different from a raw block device that has a
> partition table, then in the first partition, you recursively add
> another partition table (which is what you get when you hand one
> partition of a host block device to the guest, and the guest then
> partitions what it thinks is a full disk).  And libguestfs is able to
> see through nested partition tables, so on that front, the functionality
> is similar.
> 
> Of course, the recursion has its use for blkverify and blkdebug, but
> maybe (for our sanity) it would be worth preventing nesting across any
> format that has metadata wrapping contents.

I don't think qemu should prevent it. The architecture we want to have
in the end doesn't really distinguish between formats and filters, there
are only drivers. And technically there is nothing that allowing this
breaks.

libvirt, on the other hand, can probably expose something simpler. But
as we discussed on KVM Forum, it's not unlikely that you'll have to have
your own blockdev project sooner or later because users will want to
have some of the flexibility - and then maybe nesting qcow2 falls out as
naturally as it does in qemu now.

Kevin

On 13.12.2013 21:19, Kevin Wolf wrote:
> Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
>> It should be possible to use a format as a driver for a file which in
>> turn requires another file, i.e., nesting file formats.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Hm, does this do what I think it does?
>
> $ ./qemu-img convert -O qcow2 /home/kwolf/images/hd.img /tmp/hd.qcow2
> $ ./qemu-img convert -f raw -O qcow2 /tmp/hd.qcow2 /tmp/hd.qcow2.qcow2
> $ x86_64-softmmu/qemu-system-x86_64 -drive driver=qcow2,file.driver=qcow2,file.file.driver=file,file.file.filename=/tmp/hd.qcow2.qcow2
>
> I can't decide whether this is awesomeness or insanity, but in any case
> it works with this patch. :-)
>
> Worth a qemu-iotests case, I think.
>
>>   block.c | 17 ++++++++++++-----
>>   1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 9659eb5..9222669 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -948,14 +948,19 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>>           goto fail;
>>       }
>>   
>> -    ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err);
>> +    if (!drv->bdrv_file_open) {
>> +        ret = bdrv_open(bs, filename, options, flags, drv, &local_err);
>> +        options = NULL;
>> +    } else {
>> +        ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err);
>> +    }
>>       if (ret < 0) {
>>           error_propagate(errp, local_err);
>>           goto fail;
>>       }
>>   
>>       /* Check if any unknown options were used */
>> -    if (qdict_size(options) != 0) {
>> +    if (options && (qdict_size(options) != 0)) {
>>           const QDictEntry *entry = qdict_first(options);
>>           error_setg(errp, "Block protocol '%s' doesn't support the option '%s'",
>>                      drv->format_name, entry->key);
>> @@ -970,10 +975,12 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>>   
>>   fail:
>>       QDECREF(options);
>> -    if (!bs->drv) {
>> -        QDECREF(bs->options);
>> +    if (bs) {
>> +        if (!bs->drv) {
>> +            QDECREF(bs->options);
>> +        }
>> +        bdrv_unref(bs);
>>       }
>> -    bdrv_unref(bs);
>>       return ret;
>>   }
> Not sure why this hunk is needed, but anyway:

Ah, I think I know. It is there because of patch 9. As stated in the 
cover letter, additionally to the double QDECREF on options, I noticed a 
possible NULL dereference of bs on error in the "if (reference)" path 
and fixed it in this version. In v3, it was addressed by this hunk, 
which somehow got into patch 12 rather than 9. I'll remove it, now that 
it's unnecessary.

Max