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

[Markus Armbruster <armbru@redhat.com>]

Cc: Kevin for an improved chance of getting any nonsense I might write
corrected.

Victor Toso <victortoso@redhat.com> writes:

> 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!).

I had a closer look at the schema.

The definition of backing is

    ##
    # @BlockdevOptionsGenericCOWFormat:
    #
    # Driver specific block device options for image format that have no option
    # besides their data source and an optional backing file.
    #
    # @backing: reference to or definition of the backing file block
    #           device, null disables the backing file entirely.
    #           Defaults to the backing file stored the image file.
    #
    # Since: 2.9
    ##
    { 'struct': 'BlockdevOptionsGenericCOWFormat',
      'base': 'BlockdevOptionsGenericFormat',
      'data': { '*backing': 'BlockdevRefOrNull' } }

Meaning, if I remember correctly (with some help from commit
c42e8742f52's message):

1. Present @backing

1.a. of type 'str' means use the existing block device with this ID as
     backing image

1.b. of type 'BlockdevOptions' means use the new block device defined by
     it as backing image

1.c. that is null means use no backing image

2. Absent @backing means default to the backing file named in the COW
   image.

Therefore, ...

>
> 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",
>> 

... your patch changes the example from 1.c. to 2.  Which is probably
not what you intended.