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

[maozy]

On 3/21/20 3:14 PM, Markus Armbruster wrote:
> "Dr. David Alan Gilbert" <address@hidden> writes:
>
> > * 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.
> It's this wart:
>
>      static void qobject_output_type_str(Visitor *v, const char *name, char 
> **obj,
>                                          Error **errp)
>      {
>          QObjectOutputVisitor *qov = to_qov(v);
>          if (*obj) {
>              qobject_output_add(qov, name, qstring_from_str(*obj));
>          } else {
>              qobject_output_add(qov, name, qstring_from_str(""));
>          }
>      }
>
> Warty since day 1.
>
> In theory, !*obj should not happen, since QAPI type 'str' is not
> nullable.
>
> In practice, it handwritten code can easily make it happen.
>
> > So we can fix this problem either in qmp_query_migrate_parameters
> > and just strdup a "", or substitute it in hmp_info_migrate_parameters.
> Fixing it in qmp_query_migrate_parameters() is cleaner: it avoids null
> 'str', and bypasses the wart.
OK,  thanks for the kind reply, will fix it in v2.

Thanks,
Mao