Re: [PATCH v2] migration: use "" instead of (null) for tls-authz

[maozy]

Hi Markus,

On 3/30/20 3:18 PM, Markus Armbruster wrote:
> "Dr. David Alan Gilbert" <address@hidden> writes:
>
> > * Mao Zhongyi (address@hidden) wrote:
> > > run:
> > > (qemu) info migrate_parameters
> > > announce-initial: 50 ms
> > > ...
> > > announce-max: 550 ms
> > > multifd-compression: none
> > > xbzrle-cache-size: 4194304
> > > max-postcopy-bandwidth: 0
> > >   tls-authz: '(null)'
> > >
> > > Migration parameter 'tls-authz' is used to provide the QOM ID
> > > of a QAuthZ subclass instance that provides the access control
> > > check, default is NULL. But the empty string is not a valid
> > > object ID, so use "" instead of the default. Although it will
> > > fail when lookup an object with ID "", it is harmless, just
> > > consistent with tls_creds.
> >
> > Yes, it's probably the best we can do given Dan's explanation that
> > we can't change tls_authz to be non-null.
>
> As I explained in Message-ID: <address@hidden>, this
> is actually a crash bug on some systems.  The commit message neglects to
> mention that.  Too late to fix now.  Next time :)

Oops, I will continue to follow up on this issue.
> > > Also fixed the bad indentation on the last line.
> > >
> > > Signed-off-by: Mao Zhongyi <address@hidden>
> > > ---
> > >   migration/migration.c | 3 ++-
> > >   monitor/hmp-cmds.c    | 2 +-
> > >   2 files changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/migration/migration.c b/migration/migration.c
> > > index c1d88ace7f..b060153ef7 100644
> > > --- a/migration/migration.c
> > > +++ b/migration/migration.c
> > > @@ -790,7 +790,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error 
> > > **errp)
> > >       params->has_tls_hostname = true;
> > >       params->tls_hostname = g_strdup(s->parameters.tls_hostname);
> > >       params->has_tls_authz = true;
> > > -    params->tls_authz = g_strdup(s->parameters.tls_authz);
> > > +    params->tls_authz = s->parameters.tls_authz ? \
> > > +                        g_strdup(s->parameters.tls_authz) : g_strdup("");
> >
> > The \ is unneeded; this isn't a macro; it's also a little shorter to do
> > it as:
> >      params->tls_authz = g_strdup(s->parameters.tls_authz ?
> >                                   s->parameters.tls_authz : "");
>
> Even shorter:
>
>        params->tls_authz = g_strdup(s->parameters.tls_authz ?: "");
>
> ?: is a GNU C extension.  We use it all over the place.
>
> Just FYI.  I'm *not* asking for the code to be changed.

Yes, it does look cooler, thanks for the clarification. I would prefer
to use this syntax in future patches.

Thanks,
Mao

>