* Daniel P. Berrangé (berra...@redhat.com) wrote:
> On Fri, Mar 20, 2020 at 05:31:17PM +0000, Dr. David Alan Gilbert wrote:
> > (Rearranging the text a bit)
> >
> > * Markus Armbruster (arm...@redhat.com) 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 / dgilb...@redhat.com / Manchester, UK
