[PATCH v2] migration: use "" instead of (null) for tls-authz

Mao Zhongyi posted 1 patch 4 years ago
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test checkpatch passed
Test FreeBSD passed
Test asan passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/0a9dc2fcb78da13eb326992384bc4e57de83d9f9.1584797648.git.maozhongyi@cmss.chinamobile.com
Maintainers: "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Juan Quintela <quintela@redhat.com>
There is a newer version of this series
migration/migration.c | 3 ++-
monitor/hmp-cmds.c    | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
[PATCH v2] migration: use "" instead of (null) for tls-authz
Posted by Mao Zhongyi 4 years ago
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




Re: [PATCH v2] migration: use "" instead of (null) for tls-authz
Posted by Dr. David Alan Gilbert 4 years ago
* 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


Re: [PATCH v2] migration: use "" instead of (null) for tls-authz
Posted by Markus Armbruster 4 years ago
"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.


Re: [PATCH v2] migration: use "" instead of (null) for tls-authz
Posted by maozy 4 years ago
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

> 
>