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

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

[Mao Zhongyi]

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.

Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
---
 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)
         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 : "");
     }
-- 
2.17.1

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

[Markus Armbruster]

Mao Zhongyi <maozhongyi@cmss.chinamobile.com> 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 <maozhongyi@cmss.chinamobile.com>
> ---
>  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.

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 (armbru@redhat.com) 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 <maozhongyi@cmss.chinamobile.com> 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 <maozhongyi@cmss.chinamobile.com>
> > ---
> >  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 / dgilbert@redhat.com / Manchester, UK

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

[Daniel P. Berrangé]

On Fri, Mar 20, 2020 at 05:31:17PM +0000, Dr. David Alan Gilbert wrote:
> (Rearranging the text a bit)
> 
> * Markus Armbruster (armbru@redhat.com) 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.

It *will* break TLS, because it will cause the TLS code to lookup
an object with the ID of "".  NULL must be preserved when calling
the TLS APIs.

The assignment of "" to tls_hostname would also have broken TLS,
so the migration_tls_channel_connect method had to turn it back
into a real NULL.

The use of "" for tls_creds will similarly cause it to try and
lookup an object with ID of "", and fail. That one's harmless
though, because it would also fail if it were NULL.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Fri, Mar 20, 2020 at 05:31:17PM +0000, Dr. David Alan Gilbert wrote:
> > (Rearranging the text a bit)
> > 
> > * Markus Armbruster (armbru@redhat.com) 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.
> 
> It *will* break TLS, because it will cause the TLS code to lookup
> an object with the ID of "".  NULL must be preserved when calling
> the TLS APIs.

OK, good I asked...

> The assignment of "" to tls_hostname would also have broken TLS,
> so the migration_tls_channel_connect method had to turn it back
> into a real NULL.
> 
> The use of "" for tls_creds will similarly cause it to try and
> lookup an object with ID of "", and fail. That one's harmless
> though, because it would also fail if it were NULL.

OK.

It looks like the output of query-migrate-parameters though already
turns it into "", so I don't think you can tell it's NULL from that:

{"QMP": {"version": {"qemu": {"micro": 0, "minor": 2, "major": 4}, "package": "qemu-4.2.0-4.fc31"}, "capabilities": ["oob"]}}
{ "execute": "qmp_capabilities" }
{"return": {}}
{ "execute": "query-migrate-parameters" }
{"return": {"xbzrle-cache-size": 67108864, "cpu-throttle-initial": 20, "announce-max": 550, "decompress-threads": 2, "compress-threads": 8, "compress-level": 1, "multifd-channels": 2, "announce-initial": 50, "block-incremental": false, "compress-wait-thread": true, "downtime-limit": 300, "tls-authz": "", "announce-rounds": 5, "announce-step": 100, "tls-creds": "", "max-cpu-throttle": 99, "max-postcopy-bandwidth": 0, "tls-hostname": "", "max-bandwidth": 33554432, "x-checkpoint-delay": 20000, "cpu-throttle-increment": 10}}

I'm not sure who turns a Null into a "" but I guess it's somewhere in
the json output iterator.

So we can fix this problem either in qmp_query_migrate_parameters
and just strdup a "", or substitute it in hmp_info_migrate_parameters.

Dave

> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Daniel P. Berrangé (berrange@redhat.com) wrote:
>> On Fri, Mar 20, 2020 at 05:31:17PM +0000, Dr. David Alan Gilbert wrote:
>> > (Rearranging the text a bit)
>> > 
>> > * Markus Armbruster (armbru@redhat.com) 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.
>> 
>> It *will* break TLS, because it will cause the TLS code to lookup
>> an object with the ID of "".  NULL must be preserved when calling
>> the TLS APIs.
>
> OK, good I asked...
>
>> The assignment of "" to tls_hostname would also have broken TLS,
>> so the migration_tls_channel_connect method had to turn it back
>> into a real NULL.
>> 
>> The use of "" for tls_creds will similarly cause it to try and
>> lookup an object with ID of "", and fail. That one's harmless
>> though, because it would also fail if it were NULL.
>
> OK.
>
> It looks like the output of query-migrate-parameters though already
> turns it into "", so I don't think you can tell it's NULL from that:
>
> {"QMP": {"version": {"qemu": {"micro": 0, "minor": 2, "major": 4}, "package": "qemu-4.2.0-4.fc31"}, "capabilities": ["oob"]}}
> { "execute": "qmp_capabilities" }
> {"return": {}}
> { "execute": "query-migrate-parameters" }
> {"return": {"xbzrle-cache-size": 67108864, "cpu-throttle-initial": 20, "announce-max": 550, "decompress-threads": 2, "compress-threads": 8, "compress-level": 1, "multifd-channels": 2, "announce-initial": 50, "block-incremental": false, "compress-wait-thread": true, "downtime-limit": 300, "tls-authz": "", "announce-rounds": 5, "announce-step": 100, "tls-creds": "", "max-cpu-throttle": 99, "max-postcopy-bandwidth": 0, "tls-hostname": "", "max-bandwidth": 33554432, "x-checkpoint-delay": 20000, "cpu-throttle-increment": 10}}
>
> I'm not sure who turns a Null into a "" but I guess it's somewhere in
> the json output iterator.

