Re: [Qemu-block] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat

2015-09-14 Thread Kevin Wolf 
Am 14.09.2015 um 07:54 hat Alberto Garcia geschrieben:
> On Fri 11 Sep 2015 07:33:41 PM CEST, Max Reitz  wrote:
> 
> >>> So why do we need the new flag? Because "backing: ''" is ugly?
> >> 
> >> I guess it's just because you're the only one who actually reads the
> >> documentation. When discussing this, I didn't remember that we
> >> already had a way to express this (an additional bool wouldn't have
> >> been my favourite solution anyway). Thanks for catching this.
> >
> > I read the patch, it was part of the context. ;-)
> 
> Oh, that was embarrassing :-) Yes, it was the discussion from two weeks
> ago about passing empty strings as BlockdevRef that made me think that
> this would be ugly.
> 
> Anyway, was this ever implemented? It seems that passing a string to the
> 'backing' parameter is only specified in the JSON schema, but no one
> actually uses that.
> 
> So I'll implement that for the next version of my series.

I have a patch that actually allows passing a node-name reference as a
string here. v1 was posted a few months ago; it just turned out that I
need to kill bdrv_swap() before that can work because bdrv_swap()
doesn't work with nodes that have a BlockBackend attached.

Of course, I didn't check what an empty string would do with my patch...

Kevin

Re: [Qemu-block] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat

2015-09-11 Thread Max Reitz 
On 10.09.2015 15:39, Alberto Garcia wrote:
> If set to true, the image will be opened with the BDRV_O_NO_BACKING
> flag. This is useful for creating snapshots using images opened with
> blockdev-add, since they are not supposed to have a backing image
> before the operation.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  block.c  | 5 +
>  qapi/block-core.json | 6 +-
>  2 files changed, 10 insertions(+), 1 deletion(-)

Ignorant of any possible previous discussion that might have taken
place: The documentation for @backing says it may be set to the empty
string in order to achieve exactly that.

So why do we need the new flag? Because "backing: ''" is ugly?

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat

2015-09-11 Thread Eric Blake 
On 09/10/2015 07:39 AM, Alberto Garcia wrote:
> If set to true, the image will be opened with the BDRV_O_NO_BACKING
> flag. This is useful for creating snapshots using images opened with
> blockdev-add, since they are not supposed to have a backing image
> before the operation.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  block.c  | 5 +
>  qapi/block-core.json | 6 +-
>  2 files changed, 10 insertions(+), 1 deletion(-)

> 
> diff --git a/block.c b/block.c
> index 22d3b0e..4be32fb 100644
> --- a/block.c
> +++ b/block.c
> @@ -1469,6 +1469,11 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
> const char *filename,
>  
>  assert(drvname || !(flags & BDRV_O_PROTOCOL));
>  
> +if (qdict_get_try_bool(options, "ignore-backing", false)) {
> +flags |= BDRV_O_NO_BACKING;
> +}
> +qdict_del(options, "ignore-backing");

What happens if the user specified "ignore-backing":true, "backing":...?
 Should that be a hard error?

>  { 'struct': 'BlockdevOptionsGenericCOWFormat',
>'base': 'BlockdevOptionsGenericFormat',
> -  'data': { '*backing': 'BlockdevRef' } }
> +  'data': { '*backing': 'BlockdevRef',
> +'*ignore-backing': 'bool' } }

Depending on whether the answer to my question is that we already behave
sanely and don't leave a BlockdevRef dangling if the caller mixes the
two approaches, then:
Reviewed-by: Eric Blake 

But design-wise, would it make sense to support:

"backing":null

as an explicit request to not open a backing file?  Right now, qapi does
not have a way to express 'null' as part of an alternate type; but if it
did, BlockdevRef would merely add 'null' as one of its allowed
alternates.  Then we wouldn't need ignore-backing from the QMP
perspective. But I'm still not sure how it would map to the command line
perspective.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat

