[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: maozy
Subject: Re: [PATCH] monitor/hmp-cmds: fix bad indentation in 'info migrate_parameters' cmd output
Date: Sat, 21 Mar 2020 19:23:06 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0

On 3/21/20 3:14 PM, Markus Armbruster wrote:
"Dr. David Alan Gilbert" <address@hidden> writes:

* Daniel P. Berrangé (address@hidden) wrote:
On Fri, Mar 20, 2020 at 05:31:17PM +0000, Dr. David Alan Gilbert wrote:
(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

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.

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

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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]