Re: [Qemu-devel] [PATCH] migration: allow clearing migration string parameters

[Markus Armbruster]

"Dr. David Alan Gilbert" <address@hidden> writes:

> * Markus Armbruster (address@hidden) wrote:
>> "Daniel P. Berrange" <address@hidden> writes:
>> 
>> > On Wed, Mar 01, 2017 at 08:36:03AM -0600, Eric Blake wrote:
>> >> On 03/01/2017 06:32 AM, Daniel P. Berrange wrote:
>> >> >      }
>> >> >      if (params->has_tls_creds) {
>> >> >          g_free(s->parameters.tls_creds);
>> >> > -        s->parameters.tls_creds = g_strdup(params->tls_creds);
>> >> > +        if (*params->tls_creds == '\0') {
>> >> > +            s->parameters.tls_creds = NULL;
>> >> 
>> >> I'm wondering if you should also do s->parameters.has_tls_creds = false
>> >> at this point?  The visitors expect that if has_tls_creds is true, then
>> >> the string is non-NULL.
>> >
>> > The fact that s->parameters contains has_* fields is completely ignored
>> > by the migration code afaict. IOW the code behaves as if all the has_*
>> > fields are hardwired to true in s->parameters, even though that is not
>> > the case :-) The has_* fields are only used when the various migration
>> > QMP methods are executed, and those all use a separate MigrationParameters
>> > struct instance.
>> 
>> Not keeping the has_ members up-to-date is harmless as long as you don't
>> pass the thing to visitors, including the one hiding in qapi_free_FOO().
>> That one ignores scalars, though.
>> 
>> 
>> From a more abstract point of view, we have two related data types: one
>> for the state, and one for state changes requests.
>> 
>> In state, members are always present.
>> 
>> A state change request is a bag of state member change requests, and
>> each request can either specify the new value or ask for a reset to
>> default.  Absent member means no change.
>> 
>> We press the same QAPI type into service for both by making all members
>> optional.
>> 
>> For the state case, we hardwire the has_ to true.  Or even ignore them
>> completely.
>> 
>> For the state change request, we use has_ = false for "no change", has_
>> = true with a special value for "reset to default" (new in this patch)
>> and has_ = true with a non-special value for "set to this value".
>
> I'm confused why we need a 'reset to default' - all we need is the ability
> to change each parameter, and for the new value of that parameter
> to be an empty string.

You argue syntax, I'm arguing semantics.

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.

>> Requires a special value outside the set of non-special values.  The
>> obvious one is JSON null, but the QAPI generator doesn't quite support
>> that, yet.  "" works here, but is not general.
>> 
>> I think I can get you null support in 2.10.  Would that work for you?
>
> Dave
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK