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

[Victor Toso]

Hi,

On Thu, Sep 01, 2022 at 01:13:19PM +0200, Markus Armbruster wrote:
> 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.

This would be nice indeed.  I've filed an issue in Gitlab to
track it:

    https://gitlab.com/qemu-project/qemu/-/issues/1188

Cheers,
Victor

signature.asc
Description: PGP signature