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

[Markus Armbruster]

Victor Toso <victortoso@redhat.com> writes:

> Hi,
>
> First of all, I'm happy that this patch got us into this
> discussion.

Me too!

> On Wed, Aug 31, 2022 at 04:53:49PM +0200, Markus Armbruster wrote:
>> 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).
>
> Yep, this is fine.
>
>> In this particular instance, it isn't: "absent" means something else
>> than any possible value.
>
> The major painpoint for me is that, in Go an optional member is a
> field with a pointer to that field's type. A pointer is default
> initialized with nil and if the user of the Go module does
> nothing with it, we naturally omit it in the output JSON.
>
> This needs to be workaround in two cases so far:
> BlockdevRefOrNull and StrOrNull. This two alternate types are the
> only ones that take JSON null as value. I'm sure I'll make it
> work.

Losely related:

commit 481b002cc81ed7fc7b06e32e9d4d495d81739d14
Author: Markus Armbruster <armbru@redhat.com>
Date:   Wed Apr 29 15:35:05 2015 -0600

    qobject: Add a special null QObject
    
    I'm going to fix the JSON parser to recognize null.  The obvious
    representation of JSON null as (QObject *)NULL doesn't work, because
    the parser already uses it as an error value.  Perhaps we should
    change it to free NULL for null, but that's more than I can do right
    now.  Create a special null QObject instead.
    
    The existing QDict, QList, and QString all represent something that
    is a pointer in C and could therefore be associated with NULL.  But
    right now, all three of these sub-types are always non-null once
    created, so the new null sentinel object is intentionally unrelated
    to them.
    
    Signed-off-by: Markus Armbruster <armbru@redhat.com>
    Signed-off-by: Eric Blake <eblake@redhat.com>
    Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

I figure something like this should work for Go as well, i.e. create a
singleton type to represent the QAPI type 'null'.

> --
>
> Now, should we really keep using null type as alternative way of
> expressing "disabling feature" or even "use something else"?
>
> I'd be happy to work on improving this if that's reasonable. My
> 2c bellow.
>
>     ##
>     # @BlockdevRefOrNull:
>     #
>     # Reference to a block device.
>     #
>     # @definition: defines a new block device inline
>     # @reference: references the ID of an existing block device.
>     #             An empty string means that no block device should
>     #             be referenced.  Deprecated; use null instead.
>     # @null: No block device should be referenced (since 2.10)
>     #
>     # Since: 2.9
>     ##
>     { 'alternate': 'BlockdevRefOrNull',
>       'data': { 'definition': 'BlockdevOptions',
>                 'reference': 'str',
>                 'null': 'null' } }
>
> BlockdevRefOrNull is only used by BlockdevOptionsGenericCOWFormat
> which is used by BlockdevOptions ('qed' and 'vmdk') and extend by
> BlockdevOptionsQcow and BlockdevOptionsQcow2.
>
> As you pointed out before, setting backing to null means
> disabling. This is expressed in both BlockdevRefOrNull and
> @BlockdevOptionsGenericCOWFormat documentation.
>
> IMHO the idea of disabling the default is fine but would be
> better expressed with a boolean type, something like:
>
>     { 'alternate': 'BlockdevRefOrNull',
>       'data': { 'definition': 'BlockdevOptions',
>                 'reference': 'str',
>                 'enable': 'bool' } }
>
> Which makes { backing: false } a bit nicer to my newbie eyes. If
> backing member is made non optional, { backing: true } should
> have the same value as omitting an optional backing member.

So:

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 false means use no backing image

1.d. that is true means use the backing file named in the COW
     image.

2. Absent @backing defaults to true, i.e. 1.d.

Looks fine to me as an interface, if I ignore compatibility concerns.

We could quibble about the name 'enable'.  It's not part of the external
interface.

Sadly, the idea runs afoul a QAPI restriction:

    scripts/qapi-gen.py: In file included from ../qapi/qapi-schema.json:68:
    In file included from ../qapi/block.json:8:
    ../qapi/block-core.json: In alternate 'BlockdevRefOrNull':
    ../qapi/block-core.json:4362: branch 'enabled' can't be distinguished from 
'reference'

This is due to

