Message ID | 0a9dc2fcb78da13eb326992384bc4e57de83d9f9.1584797648.git.maozhongyi@cmss.chinamobile.com |
---|---|
State | New |
Headers | show |
Series | [v2] migration: use "" instead of (null) for tls-authz | expand |
* Mao Zhongyi (maozhongyi@cmss.chinamobile.com) wrote: > 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)' > > Migration parameter 'tls-authz' is used to provide the QOM ID > of a QAuthZ subclass instance that provides the access control > check, default is NULL. But the empty string is not a valid > object ID, so use "" instead of the default. Although it will > fail when lookup an object with ID "", it is harmless, just > consistent with tls_creds. Yes, it's probably the best we can do given Dan's explanation that we can't change tls_authz to be non-null. > Also fixed the bad indentation on the last line. > > Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com> > --- > migration/migration.c | 3 ++- > monitor/hmp-cmds.c | 2 +- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index c1d88ace7f..b060153ef7 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -790,7 +790,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp) > params->has_tls_hostname = true; > params->tls_hostname = g_strdup(s->parameters.tls_hostname); > params->has_tls_authz = true; > - params->tls_authz = g_strdup(s->parameters.tls_authz); > + params->tls_authz = s->parameters.tls_authz ? \ > + g_strdup(s->parameters.tls_authz) : g_strdup(""); The \ is unneeded; this isn't a macro; it's also a little shorter to do it as: params->tls_authz = g_strdup(s->parameters.tls_authz ? s->parameters.tls_authz : ""); Dave > params->has_max_bandwidth = true; > params->max_bandwidth = s->parameters.max_bandwidth; > params->has_downtime_limit = true; > 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 > > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > * Mao Zhongyi (maozhongyi@cmss.chinamobile.com) wrote: >> 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)' >> >> Migration parameter 'tls-authz' is used to provide the QOM ID >> of a QAuthZ subclass instance that provides the access control >> check, default is NULL. But the empty string is not a valid >> object ID, so use "" instead of the default. Although it will >> fail when lookup an object with ID "", it is harmless, just >> consistent with tls_creds. > > Yes, it's probably the best we can do given Dan's explanation that > we can't change tls_authz to be non-null. As I explained in Message-ID: <878sjv11xm.fsf@dusky.pond.sub.org>, this is actually a crash bug on some systems. The commit message neglects to mention that. Too late to fix now. Next time :) >> Also fixed the bad indentation on the last line. >> >> Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com> >> --- >> migration/migration.c | 3 ++- >> monitor/hmp-cmds.c | 2 +- >> 2 files changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/migration/migration.c b/migration/migration.c >> index c1d88ace7f..b060153ef7 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -790,7 +790,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp) >> params->has_tls_hostname = true; >> params->tls_hostname = g_strdup(s->parameters.tls_hostname); >> params->has_tls_authz = true; >> - params->tls_authz = g_strdup(s->parameters.tls_authz); >> + params->tls_authz = s->parameters.tls_authz ? \ >> + g_strdup(s->parameters.tls_authz) : g_strdup(""); > > The \ is unneeded; this isn't a macro; it's also a little shorter to do > it as: > params->tls_authz = g_strdup(s->parameters.tls_authz ? > s->parameters.tls_authz : ""); Even shorter: params->tls_authz = g_strdup(s->parameters.tls_authz ?: ""); ?: is a GNU C extension. We use it all over the place. Just FYI. I'm *not* asking for the code to be changed.
Hi Markus, On 3/30/20 3:18 PM, Markus Armbruster wrote: > "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > >> * Mao Zhongyi (maozhongyi@cmss.chinamobile.com) wrote: >>> 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)' >>> >>> Migration parameter 'tls-authz' is used to provide the QOM ID >>> of a QAuthZ subclass instance that provides the access control >>> check, default is NULL. But the empty string is not a valid >>> object ID, so use "" instead of the default. Although it will >>> fail when lookup an object with ID "", it is harmless, just >>> consistent with tls_creds. >> >> Yes, it's probably the best we can do given Dan's explanation that >> we can't change tls_authz to be non-null. > > As I explained in Message-ID: <878sjv11xm.fsf@dusky.pond.sub.org>, this > is actually a crash bug on some systems. The commit message neglects to > mention that. Too late to fix now. Next time :) Oops, I will continue to follow up on this issue. > >>> Also fixed the bad indentation on the last line. >>> >>> Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com> >>> --- >>> migration/migration.c | 3 ++- >>> monitor/hmp-cmds.c | 2 +- >>> 2 files changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/migration/migration.c b/migration/migration.c >>> index c1d88ace7f..b060153ef7 100644 >>> --- a/migration/migration.c >>> +++ b/migration/migration.c >>> @@ -790,7 +790,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp) >>> params->has_tls_hostname = true; >>> params->tls_hostname = g_strdup(s->parameters.tls_hostname); >>> params->has_tls_authz = true; >>> - params->tls_authz = g_strdup(s->parameters.tls_authz); >>> + params->tls_authz = s->parameters.tls_authz ? \ >>> + g_strdup(s->parameters.tls_authz) : g_strdup(""); >> >> The \ is unneeded; this isn't a macro; it's also a little shorter to do >> it as: >> params->tls_authz = g_strdup(s->parameters.tls_authz ? >> s->parameters.tls_authz : ""); > > Even shorter: > > params->tls_authz = g_strdup(s->parameters.tls_authz ?: ""); > > ?: is a GNU C extension. We use it all over the place. > > Just FYI. I'm *not* asking for the code to be changed. Yes, it does look cooler, thanks for the clarification. I would prefer to use this syntax in future patches. Thanks, Mao > >
diff --git a/migration/migration.c b/migration/migration.c index c1d88ace7f..b060153ef7 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -790,7 +790,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp) params->has_tls_hostname = true; params->tls_hostname = g_strdup(s->parameters.tls_hostname); params->has_tls_authz = true; - params->tls_authz = g_strdup(s->parameters.tls_authz); + params->tls_authz = s->parameters.tls_authz ? \ + g_strdup(s->parameters.tls_authz) : g_strdup(""); params->has_max_bandwidth = true; params->max_bandwidth = s->parameters.max_bandwidth; params->has_downtime_limit = true; 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 : ""); }
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)' Migration parameter 'tls-authz' is used to provide the QOM ID of a QAuthZ subclass instance that provides the access control check, default is NULL. But the empty string is not a valid object ID, so use "" instead of the default. Although it will fail when lookup an object with ID "", it is harmless, just consistent with tls_creds. Also fixed the bad indentation on the last line. Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com> --- migration/migration.c | 3 ++- monitor/hmp-cmds.c | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-)