It's this wart:

    static void qobject_output_type_str(Visitor *v, const char *name, char **obj,
                                        Error **errp)
    {
        QObjectOutputVisitor *qov = to_qov(v);
        if (*obj) {
            qobject_output_add(qov, name, qstring_from_str(*obj));
        } else {
            qobject_output_add(qov, name, qstring_from_str(""));
        }
    }

Warty since day 1.

In theory, !*obj should not happen, since QAPI type 'str' is not
nullable.

In practice, it handwritten code can easily make it happen.

> So we can fix this problem either in qmp_query_migrate_parameters
> and just strdup a "", or substitute it in hmp_info_migrate_parameters.

Fixing it in qmp_query_migrate_parameters() is cleaner: it avoids null
'str', and bypasses the wart.

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

[maozy]

On 3/21/20 3:14 PM, Markus Armbruster wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>
>> * Daniel P. Berrangé (berrange@redhat.com) wrote:
>>> On Fri, Mar 20, 2020 at 05:31:17PM +0000, Dr. David Alan Gilbert wrote:
>>>> (Rearranging the text a bit)
>>>>
>>>> * Markus Armbruster (armbru@redhat.com) 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.
>>> It *will* break TLS, because it will cause the TLS code to lookup
>>> an object with the ID of "".  NULL must be preserved when calling
>>> the TLS APIs.
>> OK, good I asked...
>>
>>> The assignment of "" to tls_hostname would also have broken TLS,
>>> so the migration_tls_channel_connect method had to turn it back
>>> into a real NULL.
>>>
>>> The use of "" for tls_creds will similarly cause it to try and
>>> lookup an object with ID of "", and fail. That one's harmless
>>> though, because it would also fail if it were NULL.
>> OK.
>>
>> It looks like the output of query-migrate-parameters though already
>> turns it into "", so I don't think you can tell it's NULL from that:
>>
>> {"QMP": {"version": {"qemu": {"micro": 0, "minor": 2, "major": 4}, "package": "qemu-4.2.0-4.fc31"}, "capabilities": ["oob"]}}
>> { "execute": "qmp_capabilities" }
>> {"return": {}}
>> { "execute": "query-migrate-parameters" }
>> {"return": {"xbzrle-cache-size": 67108864, "cpu-throttle-initial": 20, "announce-max": 550, "decompress-threads": 2, "compress-threads": 8, "compress-level": 1, "multifd-channels": 2, "announce-initial": 50, "block-incremental": false, "compress-wait-thread": true, "downtime-limit": 300, "tls-authz": "", "announce-rounds": 5, "announce-step": 100, "tls-creds": "", "max-cpu-throttle": 99, "max-postcopy-bandwidth": 0, "tls-hostname": "", "max-bandwidth": 33554432, "x-checkpoint-delay": 20000, "cpu-throttle-increment": 10}}
>>
>> I'm not sure who turns a Null into a "" but I guess it's somewhere in
>> the json output iterator.
> It's this wart:
>
>      static void qobject_output_type_str(Visitor *v, const char *name, char **obj,
>                                          Error **errp)
>      {
>          QObjectOutputVisitor *qov = to_qov(v);
>          if (*obj) {
>              qobject_output_add(qov, name, qstring_from_str(*obj));
>          } else {
>              qobject_output_add(qov, name, qstring_from_str(""));
>          }
>      }
>
> Warty since day 1.
>
> In theory, !*obj should not happen, since QAPI type 'str' is not
> nullable.
>
> In practice, it handwritten code can easily make it happen.
>
>> So we can fix this problem either in qmp_query_migrate_parameters
>> and just strdup a "", or substitute it in hmp_info_migrate_parameters.
> Fixing it in qmp_query_migrate_parameters() is cleaner: it avoids null
> 'str', and bypasses the wart.
>
OK,  thanks for the kind reply, will fix it in v2.

Thanks,
Mao