qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] qapi: Stop abusing "special" values for something entirely different
@ 2017-07-14 17:12 Markus Armbruster
  2017-07-14 17:38 ` Eric Blake
  2017-07-17  9:02 ` Daniel P. Berrange
  0 siblings, 2 replies; 4+ messages in thread
From: Markus Armbruster @ 2017-07-14 17:12 UTC (permalink / raw)
  To: qemu-devel; +Cc: Eric Blake, Daniel P. Berrange, Kevin Wolf, qemu-block

Back in March, we discussed Dan's "[PATCH] migration: allow clearing
migration string parameters".  The patch extends command
migrate-set-parameters to interpret empty string arguments to parameters
tls-creds and tls-hostname specially:

    # @tls-creds: ID of the 'tls-creds' object that provides credentials
    #             for establishing a TLS connection over the migration data
    #             channel. On the outgoing side of the migration, the credentials
    #             must be for a 'client' endpoint, while for the incoming side the
    #             credentials must be for a 'server' endpoint. Setting this
    #             will enable TLS for all migrations. The default is unset,
    #             resulting in unsecured migration at the QEMU level. (Since 2.7)
--> #             An empty string means that QEMU will use plain text mode for
--> #             migration, rather than TLS (Since 2.9)
    #
    # @tls-hostname: hostname of the target host for the migration. This
    #                is required when using x509 based TLS credentials and the
    #                migration URI does not already include a hostname. For
    #                example if using fd: or exec: based migration, the
    #                hostname must be provided so that the server's x509
    #                certificate identity can be validated. (Since 2.7)
--> #                An empty string means that QEMU will use the hostname
--> #                associated with the migration URI, if any. (Since 2.9)

Works, since "" is not a valid TLS credentials ID, and not a valid host
name.

I objected:

    The command means "set parameter P to value V".  *Except* when V is
    "", it means something else, namely "reset parameter P to its
    default, whatever that may be".

    This is (a) not general, because it won't do for cases where "" may
    occur as value, and (b) ugly.

    Ugliness is the eye of the beholder.  Lack of generality isn't.

https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg02841.html

I proposed to instead add JSON null to the set of accepted values, using
an alternate type.

However, Dan needed this for 2.9, adding a first class null type to QAPI
takes a bit of work (~50 lines, plus tests), and the freeze was
literally tomorrow, so I relented, and let this pass in the hope of
deprecating it in favor of the cleaner solution in 2.10.  It became
commit 4af245d.

There's a minor complication with my deprecation plan: the
migrate-set-parameters parameters in question are *also* results of
query-migrate-parameters, because both are defined by reference to type
MigrationParameters.  So, unless we decouple the two by duplicating the
type, changing the parameters of migrate-set-parameters also changes the
results of query-migrate-parameters.  However:

1. *Actual* results don't change (query-migrate-parameters doesn't
   suddenly return null values), only the introspection value changes.

2. This isn't actually a new issue.  The members of MigrationParameters
   are all optional, because query-migrate-parameters needs them to be
   optional.  But they're not actually optional in
   query-migrate-parameters.  Sharing types can make introspection less
   tight than it could be.


Another, similar case has crept in without me noticing: BlockdevRef
(commit d26c9a1, Sep 2013, but declared stable only in 2.9).

# @reference:       references the ID of an existing block device. An
#                   empty string means that no block device should be
#                   referenced.

Works, since "" is not a valid block ID.

BlockdevRef occurs only within arguments of blockdev-add.  blockdev-add
accepts "" in just one place: optional member @backing of COW formats.
Semantics:

* Present means "use this block device" as backing storage

* Absent means "default to the one stored in the image"

* Except "" means "don't use backing storage at all"

The first two are perfectly normal: when the parameter is absent, it
defaults to an implied value, but the value's meaning is the same.

The third one overloads the parameter with a second meaning.  The
overloading *implicit*, i.e. it's not visible in the types.

This is really the same problem as migrate-set-parameters:

* we need a way to say "do something else entirely",

* but we can't use "absent", since it's already taken,

* so we pick one the QAPI schema already allows, but that is not
  actually valid: "".

Instead of the last part, I prefer either

* so we add a *new* value, such as JSON null.

or perhaps

* so we add a second optional parameter to ask for "do something else
  entirely" (since the command can do only one of the two things, you
  may give only one of the two optional parameters).


Here's what I propose to do:

1. Stop abusing values the schema accepts, but are invalid to mean "do
something else entirely".

2. Add a first class null type to QAPI.

3. Turn MigrationParameters members tls-creds and tls-hostname into
alternate of str and null.  Deprecate "".

4. Add a null member to alternate BlockdefRef.  Deprecate "".

I got patches for 2., and I intend to work on 3. and 4.

Since this is "only" about "less than general and ugly", we may decide
to leave things as they are if my patches turn out even uglier.

Meanwhile, opinions?

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-07-18 15:42 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-14 17:12 [Qemu-devel] qapi: Stop abusing "special" values for something entirely different Markus Armbruster
2017-07-14 17:38 ` Eric Blake
2017-07-18 15:42   ` Markus Armbruster
2017-07-17  9:02 ` Daniel P. Berrange

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).