Peter Xu <pet...@redhat.com> writes:
> On Fri, Jul 04, 2025 at 10:12:33AM -0300, Fabiano Rosas wrote:
>
> [...]
>
>> >>> +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.
>
> One more trivial question:
>
>>
>> >> 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;
>
> Does tls_opt ever change? I wonder if this line is not needed, instead
> tls_opt_to_str() can take an "StrOrNull *opt" directly.
>
Well spotted!
>> >>> +}
>> >>
>> >> 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!
>>
