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

[Markus Armbruster]

"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.