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.
> >>> +}
> >>
> >> 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!
>
--
Peter Xu
