| Message ID | 1be64a26c9a89ff0af4c2b1299d6c8b58361644a.1441890725.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
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 <berto@igalia.com> > --- > 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
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 <berto@igalia.com> > --- > 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 <eblake@redhat.com> 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.
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 <berto@igalia.com> > > --- > > 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
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.
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 <berto@igalia.com> >>> --- >>> 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
On Fri 11 Sep 2015 07:33:41 PM CEST, Max Reitz <mreitz@redhat.com> 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. Berto
Am 14.09.2015 um 07:54 hat Alberto Garcia geschrieben: > On Fri 11 Sep 2015 07:33:41 PM CEST, Max Reitz <mreitz@redhat.com> 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
On Mon 14 Sep 2015 10:45:55 AM CEST, Kevin Wolf wrote: >> >>> So why do we need the new flag? Because "backing: ''" is 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. >> > 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. But there was no way to open a BDS without having a BlockBackend attached, or was there? It's possible with Max's "BlockBackend and media" series, that's why I am using it. Berto
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
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 <berto@igalia.com> --- block.c | 5 +++++ qapi/block-core.json | 6 +++++- 2 files changed, 10 insertions(+), 1 deletion(-)