Re: [PATCH v1 12/16] qapi: fix example of blockdev-add command

[Victor Toso]

Hi,

On Wed, Aug 31, 2022 at 01:40:50PM +0200, Markus Armbruster wrote:
> Victor Toso <victortoso@redhat.com> writes:
>
> > The example output is setting optional member "backing" with null.
> > This has no runtime impact. Remove it.
> >
> > Problem was noticed when using the example as a test case for Go
> > bindings.
>
> "Fix example" and "problem" implies there's something wrong.
> "No runtime impact" sounds like it's redundant, but not wrong.
> Wrong or not wrong?

I take your comment is more about the wording which is confusing.

Would it be better if I change to:
'''
   The example output is setting optional member "backing" with
   null. While this has no runtime impact, setting optional
   members with empty value should not be encouraged. Remove it.
'''

While I think the above is true, my main reason for proposing
this change is to re-use the example as a test case, but I'm not
sure if adding anything related to it would make it better (only
more confusing!).

Cheers,
Victor

> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > ---
> >  qapi/block-core.json | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index dcc6d41494..302164d575 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -1542,9 +1542,7 @@
> >  #      "arguments": { "driver": "qcow2",
> >  #                     "node-name": "node1534",
> >  #                     "data-file": { "driver": "file",
> > -#                                    "filename": "hd1.qcow2" },
> > -#                     "backing": null } }
> > -#
> > +#                                    "filename": "hd1.qcow2" } } }
> >  # <- { "return": {} }
> >  #
> >  # -> { "execute": "blockdev-snapshot",
>

signature.asc
Description: PGP signature