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

[Markus Armbruster]

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.

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.