2015-09-11 Thread Max Reitz 
On 11.09.2015 19:28, Kevin Wolf wrote:
> Am 11.09.2015 um 19:21 hat Max Reitz geschrieben:
>> On 10.09.2015 15:39, Alberto Garcia wrote:
>>> If set to true, the image will be opened with the BDRV_O_NO_BACKING
>>> flag. This is useful for creating snapshots using images opened with
>>> blockdev-add, since they are not supposed to have a backing image
>>> before the operation.
>>>
>>> Signed-off-by: Alberto Garcia 
>>> ---
>>>  block.c  | 5 +
>>>  qapi/block-core.json | 6 +-
>>>  2 files changed, 10 insertions(+), 1 deletion(-)
>>
>> Ignorant of any possible previous discussion that might have taken
>> place: The documentation for @backing says it may be set to the empty
>> string in order to achieve exactly that.
>>
>> So why do we need the new flag? Because "backing: ''" is ugly?
> 
> I guess it's just because you're the only one who actually reads the
> documentation. When discussing this, I didn't remember that we already
> had a way to express this (an additional bool wouldn't have been my
> favourite solution anyway). Thanks for catching this.

I read the patch, it was part of the context. ;-)

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat

2015-09-11 Thread Kevin Wolf 
Am 11.09.2015 um 19:21 hat Max Reitz geschrieben:
> On 10.09.2015 15:39, Alberto Garcia wrote:
> > If set to true, the image will be opened with the BDRV_O_NO_BACKING
> > flag. This is useful for creating snapshots using images opened with
> > blockdev-add, since they are not supposed to have a backing image
> > before the operation.
> > 
> > Signed-off-by: Alberto Garcia 
> > ---
> >  block.c  | 5 +
> >  qapi/block-core.json | 6 +-
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> Ignorant of any possible previous discussion that might have taken
> place: The documentation for @backing says it may be set to the empty
> string in order to achieve exactly that.
> 
> So why do we need the new flag? Because "backing: ''" is ugly?

I guess it's just because you're the only one who actually reads the
documentation. When discussing this, I didn't remember that we already
had a way to express this (an additional bool wouldn't have been my
favourite solution anyway). Thanks for catching this.

Kevin


pgp2d4z1gM5b_.pgp
Description: PGP signature

Re: [Qemu-block] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat

2015-09-11 Thread Eric Blake 
On 09/11/2015 11:28 AM, Eric Blake wrote:

> But design-wise, would it make sense to support:
> 
> "backing":null

Just read Max's response; it sounds like we already have "backing":""
(and don't need "backing":null) for what we want. So maybe we don't need
this patch after all.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat

2015-09-10 Thread Alberto Garcia 
If set to true, the image will be opened with the BDRV_O_NO_BACKING
flag. This is useful for creating snapshots using images opened with
blockdev-add, since they are not supposed to have a backing image
before the operation.

Signed-off-by: Alberto Garcia 
---
 block.c  | 5 +
 qapi/block-core.json | 6 +-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 22d3b0e..4be32fb 100644
--- a/block.c
+++ b/block.c
@@ -1469,6 +1469,11 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
const char *filename,
 
 assert(drvname || !(flags & BDRV_O_PROTOCOL));
 
+if (qdict_get_try_bool(options, "ignore-backing", false)) {
+flags |= BDRV_O_NO_BACKING;
+}
+qdict_del(options, "ignore-backing");
+
 bs->open_flags = flags;
 bs->options = options;
 options = qdict_clone_shallow(options);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index ec50f06..0f797d7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1498,11 +1498,15 @@
 #   allowed to pass an empty string here in order to disable the
 #   default backing file.
 #
+# @ignore-backing: #optional if true, no backing file will be
+#  opened. Defaults to false (Since 2.5)
+#
 # Since: 1.7
 ##
 { 'struct': 'BlockdevOptionsGenericCOWFormat',
   'base': 'BlockdevOptionsGenericFormat',
-  'data': { '*backing': 'BlockdevRef' } }
+  'data': { '*backing': 'BlockdevRef',
+'*ignore-backing': 'bool' } }
 
 ##
 # @Qcow2OverlapCheckMode
-- 
2.5.1