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

[Daniel P. Berrange]

On Tue, Jul 18, 2017 at 09:24:08PM +0200, Markus Armbruster wrote:
> "Dr. David Alan Gilbert" <address@hidden> writes:
> 
> > * Markus Armbruster (address@hidden) 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
> >
> > Is this really what's happening? IMHO the tls_creds and tls_hostname
> > behaviour isn't that "" resets to the default, it just is the default.
> > I don't think there's anything special cased for tls_creds and
> > tls_hostname in the existing code.    It's this patch that's
> > adding more special casing.
> >
> > (I'm not going to nack this, but I just don't get why it's such a big
> > deal)
> 
> It wouldn't call it a big deal.  In fact, I called it a "minor QMP
> interface design flaw".
> 
> The implementation encodes "no TLS credentials, do not use TLS" and "no
> host name, fall back to the one in the migration URI" as "".  Works
> because "" is neither a valid TLS credentials ID, nor a valid host name.
> 
> Until commit 4af245d (v2.9.0), the implementation used NULL / optional
> absent rather than "".  Cleaner, because it doesn't rely on "" being
> invalid.
> 
> The reason why commit 4af245d changed it to "" was that
> migrate-set-parameters can't do NULL / optional absent.  It can't,
> because it already uses optional absent for "do not change this
> parameter".
> 
> Replacing NULL by "" side-stepped this problem.  I dislike that because
> it's not general (only works as long as "" is not a valid value), and
> ugly.
> 
> Instead of side-stepping the problem, I proposed to tackle it: make JSON
> null mean NULL in migrate-set-parameters.  However, the freeze was
> literally the other day, we needed *some* solution, and only the one
> making "" special was ready.  So I let it pass.
> 
> Related: NULL used to be shown as "" in query-migrate-parameters.
> Commit de63ab6 (v2.8.0) fixed that oddity, but commit 4af245d (v2.9.0)
> regressed the fix, probably unintentionally.

>From libvirt POV we considered de63ab6 to be the regression (from the
original 2.6.0 behaviour), and 4af245d to be the fix for that regression.
Libvirt expected to see "" for tls-hostname in the output - without that
it doesn't know that tls-hostname is even a valida parameter.

> I'm open to fixing the query-migrate-parameters regression during
> freeze.

No please don't. That will break libvirt yet again.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|