Re: [PATCH v2 03/24] migration: Normalize tls arguments

[Fabiano Rosas <farosas@suse.de>]

Fabiano Rosas <farosas@suse.de> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Fabiano Rosas <farosas@suse.de> writes:
>>

...

>>> diff --git a/migration/options.c b/migration/options.c
>>> index 384ef9e421..f7bbdba5fc 100644
>>> --- a/migration/options.c
>>> +++ b/migration/options.c
>>
>> [...]
>>
>>> @@ -935,6 +951,37 @@ AnnounceParameters *migrate_announce_params(void)
>>>      return &ap;
>>>  }
>>>  
>>> +void migrate_tls_opts_free(MigrationParameters *params)
>>> +{
>>> +    qapi_free_StrOrNull(params->tls_creds);
>>> +    qapi_free_StrOrNull(params->tls_hostname);
>>> +    qapi_free_StrOrNull(params->tls_authz);
>>> +}
>>> +
>>> +/* either non-empty or empty string */
>>
>> This isn't true, because ...
>>
>>> +static void tls_opt_to_str(StrOrNull **tls_opt)
>>> +{
>>> +    StrOrNull *opt = *tls_opt;
>>> +
>>> +    if (!opt) {
>>> +        return;
>>
>> ... it can also be null.
>>
>
> Hmm, I'll have to double check, but with the StrOrNull property being
> initialized, NULL should not be possible. This looks like a mistake.
>

The code is correct, this is coming from the QAPI, so it could be NULL
in case the user hasn't provided the option. I'll use your suggested
wording and the code suggestion as well.

>> Maybe
>>
>>    /* Normalize QTYPE_QNULL to QTYPE_QSTRING "" */
>>
>>> +    }
>>> +
>>> +    switch (opt->type) {
>>> +    case QTYPE_QSTRING:
>>> +        return;
>>> +    case QTYPE_QNULL:
>>> +        qobject_unref(opt->u.n);
>>> +        break;
>>> +    default:
>>> +        g_assert_not_reached();
>>> +    }
>>> +
>>> +    opt->type = QTYPE_QSTRING;
>>> +    opt->u.s = g_strdup("");
>>> +    *tls_opt = opt;
>>> +}
>>
>> I'd prefer something like
>>
>>        if (!opt || opt->type == QTYPE_QSTRING) {
>>            return;
>>        }
>>        qobject_unref(opt->u.n);
>>        opt->type = QTYPE_QSTRING;
>>        opt->u.s = g_strdup("");
>>        *tls_opt = opt;
>>
>> But this is clearly a matter of taste.

This is better indeed. I was moving back-and-forth between
implementations and the code ended up a bit weird. Thanks!