Re: [PATCH] monitor/hmp-cmds: fix bad indentation in 'info migrate_parameters' cmd output

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (address@hidden) wrote:
> On Fri, Mar 20, 2020 at 05:31:17PM +0000, Dr. David Alan Gilbert wrote:
> > (Rearranging the text a bit)
> > 
> > * Markus Armbruster (address@hidden) wrote:
> > 
> > > David (cc'ed) should be able to tell us which fix is right.
> > > 
> > > @tls_creds and @tls_hostname look like they could have the same issue.
> > 
> > A certain Markus removed the Null checks in 8cc99dc because 4af245d
> > guaranteed they would be None-Null for tls-creds/hostname - so we
> > should be OK for those.
> > 
> > But tls-authz came along a lot later in d2f1d29 and doesn't
> > seem to have the initialisation, which is now in
> > migration_instance_init.
> > 
> > So I *think* the fix for this is to do the modern equivalent of 4af245d
> > :
> > 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index c1d88ace7f..0bc1b93277 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -3686,6 +3686,7 @@ static void migration_instance_init(Object *obj)
> >  
> >      params->tls_hostname = g_strdup("");
> >      params->tls_creds = g_strdup("");
> > +    params->tls_authz = g_strdup("");
> >  
> >      /* Set has_* up only for parameter checks */
> >      params->has_compress_level = true;
> > 
> > Copying in Dan to check that wouldn't break tls.
> 
> It *will* break TLS, because it will cause the TLS code to lookup
> an object with the ID of "".  NULL must be preserved when calling
> the TLS APIs.

OK, good I asked...

> The assignment of "" to tls_hostname would also have broken TLS,
> so the migration_tls_channel_connect method had to turn it back
> into a real NULL.
> 
> The use of "" for tls_creds will similarly cause it to try and
> lookup an object with ID of "", and fail. That one's harmless
> though, because it would also fail if it were NULL.

OK.

It looks like the output of query-migrate-parameters though already
turns it into "", so I don't think you can tell it's NULL from that:

{"QMP": {"version": {"qemu": {"micro": 0, "minor": 2, "major": 4}, "package": 
"qemu-4.2.0-4.fc31"}, "capabilities": ["oob"]}}
{ "execute": "qmp_capabilities" }
{"return": {}}
{ "execute": "query-migrate-parameters" }
{"return": {"xbzrle-cache-size": 67108864, "cpu-throttle-initial": 20, 
"announce-max": 550, "decompress-threads": 2, "compress-threads": 8, 
"compress-level": 1, "multifd-channels": 2, "announce-initial": 50, 
"block-incremental": false, "compress-wait-thread": true, "downtime-limit": 
300, "tls-authz": "", "announce-rounds": 5, "announce-step": 100, "tls-creds": 
"", "max-cpu-throttle": 99, "max-postcopy-bandwidth": 0, "tls-hostname": "", 
"max-bandwidth": 33554432, "x-checkpoint-delay": 20000, 
"cpu-throttle-increment": 10}}

I'm not sure who turns a Null into a "" but I guess it's somewhere in
the json output iterator.

So we can fix this problem either in qmp_query_migrate_parameters
and just strdup a "", or substitute it in hmp_info_migrate_parameters.

Dave

> 
> 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 :|
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK