* [PATCH v2] migration: use "" instead of (null) for tls-authz
@ 2020-03-21 13:37 Mao Zhongyi
2020-03-24 10:08 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 4+ messages in thread
From: Mao Zhongyi @ 2020-03-21 13:37 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, berrange, dgilbert, Mao Zhongyi, quintela
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(-)
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 : "");
}
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] migration: use "" instead of (null) for tls-authz
2020-03-21 13:37 [PATCH v2] migration: use "" instead of (null) for tls-authz Mao Zhongyi
@ 2020-03-24 10:08 ` Dr. David Alan Gilbert
2020-03-30 7:18 ` Markus Armbruster
0 siblings, 1 reply; 4+ messages in thread
From: Dr. David Alan Gilbert @ 2020-03-24 10:08 UTC (permalink / raw)
To: Mao Zhongyi; +Cc: armbru, berrange, qemu-devel, quintela
* 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] migration: use "" instead of (null) for tls-authz
2020-03-24 10:08 ` Dr. David Alan Gilbert
@ 2020-03-30 7:18 ` Markus Armbruster
2020-03-30 8:23 ` maozy
0 siblings, 1 reply; 4+ messages in thread
From: Markus Armbruster @ 2020-03-30 7:18 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: berrange, qemu-devel, Mao Zhongyi, quintela
"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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] migration: use "" instead of (null) for tls-authz
2020-03-30 7:18 ` Markus Armbruster
@ 2020-03-30 8:23 ` maozy
0 siblings, 0 replies; 4+ messages in thread
From: maozy @ 2020-03-30 8:23 UTC (permalink / raw)
To: Markus Armbruster, Dr. David Alan Gilbert; +Cc: berrange, qemu-devel, quintela
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
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-03-30 8:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-21 13:37 [PATCH v2] migration: use "" instead of (null) for tls-authz Mao Zhongyi
2020-03-24 10:08 ` Dr. David Alan Gilbert
2020-03-30 7:18 ` Markus Armbruster
2020-03-30 8:23 ` maozy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).