Re: [Qemu-devel] [PATCH for-2.10 10/10] migration: Use JSON null instead of "" to reset parameter to default

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 07/18/2017 08:41 AM, Markus Armbruster wrote:
>> migrate-set-parameters sets migration parameters according to is
>> arguments like this:
>> 
>> * Present means "set the parameter to this value"
>> 
>> * Absent means "leave the parameter unchanged"
>> 
>> * Except for parameters tls_creds and tls_hostname, "" means "reset
>>   the parameter to its default value
>> 
>> The first two are perfectly normal: presence of the parameter makes
>> the command do something.
>> 
>> The third one overloads the parameter with a second meaning.  The
>> overloading is *implicit*, i.e. it's not visible in the types.  Works
>> here, because "" is neither a valid TLS credentials ID, nor a valid
>> host name.
>> 
>> Pressing argument values the schema accepts, but are semantically
>> invalid, into service to mean "reset to default" is not general, as
>> suitable invalid values need not exist.  I also find it ugly.
>> 
>> To clean this up, we could add a separate flag argument to ask for
>> "reset to default", or add a distinct value to @tls_creds and
>> @tls_hostname.  This commit implements the latter: add JSON null to
>> the values of @tls_creds and @tls_hostname, deprecate "".
>> 
>> Because we're so close to the 2.10 freeze, implement it in the
>> stupidest way possible: have qmp_migrate_set_parameters() rewrite null
>> to "" before anything else can see the null.  The proper way to do it
>> would be rewriting "" to null, but that requires fixing up code to
>> work with null.  Add TODO comments for that.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>> +++ b/qapi-schema.json
>> @@ -116,6 +116,13 @@
>>  { 'command': 'qmp_capabilities' }
>>  
>>  ##
>> +# @StrOrNull:
>
> A little light on the documentation.

Care to suggest improvements?  I figure the schema is obvious enough
without any, but the generated documentation could perhaps use some.

[...]