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

[Victor Toso <victortoso@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 7086 bytes --]

Hi,

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

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.

--

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.

    ##
    # @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.

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

> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]