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

[Markus Armbruster]

Victor Toso <victortoso@redhat.com> writes:

> Hi,
>
> On Wed, Aug 31, 2022 at 03:16:54PM +0200, Markus Armbruster wrote:
>> 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.
>
> Over the wire, how you get the difference between 1.c and 2? Are
> you saying that for optional member "backing" we should be
> explicit sending null over the wire?

In the QAPI schema language, absent optional members do not default to
any specific value.  Or in other words, "absent" is distinct from
"present with value V" for any value V.

Now, the *semantics* of "absent" are often identical to some default
value.  Documentation should then say something like (default:
DEFAULT-VALUE).

In this particular instance, it isn't: "absent" means something else
than any possible value.

Aside: no, I don't like this part of the QAPI schema language design
either.  "Absent defaults to DEFAULT-VALUE" is easier to explain and
understand.

Back to your question: to get 1.c., you pass a member "backing": null on
the wire, and to get 2., you pass no "backing" member.

>> 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.
>
> Yep, you are correct but not just with my patch. It is confusing
> that an optional member must be set to JSON Null over the wire..
> I'll need to think a bit more on this.
>
> Many many thanks for your review. Really appreciate it.

You're welcome!