commit c0644771ebedbd8f47f3c24816445e30111d226b (tag: pull-qapi-2017-05-31)
Author: Markus Armbruster <armbru@redhat.com>
Date:   Mon May 22 18:42:15 2017 +0200

    qapi: Reject alternates that can't work with keyval_parse()
    
    Alternates are sum types like unions, but use the JSON type on the
    wire / QType in QObject instead of an explicit tag.  That's why we
    require alternate members to have distinct QTypes.
    
    The recently introduced keyval_parse() (commit d454dbe) can only
    produce string scalars.  The qobject_input_visitor_new_keyval() input
    visitor mostly hides the difference, so code using a QObject input
    visitor doesn't have to care whether its input was parsed from JSON or
    KEY=VALUE,...  The difference leaks for alternates, as noted in commit
    0ee9ae7: a non-string, non-enum scalar alternate value can't currently
    be expressed.
    
    In part, this is just our insufficiently sophisticated implementation.
    Consider alternate type 'GuestFileWhence'.  It has an integer member
    and a 'QGASeek' member.  The latter is an enumeration with values
    'set', 'cur', 'end'.  The meaning of b=set, b=cur, b=end, b=0, b=1 and
    so forth is perfectly obvious.  However, our current implementation
    falls apart at run time for b=0, b=1, and so forth.  Fixable, but not
    today; add a test case and a TODO comment.
    
    Now consider an alternate type with a string and an integer member.
    What's the meaning of a=42?  Is it the string "42" or the integer 42?
    Whichever meaning you pick makes the other inexpressible.  This isn't
    just an implementation problem, it's fundamental.  Our current
    implementation will pick string.
    
    So far, we haven't needed such alternates.  To make sure we stop and
    think before we add one that cannot sanely work with keyval_parse(),
    let's require alternate members to have sufficiently distinct
    representation in KEY=VALUE,... syntax:
    
    * A string member clashes with any other scalar member
    
    * An enumeration member clashes with bool members when it has value
      'on' or 'off'.
    
    * An enumeration member clashes with numeric members when it has a
      value that starts with '-', '+', or a decimal digit.  This is a
      rather lazy approximation of the actual number syntax accepted by
      the visitor.
    
      Note that enumeration values starting with '-' and '+' are rejected
      elsewhere already, but better safe than sorry.
    
    Signed-off-by: Markus Armbruster <armbru@redhat.com>
    Message-Id: <1495471335-23707-5-git-send-email-armbru@redhat.com>
    Reviewed-by: Eric Blake <eblake@redhat.com>
    Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Astute readers may now wonder why our string member does *not* clash
with scalar member null.  The answer is kind of embarrassing: because
the keyval input visitor supports numbers and booleans, but not null.
If you need null, you get to use JSON.

If we enhanced the visitor to support null like it supports booleans,
the value null would become ambiguous, and to remove this ambiguity,
we'd have to make 'str' clash with 'null'.

I've come to regret adding 'null' to the QAPI schema language.  I didn't
fully understand how this innocent-looking feature was going to interact
with everything else.  More trouble than it's worth.

The dotted keys syntax as an alternative to JSON is also plenty of
trouble, but we knew that, and accepted it because there's also worth.

>     ##
>     # @StrOrNull:
>     #
>     # This is a string value or the explicit lack of a string (null
>     # pointer in C).  Intended for cases when 'optional absent' already
>     # has a different meaning.
>     #
>     # @s: the string value
>     # @n: no string value
>     #
>     # Since: 2.10
>     ##
>     { 'alternate': 'StrOrNull',
>       'data': { 's': 'str',
>                 'n': 'null' } }
>
> StrOrNull is used in MigrateSetParameters (*tls-creds,
> *tls-hostname, *tls-authz):
>   JSON null: disable specifics or the entirety of migrating
>              with tls.
>   "" (empty string): Uses some specifics default.
>   omitted: Likely to error if using x509 tls?
>
>   Similarly, a boolean would make more sense to express disabled?
>
>     
> https://gitlab.com/qemu-project/qemu/-/commit/4af245dc3e6e5c96405b3edb9d75657504256469?view=parallel
>
> StrOrNull is also used in x-blockdev-set-iothread in the iothread
> member, documented as follow:
>
>    # @iothread: the name of the IOThread object or null for the
>    # main loop
>
> iothread here is non optional, meaning that the user has to set a
> string with the name of IOThread object or null. This really
> seems a case where iothread could be optional "str" instead of
> StrOrNull where omitting defaults to main loop.

Then "absent" is not the same as "present with value V" for any value V.
That's okay; it conforms to the schema language design.  I just don't
like that part of the schema language design.  To be clear: insufficient
reason to reject a patch.

We'd have to provide backward compatibility somehow.

>> 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.
>
> Well, there always room for improving.

Here's a concrete improvement I'd like to see some day: extend the QAPI
language to support optionals with a default value, i.e. absent is
indistinguishable from present with the default value.  Then use this
for as many optionals as we can.

>> 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.
>
> Cheers,
> Victor