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

[Dr. David Alan Gilbert]

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

Dave

> Mao Zhongyi <address@hidden> writes:
> 
> > 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)'
> >
> > The last line seems a bit out of place, fix it.
> 
> Yes, indentation is off, and your patch fixes that.  But there's also
> the '(null)', which emanates a certain bug smell.  Let's have a look at
> the code:
> 
> > Signed-off-by: Mao Zhongyi <address@hidden>
> > ---
> >  monitor/hmp-cmds.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> > index 58724031ea..f8be6bbb16 100644
> > --- a/monitor/hmp-cmds.c
> > +++ b/monitor/hmp-cmds.c
> > @@ -459,7 +459,7 @@ void hmp_info_migrate_parameters(Monitor *mon, const 
> > QDict *qdict)
>    void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>    {
>        MigrationParameters *params;
> 
>        params = qmp_query_migrate_parameters(NULL);
> 
>        if (params) {
>            [...]
> >          monitor_printf(mon, "%s: %" PRIu64 "\n",
> >              
> > MigrationParameter_str(MIGRATION_PARAMETER_MAX_POSTCOPY_BANDWIDTH),
> >              params->max_postcopy_bandwidth);
> > -        monitor_printf(mon, " %s: '%s'\n",
> > +        monitor_printf(mon, "%s: '%s'\n",
> >              MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
> >              params->has_tls_authz ? params->tls_authz : "");
> >      }
> 
> Here, params->tls_authz is null even though params->has_tls_authz is
> true.
> 
> GNU Libc is nice enough not to crash when you attempt to print a null
> pointer, but other libcs are not.
> 
> Where does the null pointer come from?
> 
>    MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>    {
>        MigrationParameters *params;
>        MigrationState *s = migrate_get_current();
> 
>        /* TODO use QAPI_CLONE() instead of duplicating it inline */
>        params = g_malloc0(sizeof(*params));
>        [...]
> --->   params->has_tls_authz = true;
> --->   params->tls_authz = g_strdup(s->parameters.tls_authz);
>        [...]
> 
>        return params;
>    }
> 
> Note we ignore s->parameters.has_tls_authz.
> 
> If @tls_authz is should be present in params exactly when it is present
> in s->params, we should do this:
> 
>        params->has_tls_authz = s->parameters.has_tls_authz;
>        params->tls_authz = g_strdup(s->parameters.tls_authz);
> 
> If @tls_authz is should be present exactly when it's not null, we should
> do this:
> 
>        params->has_tls_authz = !!s->parameters.tls_authz;
>        params->tls_authz = g_strdup(s->parameters.tls_authz);
> 
> If @tls_authz should always be present, we need to substitute the null
> pointer by a suitable string, like this:
> 
>        params->has_tls_authz = true;
>        params->tls_authz = s->parameters.tls_authz
>            ? g_strdup(s->parameters.tls_authz) : "";
> 
> The /* TODO use QAPI_CLONE() instead of duplicating it inline */
> suggests yet another possible fix.
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK