* [PATCH] monitor/hmp-cmds: fix bad indentation in 'info migrate_parameters' cmd output @ 2020-03-20 13:00 Mao Zhongyi 2020-03-20 15:27 ` Markus Armbruster 0 siblings, 1 reply; 7+ messages in thread From: Mao Zhongyi @ 2020-03-20 13:00 UTC (permalink / raw) To: qemu-devel; +Cc: dgilbert, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] monitor/hmp-cmds: fix bad indentation in 'info migrate_parameters' cmd output 2020-03-20 13:00 [PATCH] monitor/hmp-cmds: fix bad indentation in 'info migrate_parameters' cmd output Mao Zhongyi @ 2020-03-20 15:27 ` Markus Armbruster 2020-03-20 17:31 ` Dr. David Alan Gilbert 0 siblings, 1 reply; 7+ messages in thread From: Markus Armbruster @ 2020-03-20 15:27 UTC (permalink / raw) To: Mao Zhongyi; +Cc: qemu-devel, dgilbert 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] monitor/hmp-cmds: fix bad indentation in 'info migrate_parameters' cmd output 2020-03-20 15:27 ` Markus Armbruster @ 2020-03-20 17:31 ` Dr. David Alan Gilbert 2020-03-20 17:49 ` Daniel P. Berrangé 0 siblings, 1 reply; 7+ messages in thread From: Dr. David Alan Gilbert @ 2020-03-20 17:31 UTC (permalink / raw) To: Markus Armbruster, berrange; +Cc: qemu-devel, Mao Zhongyi (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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] monitor/hmp-cmds: fix bad indentation in 'info migrate_parameters' cmd output 2020-03-20 17:31 ` Dr. David Alan Gilbert @ 2020-03-20 17:49 ` Daniel P. Berrangé 2020-03-20 18:07 ` Dr. David Alan Gilbert 0 siblings, 1 reply; 7+ messages in thread From: Daniel P. Berrangé @ 2020-03-20 17:49 UTC (permalink / raw) To: Dr. David Alan Gilbert; +Cc: Markus Armbruster, Mao Zhongyi, qemu-devel 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 :| ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] monitor/hmp-cmds: fix bad indentation in 'info migrate_parameters' cmd output 2020-03-20 17:49 ` Daniel P. Berrangé @ 2020-03-20 18:07 ` Dr. David Alan Gilbert 2020-03-21 7:14 ` Markus Armbruster 0 siblings, 1 reply; 7+ messages in thread From: Dr. David Alan Gilbert @ 2020-03-20 18:07 UTC (permalink / raw) To: Daniel P. Berrangé; +Cc: Markus Armbruster, Mao Zhongyi, qemu-devel * 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] monitor/hmp-cmds: fix bad indentation in 'info migrate_parameters' cmd output 2020-03-20 18:07 ` Dr. David Alan Gilbert @ 2020-03-21 7:14 ` Markus Armbruster 2020-03-21 11:23 ` maozy 0 siblings, 1 reply; 7+ messages in thread From: Markus Armbruster @ 2020-03-21 7:14 UTC (permalink / raw) To: Dr. David Alan Gilbert; +Cc: Daniel P. Berrangé, qemu-devel, Mao Zhongyi "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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] monitor/hmp-cmds: fix bad indentation in 'info migrate_parameters' cmd output 2020-03-21 7:14 ` Markus Armbruster @ 2020-03-21 11:23 ` maozy 0 siblings, 0 replies; 7+ messages in thread From: maozy @ 2020-03-21 11:23 UTC (permalink / raw) To: Markus Armbruster, Dr. David Alan Gilbert Cc: Daniel P. Berrangé, qemu-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-03-21 11:24 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-20 13:00 [PATCH] monitor/hmp-cmds: fix bad indentation in 'info migrate_parameters' cmd output Mao Zhongyi 2020-03-20 15:27 ` Markus Armbruster 2020-03-20 17:31 ` Dr. David Alan Gilbert 2020-03-20 17:49 ` Daniel P. Berrangé 2020-03-20 18:07 ` Dr. David Alan Gilbert 2020-03-21 7:14 ` Markus Armbruster 2020-03-21 11:23 ` maozy
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.