[PATCH 0/6] migration: Fixes and cleanups aroung migrate-set-parameters

[PATCH 0/6] migration: Fixes and cleanups aroung migrate-set-parameters

[Markus Armbruster]

Not sure about 5.2.  The bugs aren't recent regressions.

Markus Armbruster (6):
  migration: Fix and clean up around @tls-authz
  migration: Fix migrate-set-parameters argument validation
  migration: Clean up signed vs. unsigned XBZRLE cache-size
  migration: Check xbzrle-cache-size more carefully
  migration: Fix cache_init()'s "Failed to allocate" error messages
  migration: Fix a few absurdly defective error messages

 qapi/migration.json    | 34 ++++++++++++++++----------------
 migration/migration.h  |  2 +-
 migration/page_cache.h |  2 +-
 migration/ram.h        |  2 +-
 migration/migration.c  | 44 ++++++++++++++++++++++++++----------------
 migration/page_cache.c | 23 +++++++---------------
 migration/ram.c        |  9 +--------
 monitor/hmp-cmds.c     | 26 ++++++++++++-------------
 8 files changed, 68 insertions(+), 74 deletions(-)

-- 
2.26.2

[PATCH 1/6] migration: Fix and clean up around @tls-authz

[Markus Armbruster]

Commit d2f1d29b95 "migration: add support for a "tls-authz" migration
parameter" added MigrationParameters member @tls-authz.  Whereas the
other members aren't really optional (see commit 1bda8b3c695), this
one is genuinely optional: migration_instance_init() leaves it absent,
and migration_tls_channel_process_incoming() passes it to
qcrypto_tls_session_new(), which checks for null.

Commit d2f1d29b95 has a number of issues, though:

* When qmp_query_migrate_parameters() copies migration parameters into
  its reply, it ignores has_tls_authz, and assumes true instead.  When
  it is false,

  - HMP info migrate_parameters prints the null pointer (crash bug on
    some systems), and

  - QMP query-migrate-parameters replies "tls-authz": "" (because the
    QObject output visitor silently maps null pointer to "", which it
    really shouldn't).

  The HMP defect was noticed and fixed in commit 7cd75cbdb8
  'migration: use "" instead of (null) for tls-authz'.  Unfortunately,
  the fix papered over the real bug: it made
  qmp_query_migrate_parameters() map null tls_authz to "".  It also
  dropped the check for has_tls_authz from
  hmp_info_migrate_parameters().

  Revert, and fix qmp_query_migrate_parameters() not to screw up
  has_tls_authz.  No change to HMP.  QMP now has "tls-authz" in the
  reply only when it's actually present in
  migrate_get_current()->parameters.  If we prefer to remain
  bug-compatible, we should make tls_authz non-optional there.

* migrate_params_test_apply() neglects to apply tls_authz.  Currently
  harmless, because migrate_params_check() doesn't care.  Fix it
  anyway.

* qmp_migrate_set_parameters() crashes:

    {"execute": "migrate-set-parameters", "arguments": {"tls-authz": null}}

  Add the necessary rewrite of null to "".  For background
  information, see commit 01fa559826 "migration: Use JSON null instead
  of "" to reset parameter to default".

Fixes: d2f1d29b95aa45d13262b39153ff501ed6b1ac95
Cc: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qapi/migration.json   |  2 +-
 migration/migration.c | 17 ++++++++++++++---
 monitor/hmp-cmds.c    |  2 +-
 3 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index 3c75820527..688e8da749 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -928,7 +928,7 @@
 ##
 # @MigrationParameters:
 #
-# The optional members aren't actually optional.
+# The optional members aren't actually optional, except for @tls-authz.
 #
 # @announce-initial: Initial delay (in milliseconds) before sending the
 #                    first announce (Since 4.0)
diff --git a/migration/migration.c b/migration/migration.c
index 3263aa55a9..cad56fbf8c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -855,9 +855,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->tls_creds = g_strdup(s->parameters.tls_creds);
     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 ?
-                                 s->parameters.tls_authz : "");
+    params->has_tls_authz = s->parameters.has_tls_authz;
+    params->tls_authz = g_strdup(s->parameters.tls_authz);
     params->has_max_bandwidth = true;
     params->max_bandwidth = s->parameters.max_bandwidth;
     params->has_downtime_limit = true;
@@ -1433,6 +1432,11 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
         dest->tls_hostname = params->tls_hostname->u.s;
     }
 
+    if (params->has_tls_authz) {
+        assert(params->tls_authz->type == QTYPE_QSTRING);
+        dest->tls_authz = params->tls_authz->u.s;
+    }
+
     if (params->has_max_bandwidth) {
         dest->max_bandwidth = params->max_bandwidth;
     }
@@ -1622,6 +1626,13 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
         params->tls_hostname->type = QTYPE_QSTRING;
         params->tls_hostname->u.s = strdup("");
     }
+    /* TODO Rewrite "" to null instead */
+    if (params->has_tls_authz
+        && params->tls_authz->type == QTYPE_QNULL) {
+        qobject_unref(params->tls_authz->u.n);
+        params->tls_authz->type = QTYPE_QSTRING;
+        params->tls_authz->u.s = strdup("");
+    }
 
     migrate_params_test_apply(params, &tmp);
 
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index a6a6684df1..492789248f 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -476,7 +476,7 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
             params->max_postcopy_bandwidth);
         monitor_printf(mon, "%s: '%s'\n",
             MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
-            params->tls_authz);
+            params->has_tls_authz ? params->tls_authz : "");
 
         if (params->has_block_bitmap_mapping) {
             const BitmapMigrationNodeAliasList *bmnal;
-- 
2.26.2

Re: [PATCH 1/6] migration: Fix and clean up around @tls-authz

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Commit d2f1d29b95 "migration: add support for a "tls-authz" migration
> parameter" added MigrationParameters member @tls-authz.  Whereas the
> other members aren't really optional (see commit 1bda8b3c695), this
> one is genuinely optional: migration_instance_init() leaves it absent,
> and migration_tls_channel_process_incoming() passes it to
> qcrypto_tls_session_new(), which checks for null.
> 
> Commit d2f1d29b95 has a number of issues, though:
> 
> * When qmp_query_migrate_parameters() copies migration parameters into
>   its reply, it ignores has_tls_authz, and assumes true instead.  When
>   it is false,
> 
>   - HMP info migrate_parameters prints the null pointer (crash bug on
>     some systems), and
> 
>   - QMP query-migrate-parameters replies "tls-authz": "" (because the
>     QObject output visitor silently maps null pointer to "", which it
>     really shouldn't).
> 
>   The HMP defect was noticed and fixed in commit 7cd75cbdb8
>   'migration: use "" instead of (null) for tls-authz'.  Unfortunately,
>   the fix papered over the real bug: it made
>   qmp_query_migrate_parameters() map null tls_authz to "".  It also
>   dropped the check for has_tls_authz from
>   hmp_info_migrate_parameters().
> 
>   Revert, and fix qmp_query_migrate_parameters() not to screw up
>   has_tls_authz.  No change to HMP.  QMP now has "tls-authz" in the
>   reply only when it's actually present in
>   migrate_get_current()->parameters.  If we prefer to remain
>   bug-compatible, we should make tls_authz non-optional there.
> 
> * migrate_params_test_apply() neglects to apply tls_authz.  Currently
>   harmless, because migrate_params_check() doesn't care.  Fix it
>   anyway.
> 
> * qmp_migrate_set_parameters() crashes:
> 
>     {"execute": "migrate-set-parameters", "arguments": {"tls-authz": null}}
> 
>   Add the necessary rewrite of null to "".  For background
>   information, see commit 01fa559826 "migration: Use JSON null instead
>   of "" to reset parameter to default".
> 
> Fixes: d2f1d29b95aa45d13262b39153ff501ed6b1ac95
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Yes, but I'd like an Ack from Dan as well for this one

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  qapi/migration.json   |  2 +-
>  migration/migration.c | 17 ++++++++++++++---
>  monitor/hmp-cmds.c    |  2 +-
>  3 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 3c75820527..688e8da749 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -928,7 +928,7 @@
>  ##
>  # @MigrationParameters:
>  #
> -# The optional members aren't actually optional.
> +# The optional members aren't actually optional, except for @tls-authz.
>  #
>  # @announce-initial: Initial delay (in milliseconds) before sending the
>  #                    first announce (Since 4.0)
> diff --git a/migration/migration.c b/migration/migration.c
> index 3263aa55a9..cad56fbf8c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -855,9 +855,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>      params->tls_creds = g_strdup(s->parameters.tls_creds);
>      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 ?
> -                                 s->parameters.tls_authz : "");
> +    params->has_tls_authz = s->parameters.has_tls_authz;
> +    params->tls_authz = g_strdup(s->parameters.tls_authz);
>      params->has_max_bandwidth = true;
>      params->max_bandwidth = s->parameters.max_bandwidth;
>      params->has_downtime_limit = true;
> @@ -1433,6 +1432,11 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
>          dest->tls_hostname = params->tls_hostname->u.s;
>      }
>  
> +    if (params->has_tls_authz) {
> +        assert(params->tls_authz->type == QTYPE_QSTRING);
> +        dest->tls_authz = params->tls_authz->u.s;
> +    }
> +
>      if (params->has_max_bandwidth) {
>          dest->max_bandwidth = params->max_bandwidth;
>      }
> @@ -1622,6 +1626,13 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
>          params->tls_hostname->type = QTYPE_QSTRING;
>          params->tls_hostname->u.s = strdup("");
>      }
> +    /* TODO Rewrite "" to null instead */
> +    if (params->has_tls_authz
> +        && params->tls_authz->type == QTYPE_QNULL) {
> +        qobject_unref(params->tls_authz->u.n);
> +        params->tls_authz->type = QTYPE_QSTRING;
> +        params->tls_authz->u.s = strdup("");
> +    }
>  
>      migrate_params_test_apply(params, &tmp);
>  
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index a6a6684df1..492789248f 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -476,7 +476,7 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>              params->max_postcopy_bandwidth);
>          monitor_printf(mon, "%s: '%s'\n",
>              MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
> -            params->tls_authz);
> +            params->has_tls_authz ? params->tls_authz : "");
>  
>          if (params->has_block_bitmap_mapping) {
>              const BitmapMigrationNodeAliasList *bmnal;
> -- 
> 2.26.2
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 1/6] migration: Fix and clean up around @tls-authz

[Dr. David Alan Gilbert]

Dan: Can you please see if this makes sense to you, it did to me
at the time.

Dave

* Dr. David Alan Gilbert (dgilbert@redhat.com) wrote:
> * Markus Armbruster (armbru@redhat.com) wrote:
> > Commit d2f1d29b95 "migration: add support for a "tls-authz" migration
> > parameter" added MigrationParameters member @tls-authz.  Whereas the
> > other members aren't really optional (see commit 1bda8b3c695), this
> > one is genuinely optional: migration_instance_init() leaves it absent,
> > and migration_tls_channel_process_incoming() passes it to
> > qcrypto_tls_session_new(), which checks for null.
> > 
> > Commit d2f1d29b95 has a number of issues, though:
> > 
> > * When qmp_query_migrate_parameters() copies migration parameters into
> >   its reply, it ignores has_tls_authz, and assumes true instead.  When
> >   it is false,
> > 
> >   - HMP info migrate_parameters prints the null pointer (crash bug on
> >     some systems), and
> > 
> >   - QMP query-migrate-parameters replies "tls-authz": "" (because the
> >     QObject output visitor silently maps null pointer to "", which it
> >     really shouldn't).
> > 
> >   The HMP defect was noticed and fixed in commit 7cd75cbdb8
> >   'migration: use "" instead of (null) for tls-authz'.  Unfortunately,
> >   the fix papered over the real bug: it made
> >   qmp_query_migrate_parameters() map null tls_authz to "".  It also
> >   dropped the check for has_tls_authz from
> >   hmp_info_migrate_parameters().
> > 
> >   Revert, and fix qmp_query_migrate_parameters() not to screw up
> >   has_tls_authz.  No change to HMP.  QMP now has "tls-authz" in the
> >   reply only when it's actually present in
> >   migrate_get_current()->parameters.  If we prefer to remain
> >   bug-compatible, we should make tls_authz non-optional there.
> > 
> > * migrate_params_test_apply() neglects to apply tls_authz.  Currently
> >   harmless, because migrate_params_check() doesn't care.  Fix it
> >   anyway.
> > 
> > * qmp_migrate_set_parameters() crashes:
> > 
> >     {"execute": "migrate-set-parameters", "arguments": {"tls-authz": null}}
> > 
> >   Add the necessary rewrite of null to "".  For background
> >   information, see commit 01fa559826 "migration: Use JSON null instead
> >   of "" to reset parameter to default".
> > 
> > Fixes: d2f1d29b95aa45d13262b39153ff501ed6b1ac95
> > Cc: Daniel P. Berrangé <berrange@redhat.com>
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> 
> Yes, but I'd like an Ack from Dan as well for this one
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> > ---
> >  qapi/migration.json   |  2 +-
> >  migration/migration.c | 17 ++++++++++++++---
> >  monitor/hmp-cmds.c    |  2 +-
> >  3 files changed, 16 insertions(+), 5 deletions(-)
> > 
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index 3c75820527..688e8da749 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -928,7 +928,7 @@
> >  ##
> >  # @MigrationParameters:
> >  #
> > -# The optional members aren't actually optional.
> > +# The optional members aren't actually optional, except for @tls-authz.
> >  #
> >  # @announce-initial: Initial delay (in milliseconds) before sending the
> >  #                    first announce (Since 4.0)
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 3263aa55a9..cad56fbf8c 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -855,9 +855,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
> >      params->tls_creds = g_strdup(s->parameters.tls_creds);
> >      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 ?
> > -                                 s->parameters.tls_authz : "");
> > +    params->has_tls_authz = s->parameters.has_tls_authz;
> > +    params->tls_authz = g_strdup(s->parameters.tls_authz);
> >      params->has_max_bandwidth = true;
> >      params->max_bandwidth = s->parameters.max_bandwidth;
> >      params->has_downtime_limit = true;
> > @@ -1433,6 +1432,11 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
> >          dest->tls_hostname = params->tls_hostname->u.s;
> >      }
> >  
> > +    if (params->has_tls_authz) {
> > +        assert(params->tls_authz->type == QTYPE_QSTRING);
> > +        dest->tls_authz = params->tls_authz->u.s;
> > +    }
> > +
> >      if (params->has_max_bandwidth) {
> >          dest->max_bandwidth = params->max_bandwidth;
> >      }
> > @@ -1622,6 +1626,13 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
> >          params->tls_hostname->type = QTYPE_QSTRING;
> >          params->tls_hostname->u.s = strdup("");
> >      }
> > +    /* TODO Rewrite "" to null instead */
> > +    if (params->has_tls_authz
> > +        && params->tls_authz->type == QTYPE_QNULL) {
> > +        qobject_unref(params->tls_authz->u.n);
> > +        params->tls_authz->type = QTYPE_QSTRING;
> > +        params->tls_authz->u.s = strdup("");
> > +    }
> >  
> >      migrate_params_test_apply(params, &tmp);
> >  
> > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> > index a6a6684df1..492789248f 100644
> > --- a/monitor/hmp-cmds.c
> > +++ b/monitor/hmp-cmds.c
> > @@ -476,7 +476,7 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
> >              params->max_postcopy_bandwidth);
> >          monitor_printf(mon, "%s: '%s'\n",
> >              MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
> > -            params->tls_authz);
> > +            params->has_tls_authz ? params->tls_authz : "");
> >  
> >          if (params->has_block_bitmap_mapping) {
> >              const BitmapMigrationNodeAliasList *bmnal;
> > -- 
> > 2.26.2
> > 
> -- 
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 1/6] migration: Fix and clean up around @tls-authz

[Daniel P. Berrangé]

On Fri, Nov 13, 2020 at 07:52:31AM +0100, Markus Armbruster wrote:
> Commit d2f1d29b95 "migration: add support for a "tls-authz" migration
> parameter" added MigrationParameters member @tls-authz.  Whereas the
> other members aren't really optional (see commit 1bda8b3c695), this
> one is genuinely optional: migration_instance_init() leaves it absent,
> and migration_tls_channel_process_incoming() passes it to
> qcrypto_tls_session_new(), which checks for null.
> 
> Commit d2f1d29b95 has a number of issues, though:
> 
> * When qmp_query_migrate_parameters() copies migration parameters into
>   its reply, it ignores has_tls_authz, and assumes true instead.  When
>   it is false,
> 
>   - HMP info migrate_parameters prints the null pointer (crash bug on
>     some systems), and
> 
>   - QMP query-migrate-parameters replies "tls-authz": "" (because the
>     QObject output visitor silently maps null pointer to "", which it
>     really shouldn't).
> 
>   The HMP defect was noticed and fixed in commit 7cd75cbdb8
>   'migration: use "" instead of (null) for tls-authz'.  Unfortunately,
>   the fix papered over the real bug: it made
>   qmp_query_migrate_parameters() map null tls_authz to "".  It also
>   dropped the check for has_tls_authz from
>   hmp_info_migrate_parameters().
> 
>   Revert, and fix qmp_query_migrate_parameters() not to screw up
>   has_tls_authz.  No change to HMP.  QMP now has "tls-authz" in the
>   reply only when it's actually present in
>   migrate_get_current()->parameters.  If we prefer to remain
>   bug-compatible, we should make tls_authz non-optional there.
> 
> * migrate_params_test_apply() neglects to apply tls_authz.  Currently
>   harmless, because migrate_params_check() doesn't care.  Fix it
>   anyway.
> 
> * qmp_migrate_set_parameters() crashes:
> 
>     {"execute": "migrate-set-parameters", "arguments": {"tls-authz": null}}
> 
>   Add the necessary rewrite of null to "".  For background
>   information, see commit 01fa559826 "migration: Use JSON null instead
>   of "" to reset parameter to default".
> 
> Fixes: d2f1d29b95aa45d13262b39153ff501ed6b1ac95
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qapi/migration.json   |  2 +-
>  migration/migration.c | 17 ++++++++++++++---
>  monitor/hmp-cmds.c    |  2 +-
>  3 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 3c75820527..688e8da749 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -928,7 +928,7 @@
>  ##
>  # @MigrationParameters:
>  #
> -# The optional members aren't actually optional.
> +# The optional members aren't actually optional, except for @tls-authz.

and tls-hostname and tls-creds.

>  #
>  # @announce-initial: Initial delay (in milliseconds) before sending the
>  #                    first announce (Since 4.0)
> diff --git a/migration/migration.c b/migration/migration.c
> index 3263aa55a9..cad56fbf8c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -855,9 +855,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>      params->tls_creds = g_strdup(s->parameters.tls_creds);
>      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 ?
> -                                 s->parameters.tls_authz : "");
> +    params->has_tls_authz = s->parameters.has_tls_authz;

I'm kind of confused why has_tls_authz needs to be handled differently
from tls_hostname and tls_creds - both of these are optional to
the same extent that tls_authz is AFAIR.

> +    params->tls_authz = g_strdup(s->parameters.tls_authz);

This makes it match what is done for tls_hostname/creds though
which makes sense.

>      params->has_max_bandwidth = true;
>      params->max_bandwidth = s->parameters.max_bandwidth;
>      params->has_downtime_limit = true;
> @@ -1433,6 +1432,11 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
>          dest->tls_hostname = params->tls_hostname->u.s;
>      }
>  
> +    if (params->has_tls_authz) {
> +        assert(params->tls_authz->type == QTYPE_QSTRING);
> +        dest->tls_authz = params->tls_authz->u.s;
> +    }
> +

Makes sense, as it was missed previously

>      if (params->has_max_bandwidth) {
>          dest->max_bandwidth = params->max_bandwidth;
>      }
> @@ -1622,6 +1626,13 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
>          params->tls_hostname->type = QTYPE_QSTRING;
>          params->tls_hostname->u.s = strdup("");
>      }
> +    /* TODO Rewrite "" to null instead */
> +    if (params->has_tls_authz
> +        && params->tls_authz->type == QTYPE_QNULL) {
> +        qobject_unref(params->tls_authz->u.n);
> +        params->tls_authz->type = QTYPE_QSTRING;
> +        params->tls_authz->u.s = strdup("");
> +    }

Makes sense, as it matches what was done for tls_creds/tls_hostname

>  
>      migrate_params_test_apply(params, &tmp);
>  
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index a6a6684df1..492789248f 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -476,7 +476,7 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>              params->max_postcopy_bandwidth);
>          monitor_printf(mon, "%s: '%s'\n",
>              MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
> -            params->tls_authz);
> +            params->has_tls_authz ? params->tls_authz : "");

Again, I'm confused why it needs to be handled differently from
tls_creds / tls_hostname, which are also optional. It feels like
either we need to change all three, or none of them.


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

Re: [PATCH 1/6] migration: Fix and clean up around @tls-authz

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Nov 13, 2020 at 07:52:31AM +0100, Markus Armbruster wrote:
>> Commit d2f1d29b95 "migration: add support for a "tls-authz" migration
>> parameter" added MigrationParameters member @tls-authz.  Whereas the
>> other members aren't really optional (see commit 1bda8b3c695), this
>> one is genuinely optional: migration_instance_init() leaves it absent,
>> and migration_tls_channel_process_incoming() passes it to
>> qcrypto_tls_session_new(), which checks for null.
>> 
>> Commit d2f1d29b95 has a number of issues, though:
>> 
>> * When qmp_query_migrate_parameters() copies migration parameters into
>>   its reply, it ignores has_tls_authz, and assumes true instead.  When
>>   it is false,
>> 
>>   - HMP info migrate_parameters prints the null pointer (crash bug on
>>     some systems), and
>> 
>>   - QMP query-migrate-parameters replies "tls-authz": "" (because the
>>     QObject output visitor silently maps null pointer to "", which it
>>     really shouldn't).
>> 
>>   The HMP defect was noticed and fixed in commit 7cd75cbdb8
>>   'migration: use "" instead of (null) for tls-authz'.  Unfortunately,
>>   the fix papered over the real bug: it made
>>   qmp_query_migrate_parameters() map null tls_authz to "".  It also
>>   dropped the check for has_tls_authz from
>>   hmp_info_migrate_parameters().
>> 
>>   Revert, and fix qmp_query_migrate_parameters() not to screw up
>>   has_tls_authz.  No change to HMP.  QMP now has "tls-authz" in the
>>   reply only when it's actually present in
>>   migrate_get_current()->parameters.  If we prefer to remain
>>   bug-compatible, we should make tls_authz non-optional there.
>> 
>> * migrate_params_test_apply() neglects to apply tls_authz.  Currently
>>   harmless, because migrate_params_check() doesn't care.  Fix it
>>   anyway.
>> 
>> * qmp_migrate_set_parameters() crashes:
>> 
>>     {"execute": "migrate-set-parameters", "arguments": {"tls-authz": null}}
>> 
>>   Add the necessary rewrite of null to "".  For background
>>   information, see commit 01fa559826 "migration: Use JSON null instead
>>   of "" to reset parameter to default".
>> 
>> Fixes: d2f1d29b95aa45d13262b39153ff501ed6b1ac95
>> Cc: Daniel P. Berrangé <berrange@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  qapi/migration.json   |  2 +-
>>  migration/migration.c | 17 ++++++++++++++---
>>  monitor/hmp-cmds.c    |  2 +-
>>  3 files changed, 16 insertions(+), 5 deletions(-)
>> 
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index 3c75820527..688e8da749 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -928,7 +928,7 @@
>>  ##
>>  # @MigrationParameters:
>>  #
>> -# The optional members aren't actually optional.
>> +# The optional members aren't actually optional, except for @tls-authz.
>
> and tls-hostname and tls-creds.

Really?  See [*] below.

>>  #
>>  # @announce-initial: Initial delay (in milliseconds) before sending the
>>  #                    first announce (Since 4.0)
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 3263aa55a9..cad56fbf8c 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -855,9 +855,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
        params->has_tls_creds = true;
>>      params->tls_creds = g_strdup(s->parameters.tls_creds);
>>      params->has_tls_hostname = true;
>>      params->tls_hostname = g_strdup(s->parameters.tls_hostname);

[*] Looks non-optional to me.

>> -    params->has_tls_authz = true;
>> -    params->tls_authz = g_strdup(s->parameters.tls_authz ?
>> -                                 s->parameters.tls_authz : "");
>> +    params->has_tls_authz = s->parameters.has_tls_authz;
>
> I'm kind of confused why has_tls_authz needs to be handled differently
> from tls_hostname and tls_creds - both of these are optional to
> the same extent that tls_authz is AFAIR.

I'm kind of confused about pretty much everything around here :)

The patch hunk is part of the revert of flawed commit 7cd75cbdb8.  We
need to revert both parts or none.

One difference between tls_authz and the others is in
migration_instance_init(): it leaves params->tls_authz null, unlike
->tls_hostname and ->tls_creds.

Hmm, it sets ->has_ for none of them.  Wrong.  If we set ->FOO, we must
also set ->has_FOO = true, and if we leave ->has_FOO false, we should
leave ->FOO null.

Another difference is in migration_tls_channel_process_incoming():
s->parameters.tls_creds must not be null (it's used unchecked in
migration_tls_get_creds()), while s->parameters.tls_authz may be
(qcrypto_tls_session_new() checks).

We need to make up our minds what is optional and what isn't.

>> +    params->tls_authz = g_strdup(s->parameters.tls_authz);
>
> This makes it match what is done for tls_hostname/creds though
> which makes sense.
>
>>      params->has_max_bandwidth = true;
>>      params->max_bandwidth = s->parameters.max_bandwidth;
>>      params->has_downtime_limit = true;
>> @@ -1433,6 +1432,11 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
>>          dest->tls_hostname = params->tls_hostname->u.s;
>>      }
>>  
>> +    if (params->has_tls_authz) {
>> +        assert(params->tls_authz->type == QTYPE_QSTRING);
>> +        dest->tls_authz = params->tls_authz->u.s;
>> +    }
>> +
>
> Makes sense, as it was missed previously

Second item in the commit message's list.

>>      if (params->has_max_bandwidth) {
>>          dest->max_bandwidth = params->max_bandwidth;
>>      }
>> @@ -1622,6 +1626,13 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
>>          params->tls_hostname->type = QTYPE_QSTRING;
>>          params->tls_hostname->u.s = strdup("");
>>      }
>> +    /* TODO Rewrite "" to null instead */
>> +    if (params->has_tls_authz
>> +        && params->tls_authz->type == QTYPE_QNULL) {
>> +        qobject_unref(params->tls_authz->u.n);
>> +        params->tls_authz->type = QTYPE_QSTRING;
>> +        params->tls_authz->u.s = strdup("");
>> +    }
>
> Makes sense, as it matches what was done for tls_creds/tls_hostname

Third item.

>>  
>>      migrate_params_test_apply(params, &tmp);
>>  
>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>> index a6a6684df1..492789248f 100644
>> --- a/monitor/hmp-cmds.c
>> +++ b/monitor/hmp-cmds.c
>> @@ -476,7 +476,7 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>>              params->max_postcopy_bandwidth);
>>          monitor_printf(mon, "%s: '%s'\n",
>>              MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
>> -            params->tls_authz);
>> +            params->has_tls_authz ? params->tls_authz : "");
>
> Again, I'm confused why it needs to be handled differently from
> tls_creds / tls_hostname, which are also optional. It feels like
> either we need to change all three, or none of them.

This is the other part of the revert of flawed commit 7cd75cbdb8.  We
need to revert both parts or none.

Re: [PATCH 1/6] migration: Fix and clean up around @tls-authz

[Daniel P. Berrangé]

On Mon, Dec 14, 2020 at 11:14:34AM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Fri, Nov 13, 2020 at 07:52:31AM +0100, Markus Armbruster wrote:
> >> Commit d2f1d29b95 "migration: add support for a "tls-authz" migration
> >> parameter" added MigrationParameters member @tls-authz.  Whereas the
> >> other members aren't really optional (see commit 1bda8b3c695), this
> >> one is genuinely optional: migration_instance_init() leaves it absent,
> >> and migration_tls_channel_process_incoming() passes it to
> >> qcrypto_tls_session_new(), which checks for null.
> >> 
> >> Commit d2f1d29b95 has a number of issues, though:
> >> 
> >> * When qmp_query_migrate_parameters() copies migration parameters into
> >>   its reply, it ignores has_tls_authz, and assumes true instead.  When
> >>   it is false,
> >> 
> >>   - HMP info migrate_parameters prints the null pointer (crash bug on
> >>     some systems), and
> >> 
> >>   - QMP query-migrate-parameters replies "tls-authz": "" (because the
> >>     QObject output visitor silently maps null pointer to "", which it
> >>     really shouldn't).
> >> 
> >>   The HMP defect was noticed and fixed in commit 7cd75cbdb8
> >>   'migration: use "" instead of (null) for tls-authz'.  Unfortunately,
> >>   the fix papered over the real bug: it made
> >>   qmp_query_migrate_parameters() map null tls_authz to "".  It also
> >>   dropped the check for has_tls_authz from
> >>   hmp_info_migrate_parameters().
> >> 
> >>   Revert, and fix qmp_query_migrate_parameters() not to screw up
> >>   has_tls_authz.  No change to HMP.  QMP now has "tls-authz" in the
> >>   reply only when it's actually present in
> >>   migrate_get_current()->parameters.  If we prefer to remain
> >>   bug-compatible, we should make tls_authz non-optional there.
> >> 
> >> * migrate_params_test_apply() neglects to apply tls_authz.  Currently
> >>   harmless, because migrate_params_check() doesn't care.  Fix it
> >>   anyway.
> >> 
> >> * qmp_migrate_set_parameters() crashes:
> >> 
> >>     {"execute": "migrate-set-parameters", "arguments": {"tls-authz": null}}
> >> 
> >>   Add the necessary rewrite of null to "".  For background
> >>   information, see commit 01fa559826 "migration: Use JSON null instead
> >>   of "" to reset parameter to default".
> >> 
> >> Fixes: d2f1d29b95aa45d13262b39153ff501ed6b1ac95
> >> Cc: Daniel P. Berrangé <berrange@redhat.com>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  qapi/migration.json   |  2 +-
> >>  migration/migration.c | 17 ++++++++++++++---
> >>  monitor/hmp-cmds.c    |  2 +-
> >>  3 files changed, 16 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/qapi/migration.json b/qapi/migration.json
> >> index 3c75820527..688e8da749 100644
> >> --- a/qapi/migration.json
> >> +++ b/qapi/migration.json
> >> @@ -928,7 +928,7 @@
> >>  ##
> >>  # @MigrationParameters:
> >>  #
> >> -# The optional members aren't actually optional.
> >> +# The optional members aren't actually optional, except for @tls-authz.
> >
> > and tls-hostname and tls-creds.
> 
> Really?  See [*] below.
> 
> >>  #
> >>  # @announce-initial: Initial delay (in milliseconds) before sending the
> >>  #                    first announce (Since 4.0)
> >> diff --git a/migration/migration.c b/migration/migration.c
> >> index 3263aa55a9..cad56fbf8c 100644
> >> --- a/migration/migration.c
> >> +++ b/migration/migration.c
> >> @@ -855,9 +855,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>         params->has_tls_creds = true;
> >>      params->tls_creds = g_strdup(s->parameters.tls_creds);
> >>      params->has_tls_hostname = true;
> >>      params->tls_hostname = g_strdup(s->parameters.tls_hostname);
> 
> [*] Looks non-optional to me.

I guess it depends on what you mean by "optional" :-)

When I say they are all optional, I'm talking about from the POV
of the end users / mgmt who first configures a migration operation.

tls-creds only needs to be set if you want to enable TLS

tls-hostname only needs to be set if you need to override the
default hostname used for cert validation.

tls-authz only needs to be set if you want to enable access
control over migration clients.

IOW, all three are optional from the POV of configuring a
migration.

As with many things though, simple theory has turned into
messy reality, by virtue of this previous fixup:

  commit 4af245dc3e6e5c96405b3edb9d75657504256469
  Author: Daniel P. Berrangé <berrange@redhat.com>
  Date:   Wed Mar 15 16:16:03 2017 +0000

    migration: use "" as the default for tls-creds/hostname
    
    The tls-creds parameter has a default value of NULL indicating
    that TLS should not be used. Setting it to non-NULL enables
    use of TLS. Once tls-creds are set to a non-NULL value via the
    monitor, it isn't possible to set them back to NULL again, due
    to current implementation limitations. The empty string is not
    a valid QObject identifier, so this switches to use "" as the
    default, indicating that TLS will not be used
    
    The tls-hostname parameter has a default value of NULL indicating
    the the hostname from the migrate connection URI should be used.
    Again, once tls-hostname is set non-NULL, to override the default
    hostname for x509 cert validation, it isn't possible to reset it
    back to NULL via the monitor. The empty string is not a valid
    hostname, so this switches to use "" as the default, indicating
    that the migrate URI hostname should be used.
    
    Using "" as the default for both, also means that the monitor
    commands "info migrate_parameters" / "query-migrate-parameters"
    will report existance of tls-creds/tls-parameters even when set
    to their default values.
    
    Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
    Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
    Reviewed-by: Eric Blake <eblake@redhat.com>
    
    Signed-off-by: Juan Quintela <quintela@redhat.com>


I have a nasty feeling that libvirt relies on that last paragraph
to determine whether TLS is supported in QEMU or not too :-( Ideally
we should be able to report their existance, but also report that
they are set to NULL. I guess that could be considered a regression
at this point though.

So anyway, this explains why we have the wierd behaviour where
querying parameters always reports them as being set.

> 
> >> -    params->has_tls_authz = true;
> >> -    params->tls_authz = g_strdup(s->parameters.tls_authz ?
> >> -                                 s->parameters.tls_authz : "");
> >> +    params->has_tls_authz = s->parameters.has_tls_authz;
> >
> > I'm kind of confused why has_tls_authz needs to be handled differently
> > from tls_hostname and tls_creds - both of these are optional to
> > the same extent that tls_authz is AFAIR.
> 
> I'm kind of confused about pretty much everything around here :)

So tls_authz was following the wierd precedent used by tls_hostname
and tls_creds in always reporting its own existance, as the empty
string.

> The patch hunk is part of the revert of flawed commit 7cd75cbdb8.  We
> need to revert both parts or none.
> 
> One difference between tls_authz and the others is in
> migration_instance_init(): it leaves params->tls_authz null, unlike
> ->tls_hostname and ->tls_creds.
> 
> Hmm, it sets ->has_ for none of them.  Wrong.  If we set ->FOO, we must
> also set ->has_FOO = true, and if we leave ->has_FOO false, we should
> leave ->FOO null.
> 
> Another difference is in migration_tls_channel_process_incoming():
> s->parameters.tls_creds must not be null (it's used unchecked in
> migration_tls_get_creds()), while s->parameters.tls_authz may be
> (qcrypto_tls_session_new() checks).
> 
> We need to make up our minds what is optional and what isn't.

So they are all optional in terms of what needs to be set.

They are all always reported when querying parameters.

The main difference seems to be that internally we use NULL
as a default for tls_authz, and convert NULL to "" when reporting,
while for tls_creds/tls_hostname we convert NULL to "" immediately
so we always have "" internally.

Should we instead set tls_authz to "" internally straight away
like we do for tls_creds/tls_hostname, and then make the code
turn "" back into NULL at time of use.


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

Re: [PATCH 1/6] migration: Fix and clean up around @tls-authz

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, Dec 14, 2020 at 11:14:34AM +0100, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Fri, Nov 13, 2020 at 07:52:31AM +0100, Markus Armbruster wrote:
>> >> Commit d2f1d29b95 "migration: add support for a "tls-authz" migration
>> >> parameter" added MigrationParameters member @tls-authz.  Whereas the
>> >> other members aren't really optional (see commit 1bda8b3c695), this
>> >> one is genuinely optional: migration_instance_init() leaves it absent,
>> >> and migration_tls_channel_process_incoming() passes it to
>> >> qcrypto_tls_session_new(), which checks for null.
>> >> 
>> >> Commit d2f1d29b95 has a number of issues, though:
>> >> 
>> >> * When qmp_query_migrate_parameters() copies migration parameters into
>> >>   its reply, it ignores has_tls_authz, and assumes true instead.  When
>> >>   it is false,
>> >> 
>> >>   - HMP info migrate_parameters prints the null pointer (crash bug on
>> >>     some systems), and
>> >> 
>> >>   - QMP query-migrate-parameters replies "tls-authz": "" (because the
>> >>     QObject output visitor silently maps null pointer to "", which it
>> >>     really shouldn't).
>> >> 
>> >>   The HMP defect was noticed and fixed in commit 7cd75cbdb8
>> >>   'migration: use "" instead of (null) for tls-authz'.  Unfortunately,
>> >>   the fix papered over the real bug: it made
>> >>   qmp_query_migrate_parameters() map null tls_authz to "".  It also
>> >>   dropped the check for has_tls_authz from
>> >>   hmp_info_migrate_parameters().
>> >> 
>> >>   Revert, and fix qmp_query_migrate_parameters() not to screw up
>> >>   has_tls_authz.  No change to HMP.  QMP now has "tls-authz" in the
>> >>   reply only when it's actually present in
>> >>   migrate_get_current()->parameters.  If we prefer to remain
>> >>   bug-compatible, we should make tls_authz non-optional there.
>> >> 
>> >> * migrate_params_test_apply() neglects to apply tls_authz.  Currently
>> >>   harmless, because migrate_params_check() doesn't care.  Fix it
>> >>   anyway.
>> >> 
>> >> * qmp_migrate_set_parameters() crashes:
>> >> 
>> >>     {"execute": "migrate-set-parameters", "arguments": {"tls-authz": null}}
>> >> 
>> >>   Add the necessary rewrite of null to "".  For background
>> >>   information, see commit 01fa559826 "migration: Use JSON null instead
>> >>   of "" to reset parameter to default".
>> >> 
>> >> Fixes: d2f1d29b95aa45d13262b39153ff501ed6b1ac95
>> >> Cc: Daniel P. Berrangé <berrange@redhat.com>
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> ---
>> >>  qapi/migration.json   |  2 +-
>> >>  migration/migration.c | 17 ++++++++++++++---
>> >>  monitor/hmp-cmds.c    |  2 +-
>> >>  3 files changed, 16 insertions(+), 5 deletions(-)
>> >> 
>> >> diff --git a/qapi/migration.json b/qapi/migration.json
>> >> index 3c75820527..688e8da749 100644
>> >> --- a/qapi/migration.json
>> >> +++ b/qapi/migration.json
>> >> @@ -928,7 +928,7 @@
>> >>  ##
>> >>  # @MigrationParameters:
>> >>  #
>> >> -# The optional members aren't actually optional.
>> >> +# The optional members aren't actually optional, except for @tls-authz.
>> >
>> > and tls-hostname and tls-creds.
>> 
>> Really?  See [*] below.
>> 
>> >>  #
>> >>  # @announce-initial: Initial delay (in milliseconds) before sending the
>> >>  #                    first announce (Since 4.0)
>> >> diff --git a/migration/migration.c b/migration/migration.c
>> >> index 3263aa55a9..cad56fbf8c 100644
>> >> --- a/migration/migration.c
>> >> +++ b/migration/migration.c
>> >> @@ -855,9 +855,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>>         params->has_tls_creds = true;
>> >>      params->tls_creds = g_strdup(s->parameters.tls_creds);
>> >>      params->has_tls_hostname = true;
>> >>      params->tls_hostname = g_strdup(s->parameters.tls_hostname);
>> 
>> [*] Looks non-optional to me.
>
> I guess it depends on what you mean by "optional" :-)

I meant "non-optional in the value of query-migrate-parameters".  The
comment were debating applies to that value, and nothing else.

> When I say they are all optional, I'm talking about from the POV
> of the end users / mgmt who first configures a migration operation.
>
> tls-creds only needs to be set if you want to enable TLS
>
> tls-hostname only needs to be set if you need to override the
> default hostname used for cert validation.
>
> tls-authz only needs to be set if you want to enable access
> control over migration clients.
>
> IOW, all three are optional from the POV of configuring a
> migration.

Understood.

> As with many things though, simple theory has turned into
> messy reality, by virtue of this previous fixup:
>
>   commit 4af245dc3e6e5c96405b3edb9d75657504256469
>   Author: Daniel P. Berrangé <berrange@redhat.com>
>   Date:   Wed Mar 15 16:16:03 2017 +0000
>
>     migration: use "" as the default for tls-creds/hostname
>     
>     The tls-creds parameter has a default value of NULL indicating
>     that TLS should not be used. Setting it to non-NULL enables
>     use of TLS. Once tls-creds are set to a non-NULL value via the
>     monitor, it isn't possible to set them back to NULL again, due
>     to current implementation limitations. The empty string is not
>     a valid QObject identifier, so this switches to use "" as the
>     default, indicating that TLS will not be used
>     
>     The tls-hostname parameter has a default value of NULL indicating
>     the the hostname from the migrate connection URI should be used.
>     Again, once tls-hostname is set non-NULL, to override the default
>     hostname for x509 cert validation, it isn't possible to reset it
>     back to NULL via the monitor. The empty string is not a valid
>     hostname, so this switches to use "" as the default, indicating
>     that the migrate URI hostname should be used.
>     
>     Using "" as the default for both, also means that the monitor
>     commands "info migrate_parameters" / "query-migrate-parameters"
>     will report existance of tls-creds/tls-parameters even when set
>     to their default values.
>     
>     Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>     Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>     Reviewed-by: Eric Blake <eblake@redhat.com>
>     
>     Signed-off-by: Juan Quintela <quintela@redhat.com>
>
>
> I have a nasty feeling that libvirt relies on that last paragraph
> to determine whether TLS is supported in QEMU or not too :-( Ideally
> we should be able to report their existance, but also report that
> they are set to NULL. I guess that could be considered a regression
> at this point though.
>
> So anyway, this explains why we have the wierd behaviour where
> querying parameters always reports them as being set.

Yes.

What do you want me to change in my patch?

>> >> -    params->has_tls_authz = true;
>> >> -    params->tls_authz = g_strdup(s->parameters.tls_authz ?
>> >> -                                 s->parameters.tls_authz : "");
>> >> +    params->has_tls_authz = s->parameters.has_tls_authz;
>> >
>> > I'm kind of confused why has_tls_authz needs to be handled differently
>> > from tls_hostname and tls_creds - both of these are optional to
>> > the same extent that tls_authz is AFAIR.
>> 
>> I'm kind of confused about pretty much everything around here :)
>
> So tls_authz was following the wierd precedent used by tls_hostname
> and tls_creds in always reporting its own existance, as the empty
> string.
>
>> The patch hunk is part of the revert of flawed commit 7cd75cbdb8.  We
>> need to revert both parts or none.
>> 
>> One difference between tls_authz and the others is in
>> migration_instance_init(): it leaves params->tls_authz null, unlike
>> ->tls_hostname and ->tls_creds.
>> 
>> Hmm, it sets ->has_ for none of them.  Wrong.  If we set ->FOO, we must
>> also set ->has_FOO = true, and if we leave ->has_FOO false, we should
>> leave ->FOO null.
>> 
>> Another difference is in migration_tls_channel_process_incoming():
>> s->parameters.tls_creds must not be null (it's used unchecked in
>> migration_tls_get_creds()), while s->parameters.tls_authz may be
>> (qcrypto_tls_session_new() checks).
>> 
>> We need to make up our minds what is optional and what isn't.
>
> So they are all optional in terms of what needs to be set.
>
> They are all always reported when querying parameters.
>
> The main difference seems to be that internally we use NULL
> as a default for tls_authz, and convert NULL to "" when reporting,
> while for tls_creds/tls_hostname we convert NULL to "" immediately
> so we always have "" internally.
>
> Should we instead set tls_authz to "" internally straight away
> like we do for tls_creds/tls_hostname, and then make the code
> turn "" back into NULL at time of use.

I don't know!  I'm merely trying to fix a crash bug I ran into :)

Re: [PATCH 1/6] migration: Fix and clean up around @tls-authz

[Daniel P. Berrangé]

On Thu, Dec 17, 2020 at 02:07:01PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Mon, Dec 14, 2020 at 11:14:34AM +0100, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > On Fri, Nov 13, 2020 at 07:52:31AM +0100, Markus Armbruster wrote:
> >> >> Commit d2f1d29b95 "migration: add support for a "tls-authz" migration
> >> >> parameter" added MigrationParameters member @tls-authz.  Whereas the
> >> >> other members aren't really optional (see commit 1bda8b3c695), this
> >> >> one is genuinely optional: migration_instance_init() leaves it absent,
> >> >> and migration_tls_channel_process_incoming() passes it to
> >> >> qcrypto_tls_session_new(), which checks for null.
> >> >> 
> >> >> Commit d2f1d29b95 has a number of issues, though:
> >> >> 
> >> >> * When qmp_query_migrate_parameters() copies migration parameters into
> >> >>   its reply, it ignores has_tls_authz, and assumes true instead.  When
> >> >>   it is false,
> >> >> 
> >> >>   - HMP info migrate_parameters prints the null pointer (crash bug on
> >> >>     some systems), and
> >> >> 
> >> >>   - QMP query-migrate-parameters replies "tls-authz": "" (because the
> >> >>     QObject output visitor silently maps null pointer to "", which it
> >> >>     really shouldn't).
> >> >> 
> >> >>   The HMP defect was noticed and fixed in commit 7cd75cbdb8
> >> >>   'migration: use "" instead of (null) for tls-authz'.  Unfortunately,
> >> >>   the fix papered over the real bug: it made
> >> >>   qmp_query_migrate_parameters() map null tls_authz to "".  It also
> >> >>   dropped the check for has_tls_authz from
> >> >>   hmp_info_migrate_parameters().
> >> >> 
> >> >>   Revert, and fix qmp_query_migrate_parameters() not to screw up
> >> >>   has_tls_authz.  No change to HMP.  QMP now has "tls-authz" in the
> >> >>   reply only when it's actually present in
> >> >>   migrate_get_current()->parameters.  If we prefer to remain
> >> >>   bug-compatible, we should make tls_authz non-optional there.
> >> >> 
> >> >> * migrate_params_test_apply() neglects to apply tls_authz.  Currently
> >> >>   harmless, because migrate_params_check() doesn't care.  Fix it
> >> >>   anyway.
> >> >> 
> >> >> * qmp_migrate_set_parameters() crashes:
> >> >> 
> >> >>     {"execute": "migrate-set-parameters", "arguments": {"tls-authz": null}}
> >> >> 
> >> >>   Add the necessary rewrite of null to "".  For background
> >> >>   information, see commit 01fa559826 "migration: Use JSON null instead
> >> >>   of "" to reset parameter to default".
> >> >> 
> >> >> Fixes: d2f1d29b95aa45d13262b39153ff501ed6b1ac95
> >> >> Cc: Daniel P. Berrangé <berrange@redhat.com>
> >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> >> ---
> >> >>  qapi/migration.json   |  2 +-
> >> >>  migration/migration.c | 17 ++++++++++++++---
> >> >>  monitor/hmp-cmds.c    |  2 +-
> >> >>  3 files changed, 16 insertions(+), 5 deletions(-)
> >> >> 
> >> >> diff --git a/qapi/migration.json b/qapi/migration.json
> >> >> index 3c75820527..688e8da749 100644
> >> >> --- a/qapi/migration.json
> >> >> +++ b/qapi/migration.json
> >> >> @@ -928,7 +928,7 @@
> >> >>  ##
> >> >>  # @MigrationParameters:
> >> >>  #
> >> >> -# The optional members aren't actually optional.
> >> >> +# The optional members aren't actually optional, except for @tls-authz.
> >> >
> >> > and tls-hostname and tls-creds.
> >> 
> >> Really?  See [*] below.
> >> 
> >> >>  #
> >> >>  # @announce-initial: Initial delay (in milliseconds) before sending the
> >> >>  #                    first announce (Since 4.0)
> >> >> diff --git a/migration/migration.c b/migration/migration.c
> >> >> index 3263aa55a9..cad56fbf8c 100644
> >> >> --- a/migration/migration.c
> >> >> +++ b/migration/migration.c
> >> >> @@ -855,9 +855,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
> >>         params->has_tls_creds = true;
> >> >>      params->tls_creds = g_strdup(s->parameters.tls_creds);
> >> >>      params->has_tls_hostname = true;
> >> >>      params->tls_hostname = g_strdup(s->parameters.tls_hostname);
> >> 
> >> [*] Looks non-optional to me.
> >
> > I guess it depends on what you mean by "optional" :-)
> 
> I meant "non-optional in the value of query-migrate-parameters".  The
> comment were debating applies to that value, and nothing else.
> 
> > When I say they are all optional, I'm talking about from the POV
> > of the end users / mgmt who first configures a migration operation.
> >
> > tls-creds only needs to be set if you want to enable TLS
> >
> > tls-hostname only needs to be set if you need to override the
> > default hostname used for cert validation.
> >
> > tls-authz only needs to be set if you want to enable access
> > control over migration clients.
> >
> > IOW, all three are optional from the POV of configuring a
> > migration.
> 
> Understood.
> 
> > As with many things though, simple theory has turned into
> > messy reality, by virtue of this previous fixup:
> >
> >   commit 4af245dc3e6e5c96405b3edb9d75657504256469
> >   Author: Daniel P. Berrangé <berrange@redhat.com>
> >   Date:   Wed Mar 15 16:16:03 2017 +0000
> >
> >     migration: use "" as the default for tls-creds/hostname
> >     
> >     The tls-creds parameter has a default value of NULL indicating
> >     that TLS should not be used. Setting it to non-NULL enables
> >     use of TLS. Once tls-creds are set to a non-NULL value via the
> >     monitor, it isn't possible to set them back to NULL again, due
> >     to current implementation limitations. The empty string is not
> >     a valid QObject identifier, so this switches to use "" as the
> >     default, indicating that TLS will not be used
> >     
> >     The tls-hostname parameter has a default value of NULL indicating
> >     the the hostname from the migrate connection URI should be used.
> >     Again, once tls-hostname is set non-NULL, to override the default
> >     hostname for x509 cert validation, it isn't possible to reset it
> >     back to NULL via the monitor. The empty string is not a valid
> >     hostname, so this switches to use "" as the default, indicating
> >     that the migrate URI hostname should be used.
> >     
> >     Using "" as the default for both, also means that the monitor
> >     commands "info migrate_parameters" / "query-migrate-parameters"
> >     will report existance of tls-creds/tls-parameters even when set
> >     to their default values.
> >     
> >     Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> >     Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >     Reviewed-by: Eric Blake <eblake@redhat.com>
> >     
> >     Signed-off-by: Juan Quintela <quintela@redhat.com>
> >
> >
> > I have a nasty feeling that libvirt relies on that last paragraph
> > to determine whether TLS is supported in QEMU or not too :-( Ideally
> > we should be able to report their existance, but also report that
> > they are set to NULL. I guess that could be considered a regression
> > at this point though.
> >
> > So anyway, this explains why we have the wierd behaviour where
> > querying parameters always reports them as being set.
> 
> Yes.
> 
> What do you want me to change in my patch?
> 
> >> >> -    params->has_tls_authz = true;
> >> >> -    params->tls_authz = g_strdup(s->parameters.tls_authz ?
> >> >> -                                 s->parameters.tls_authz : "");
> >> >> +    params->has_tls_authz = s->parameters.has_tls_authz;
> >> >
> >> > I'm kind of confused why has_tls_authz needs to be handled differently
> >> > from tls_hostname and tls_creds - both of these are optional to
> >> > the same extent that tls_authz is AFAIR.
> >> 
> >> I'm kind of confused about pretty much everything around here :)
> >
> > So tls_authz was following the wierd precedent used by tls_hostname
> > and tls_creds in always reporting its own existance, as the empty
> > string.
> >
> >> The patch hunk is part of the revert of flawed commit 7cd75cbdb8.  We
> >> need to revert both parts or none.
> >> 
> >> One difference between tls_authz and the others is in
> >> migration_instance_init(): it leaves params->tls_authz null, unlike
> >> ->tls_hostname and ->tls_creds.
> >> 
> >> Hmm, it sets ->has_ for none of them.  Wrong.  If we set ->FOO, we must
> >> also set ->has_FOO = true, and if we leave ->has_FOO false, we should
> >> leave ->FOO null.
> >> 
> >> Another difference is in migration_tls_channel_process_incoming():
> >> s->parameters.tls_creds must not be null (it's used unchecked in
> >> migration_tls_get_creds()), while s->parameters.tls_authz may be
> >> (qcrypto_tls_session_new() checks).
> >> 
> >> We need to make up our minds what is optional and what isn't.
> >
> > So they are all optional in terms of what needs to be set.
> >
> > They are all always reported when querying parameters.
> >
> > The main difference seems to be that internally we use NULL
> > as a default for tls_authz, and convert NULL to "" when reporting,
> > while for tls_creds/tls_hostname we convert NULL to "" immediately
> > so we always have "" internally.
> >
> > Should we instead set tls_authz to "" internally straight away
> > like we do for tls_creds/tls_hostname, and then make the code
> > turn "" back into NULL at time of use.
> 
> I don't know!  I'm merely trying to fix a crash bug I ran into :)

Ok, if you don't mind which approach, then I'd vote for making
migration_instance_init() set  tls_authz to "", in common with
tls_hostname/tls_creds.

Then in migration_tls_channel_process_incoming we can turn the
"" back into NULL.

That way we'll have consistently used "" internally for all the
TLS related parameters.

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

Re: [PATCH 1/6] migration: Fix and clean up around @tls-authz

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Dec 17, 2020 at 02:07:01PM +0100, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Mon, Dec 14, 2020 at 11:14:34AM +0100, Markus Armbruster wrote:
>> >> Daniel P. Berrangé <berrange@redhat.com> writes:
>> >> 
>> >> > On Fri, Nov 13, 2020 at 07:52:31AM +0100, Markus Armbruster wrote:
>> >> >> Commit d2f1d29b95 "migration: add support for a "tls-authz" migration
>> >> >> parameter" added MigrationParameters member @tls-authz.  Whereas the
>> >> >> other members aren't really optional (see commit 1bda8b3c695), this
>> >> >> one is genuinely optional: migration_instance_init() leaves it absent,
>> >> >> and migration_tls_channel_process_incoming() passes it to
>> >> >> qcrypto_tls_session_new(), which checks for null.
>> >> >> 
>> >> >> Commit d2f1d29b95 has a number of issues, though:
>> >> >> 
>> >> >> * When qmp_query_migrate_parameters() copies migration parameters into
>> >> >>   its reply, it ignores has_tls_authz, and assumes true instead.  When
>> >> >>   it is false,
>> >> >> 
>> >> >>   - HMP info migrate_parameters prints the null pointer (crash bug on
>> >> >>     some systems), and
>> >> >> 
>> >> >>   - QMP query-migrate-parameters replies "tls-authz": "" (because the
>> >> >>     QObject output visitor silently maps null pointer to "", which it
>> >> >>     really shouldn't).
>> >> >> 
>> >> >>   The HMP defect was noticed and fixed in commit 7cd75cbdb8
>> >> >>   'migration: use "" instead of (null) for tls-authz'.  Unfortunately,
>> >> >>   the fix papered over the real bug: it made
>> >> >>   qmp_query_migrate_parameters() map null tls_authz to "".  It also
>> >> >>   dropped the check for has_tls_authz from
>> >> >>   hmp_info_migrate_parameters().
>> >> >> 
>> >> >>   Revert, and fix qmp_query_migrate_parameters() not to screw up
>> >> >>   has_tls_authz.  No change to HMP.  QMP now has "tls-authz" in the
>> >> >>   reply only when it's actually present in
>> >> >>   migrate_get_current()->parameters.  If we prefer to remain
>> >> >>   bug-compatible, we should make tls_authz non-optional there.
>> >> >> 
>> >> >> * migrate_params_test_apply() neglects to apply tls_authz.  Currently
>> >> >>   harmless, because migrate_params_check() doesn't care.  Fix it
>> >> >>   anyway.
>> >> >> 
>> >> >> * qmp_migrate_set_parameters() crashes:
>> >> >> 
>> >> >>     {"execute": "migrate-set-parameters", "arguments": {"tls-authz": null}}
>> >> >> 
>> >> >>   Add the necessary rewrite of null to "".  For background
>> >> >>   information, see commit 01fa559826 "migration: Use JSON null instead
>> >> >>   of "" to reset parameter to default".
>> >> >> 
>> >> >> Fixes: d2f1d29b95aa45d13262b39153ff501ed6b1ac95
>> >> >> Cc: Daniel P. Berrangé <berrange@redhat.com>
>> >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> >> ---
>> >> >>  qapi/migration.json   |  2 +-
>> >> >>  migration/migration.c | 17 ++++++++++++++---
>> >> >>  monitor/hmp-cmds.c    |  2 +-
>> >> >>  3 files changed, 16 insertions(+), 5 deletions(-)
>> >> >> 
>> >> >> diff --git a/qapi/migration.json b/qapi/migration.json
>> >> >> index 3c75820527..688e8da749 100644
>> >> >> --- a/qapi/migration.json
>> >> >> +++ b/qapi/migration.json
>> >> >> @@ -928,7 +928,7 @@
>> >> >>  ##
>> >> >>  # @MigrationParameters:
>> >> >>  #
>> >> >> -# The optional members aren't actually optional.
>> >> >> +# The optional members aren't actually optional, except for @tls-authz.
>> >> >
>> >> > and tls-hostname and tls-creds.
>> >> 
>> >> Really?  See [*] below.
>> >> 
>> >> >>  #
>> >> >>  # @announce-initial: Initial delay (in milliseconds) before sending the
>> >> >>  #                    first announce (Since 4.0)
>> >> >> diff --git a/migration/migration.c b/migration/migration.c
>> >> >> index 3263aa55a9..cad56fbf8c 100644
>> >> >> --- a/migration/migration.c
>> >> >> +++ b/migration/migration.c
>> >> >> @@ -855,9 +855,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>> >>         params->has_tls_creds = true;
>> >> >>      params->tls_creds = g_strdup(s->parameters.tls_creds);
>> >> >>      params->has_tls_hostname = true;
>> >> >>      params->tls_hostname = g_strdup(s->parameters.tls_hostname);
>> >> 
>> >> [*] Looks non-optional to me.
>> >
>> > I guess it depends on what you mean by "optional" :-)
>> 
>> I meant "non-optional in the value of query-migrate-parameters".  The
>> comment were debating applies to that value, and nothing else.
>> 
>> > When I say they are all optional, I'm talking about from the POV
>> > of the end users / mgmt who first configures a migration operation.
>> >
>> > tls-creds only needs to be set if you want to enable TLS
>> >
>> > tls-hostname only needs to be set if you need to override the
>> > default hostname used for cert validation.
>> >
>> > tls-authz only needs to be set if you want to enable access
>> > control over migration clients.
>> >
>> > IOW, all three are optional from the POV of configuring a
>> > migration.
>> 
>> Understood.
>> 
>> > As with many things though, simple theory has turned into
>> > messy reality, by virtue of this previous fixup:
>> >
>> >   commit 4af245dc3e6e5c96405b3edb9d75657504256469
>> >   Author: Daniel P. Berrangé <berrange@redhat.com>
>> >   Date:   Wed Mar 15 16:16:03 2017 +0000
>> >
>> >     migration: use "" as the default for tls-creds/hostname
>> >     
>> >     The tls-creds parameter has a default value of NULL indicating
>> >     that TLS should not be used. Setting it to non-NULL enables
>> >     use of TLS. Once tls-creds are set to a non-NULL value via the
>> >     monitor, it isn't possible to set them back to NULL again, due
>> >     to current implementation limitations. The empty string is not
>> >     a valid QObject identifier, so this switches to use "" as the
>> >     default, indicating that TLS will not be used
>> >     
>> >     The tls-hostname parameter has a default value of NULL indicating
>> >     the the hostname from the migrate connection URI should be used.
>> >     Again, once tls-hostname is set non-NULL, to override the default
>> >     hostname for x509 cert validation, it isn't possible to reset it
>> >     back to NULL via the monitor. The empty string is not a valid
>> >     hostname, so this switches to use "" as the default, indicating
>> >     that the migrate URI hostname should be used.
>> >     
>> >     Using "" as the default for both, also means that the monitor
>> >     commands "info migrate_parameters" / "query-migrate-parameters"
>> >     will report existance of tls-creds/tls-parameters even when set
>> >     to their default values.
>> >     
>> >     Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>> >     Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> >     Reviewed-by: Eric Blake <eblake@redhat.com>
>> >     
>> >     Signed-off-by: Juan Quintela <quintela@redhat.com>
>> >
>> >
>> > I have a nasty feeling that libvirt relies on that last paragraph
>> > to determine whether TLS is supported in QEMU or not too :-( Ideally
>> > we should be able to report their existance, but also report that
>> > they are set to NULL. I guess that could be considered a regression
>> > at this point though.
>> >
>> > So anyway, this explains why we have the wierd behaviour where
>> > querying parameters always reports them as being set.
>> 
>> Yes.
>> 
>> What do you want me to change in my patch?
>> 
>> >> >> -    params->has_tls_authz = true;
>> >> >> -    params->tls_authz = g_strdup(s->parameters.tls_authz ?
>> >> >> -                                 s->parameters.tls_authz : "");
>> >> >> +    params->has_tls_authz = s->parameters.has_tls_authz;
>> >> >
>> >> > I'm kind of confused why has_tls_authz needs to be handled differently
>> >> > from tls_hostname and tls_creds - both of these are optional to
>> >> > the same extent that tls_authz is AFAIR.
>> >> 
>> >> I'm kind of confused about pretty much everything around here :)
>> >
>> > So tls_authz was following the wierd precedent used by tls_hostname
>> > and tls_creds in always reporting its own existance, as the empty
>> > string.
>> >
>> >> The patch hunk is part of the revert of flawed commit 7cd75cbdb8.  We
>> >> need to revert both parts or none.
>> >> 
>> >> One difference between tls_authz and the others is in
>> >> migration_instance_init(): it leaves params->tls_authz null, unlike
>> >> ->tls_hostname and ->tls_creds.
>> >> 
>> >> Hmm, it sets ->has_ for none of them.  Wrong.  If we set ->FOO, we must
>> >> also set ->has_FOO = true, and if we leave ->has_FOO false, we should
>> >> leave ->FOO null.
>> >> 
>> >> Another difference is in migration_tls_channel_process_incoming():
>> >> s->parameters.tls_creds must not be null (it's used unchecked in
>> >> migration_tls_get_creds()), while s->parameters.tls_authz may be
>> >> (qcrypto_tls_session_new() checks).
>> >> 
>> >> We need to make up our minds what is optional and what isn't.
>> >
>> > So they are all optional in terms of what needs to be set.
>> >
>> > They are all always reported when querying parameters.
>> >
>> > The main difference seems to be that internally we use NULL
>> > as a default for tls_authz, and convert NULL to "" when reporting,
>> > while for tls_creds/tls_hostname we convert NULL to "" immediately
>> > so we always have "" internally.
>> >
>> > Should we instead set tls_authz to "" internally straight away
>> > like we do for tls_creds/tls_hostname, and then make the code
>> > turn "" back into NULL at time of use.
>> 
>> I don't know!  I'm merely trying to fix a crash bug I ran into :)
>
> Ok, if you don't mind which approach, then I'd vote for making
> migration_instance_init() set  tls_authz to "", in common with
> tls_hostname/tls_creds.
>
> Then in migration_tls_channel_process_incoming we can turn the
> "" back into NULL.
>
> That way we'll have consistently used "" internally for all the
> TLS related parameters.

I suffered mental garbage collection during the Christmas break, and can
no longer make heads or tails out of all this.

I might have to drop the series on the floor :(

[PATCH 2/6] migration: Fix migrate-set-parameters argument validation

[Markus Armbruster]

Commit 741d4086c8 "migration: Use proper types in json" (v2.12.0)
switched MigrationParameters to narrower integer types, and removed
the simplified qmp_migrate_set_parameters()'s argument checking
accordingly.

Good idea, except qmp_migrate_set_parameters() takes
MigrateSetParameters, not MigrationParameters.  Its job is updating
migrate_get_current()->parameters (which *is* of type
MigrationParameters) according to its argument.  The integers now get
truncated silently.  Reproducer:

    ---> {'execute': 'query-migrate-parameters'}
    <--- {"return": {[...] "compress-threads": 8, [...]}}
    ---> {"execute": "migrate-set-parameters", "arguments": {"compress-threads": 257}}
    <--- {"return": {}}
    ---> {'execute': 'query-migrate-parameters'}
    <--- {"return": {[...] "compress-threads": 1, [...]}}

Fix by resynchronizing MigrateSetParameters with MigrationParameters.

Fixes: 741d4086c856320807a2575389d7c0505578270b
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qapi/migration.json | 28 ++++++++++++++--------------
 monitor/hmp-cmds.c  | 24 ++++++++++++------------
 2 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index 688e8da749..3ad3720cf0 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -885,28 +885,28 @@
             '*announce-max': 'size',
             '*announce-rounds': 'size',
             '*announce-step': 'size',
-            '*compress-level': 'int',
-            '*compress-threads': 'int',
+            '*compress-level': 'uint8',
+            '*compress-threads': 'uint8',
             '*compress-wait-thread': 'bool',
-            '*decompress-threads': 'int',
-            '*throttle-trigger-threshold': 'int',
-            '*cpu-throttle-initial': 'int',
-            '*cpu-throttle-increment': 'int',
+            '*decompress-threads': 'uint8',
+            '*throttle-trigger-threshold': 'uint8',
+            '*cpu-throttle-initial': 'uint8',
+            '*cpu-throttle-increment': 'uint8',
             '*cpu-throttle-tailslow': 'bool',
             '*tls-creds': 'StrOrNull',
             '*tls-hostname': 'StrOrNull',
             '*tls-authz': 'StrOrNull',
-            '*max-bandwidth': 'int',
-            '*downtime-limit': 'int',
-            '*x-checkpoint-delay': 'int',
+            '*max-bandwidth': 'size',
+            '*downtime-limit': 'uint64',
+            '*x-checkpoint-delay': 'uint32',
             '*block-incremental': 'bool',
-            '*multifd-channels': 'int',
+            '*multifd-channels': 'uint8',
             '*xbzrle-cache-size': 'size',
             '*max-postcopy-bandwidth': 'size',
-            '*max-cpu-throttle': 'int',
+            '*max-cpu-throttle': 'uint8',
             '*multifd-compression': 'MultiFDCompression',
-            '*multifd-zlib-level': 'int',
-            '*multifd-zstd-level': 'int',
+            '*multifd-zlib-level': 'uint8',
+            '*multifd-zstd-level': 'uint8',
             '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
 
 ##
@@ -1093,7 +1093,7 @@
             '*max-bandwidth': 'size',
             '*downtime-limit': 'uint64',
             '*x-checkpoint-delay': 'uint32',
-            '*block-incremental': 'bool' ,
+            '*block-incremental': 'bool',
             '*multifd-channels': 'uint8',
             '*xbzrle-cache-size': 'size',
             '*max-postcopy-bandwidth': 'size',
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 492789248f..f8ef061510 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1292,11 +1292,11 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
     switch (val) {
     case MIGRATION_PARAMETER_COMPRESS_LEVEL:
         p->has_compress_level = true;
-        visit_type_int(v, param, &p->compress_level, &err);
+        visit_type_uint8(v, param, &p->compress_level, &err);
         break;
     case MIGRATION_PARAMETER_COMPRESS_THREADS:
         p->has_compress_threads = true;
-        visit_type_int(v, param, &p->compress_threads, &err);
+        visit_type_uint8(v, param, &p->compress_threads, &err);
         break;
     case MIGRATION_PARAMETER_COMPRESS_WAIT_THREAD:
         p->has_compress_wait_thread = true;
@@ -1304,19 +1304,19 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
         break;
     case MIGRATION_PARAMETER_DECOMPRESS_THREADS:
         p->has_decompress_threads = true;
-        visit_type_int(v, param, &p->decompress_threads, &err);
+        visit_type_uint8(v, param, &p->decompress_threads, &err);
         break;
     case MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD:
         p->has_throttle_trigger_threshold = true;
-        visit_type_int(v, param, &p->throttle_trigger_threshold, &err);
+        visit_type_uint8(v, param, &p->throttle_trigger_threshold, &err);
         break;
     case MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL:
         p->has_cpu_throttle_initial = true;
-        visit_type_int(v, param, &p->cpu_throttle_initial, &err);
+        visit_type_uint8(v, param, &p->cpu_throttle_initial, &err);
         break;
     case MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT:
         p->has_cpu_throttle_increment = true;
-        visit_type_int(v, param, &p->cpu_throttle_increment, &err);
+        visit_type_uint8(v, param, &p->cpu_throttle_increment, &err);
         break;
     case MIGRATION_PARAMETER_CPU_THROTTLE_TAILSLOW:
         p->has_cpu_throttle_tailslow = true;
@@ -1324,7 +1324,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
         break;
     case MIGRATION_PARAMETER_MAX_CPU_THROTTLE:
         p->has_max_cpu_throttle = true;
-        visit_type_int(v, param, &p->max_cpu_throttle, &err);
+        visit_type_uint8(v, param, &p->max_cpu_throttle, &err);
         break;
     case MIGRATION_PARAMETER_TLS_CREDS:
         p->has_tls_creds = true;
@@ -1360,11 +1360,11 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
         break;
     case MIGRATION_PARAMETER_DOWNTIME_LIMIT:
         p->has_downtime_limit = true;
-        visit_type_int(v, param, &p->downtime_limit, &err);
+        visit_type_size(v, param, &p->downtime_limit, &err);
         break;
     case MIGRATION_PARAMETER_X_CHECKPOINT_DELAY:
         p->has_x_checkpoint_delay = true;
-        visit_type_int(v, param, &p->x_checkpoint_delay, &err);
+        visit_type_uint32(v, param, &p->x_checkpoint_delay, &err);
         break;
     case MIGRATION_PARAMETER_BLOCK_INCREMENTAL:
         p->has_block_incremental = true;
@@ -1372,7 +1372,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
         break;
     case MIGRATION_PARAMETER_MULTIFD_CHANNELS:
         p->has_multifd_channels = true;
-        visit_type_int(v, param, &p->multifd_channels, &err);
+        visit_type_uint8(v, param, &p->multifd_channels, &err);
         break;
     case MIGRATION_PARAMETER_MULTIFD_COMPRESSION:
         p->has_multifd_compression = true;
@@ -1381,11 +1381,11 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
         break;
     case MIGRATION_PARAMETER_MULTIFD_ZLIB_LEVEL:
         p->has_multifd_zlib_level = true;
-        visit_type_int(v, param, &p->multifd_zlib_level, &err);
+        visit_type_uint8(v, param, &p->multifd_zlib_level, &err);
         break;
     case MIGRATION_PARAMETER_MULTIFD_ZSTD_LEVEL:
         p->has_multifd_zstd_level = true;
-        visit_type_int(v, param, &p->multifd_zstd_level, &err);
+        visit_type_uint8(v, param, &p->multifd_zstd_level, &err);
         break;
     case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE:
         p->has_xbzrle_cache_size = true;
-- 
2.26.2

Re: [PATCH 2/6] migration: Fix migrate-set-parameters argument validation

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Commit 741d4086c8 "migration: Use proper types in json" (v2.12.0)
> switched MigrationParameters to narrower integer types, and removed
> the simplified qmp_migrate_set_parameters()'s argument checking
> accordingly.
> 
> Good idea, except qmp_migrate_set_parameters() takes
> MigrateSetParameters, not MigrationParameters.  Its job is updating
> migrate_get_current()->parameters (which *is* of type
> MigrationParameters) according to its argument.  The integers now get
> truncated silently.  Reproducer:
> 
>     ---> {'execute': 'query-migrate-parameters'}
>     <--- {"return": {[...] "compress-threads": 8, [...]}}
>     ---> {"execute": "migrate-set-parameters", "arguments": {"compress-threads": 257}}
>     <--- {"return": {}}
>     ---> {'execute': 'query-migrate-parameters'}
>     <--- {"return": {[...] "compress-threads": 1, [...]}}
> 
> Fix by resynchronizing MigrateSetParameters with MigrationParameters.

Having those two separate types is a pain!

> Fixes: 741d4086c856320807a2575389d7c0505578270b
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  qapi/migration.json | 28 ++++++++++++++--------------
>  monitor/hmp-cmds.c  | 24 ++++++++++++------------
>  2 files changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 688e8da749..3ad3720cf0 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -885,28 +885,28 @@
>              '*announce-max': 'size',
>              '*announce-rounds': 'size',
>              '*announce-step': 'size',
> -            '*compress-level': 'int',
> -            '*compress-threads': 'int',
> +            '*compress-level': 'uint8',
> +            '*compress-threads': 'uint8',
>              '*compress-wait-thread': 'bool',
> -            '*decompress-threads': 'int',
> -            '*throttle-trigger-threshold': 'int',
> -            '*cpu-throttle-initial': 'int',
> -            '*cpu-throttle-increment': 'int',
> +            '*decompress-threads': 'uint8',
> +            '*throttle-trigger-threshold': 'uint8',
> +            '*cpu-throttle-initial': 'uint8',
> +            '*cpu-throttle-increment': 'uint8',
>              '*cpu-throttle-tailslow': 'bool',
>              '*tls-creds': 'StrOrNull',
>              '*tls-hostname': 'StrOrNull',
>              '*tls-authz': 'StrOrNull',
> -            '*max-bandwidth': 'int',
> -            '*downtime-limit': 'int',
> -            '*x-checkpoint-delay': 'int',
> +            '*max-bandwidth': 'size',
> +            '*downtime-limit': 'uint64',
> +            '*x-checkpoint-delay': 'uint32',
>              '*block-incremental': 'bool',
> -            '*multifd-channels': 'int',
> +            '*multifd-channels': 'uint8',
>              '*xbzrle-cache-size': 'size',
>              '*max-postcopy-bandwidth': 'size',
> -            '*max-cpu-throttle': 'int',
> +            '*max-cpu-throttle': 'uint8',
>              '*multifd-compression': 'MultiFDCompression',
> -            '*multifd-zlib-level': 'int',
> -            '*multifd-zstd-level': 'int',
> +            '*multifd-zlib-level': 'uint8',
> +            '*multifd-zstd-level': 'uint8',
>              '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
>  
>  ##
> @@ -1093,7 +1093,7 @@
>              '*max-bandwidth': 'size',
>              '*downtime-limit': 'uint64',
>              '*x-checkpoint-delay': 'uint32',
> -            '*block-incremental': 'bool' ,
> +            '*block-incremental': 'bool',
>              '*multifd-channels': 'uint8',
>              '*xbzrle-cache-size': 'size',
>              '*max-postcopy-bandwidth': 'size',
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 492789248f..f8ef061510 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1292,11 +1292,11 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>      switch (val) {
>      case MIGRATION_PARAMETER_COMPRESS_LEVEL:
>          p->has_compress_level = true;
> -        visit_type_int(v, param, &p->compress_level, &err);
> +        visit_type_uint8(v, param, &p->compress_level, &err);
>          break;
>      case MIGRATION_PARAMETER_COMPRESS_THREADS:
>          p->has_compress_threads = true;
> -        visit_type_int(v, param, &p->compress_threads, &err);
> +        visit_type_uint8(v, param, &p->compress_threads, &err);
>          break;
>      case MIGRATION_PARAMETER_COMPRESS_WAIT_THREAD:
>          p->has_compress_wait_thread = true;
> @@ -1304,19 +1304,19 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>          break;
>      case MIGRATION_PARAMETER_DECOMPRESS_THREADS:
>          p->has_decompress_threads = true;
> -        visit_type_int(v, param, &p->decompress_threads, &err);
> +        visit_type_uint8(v, param, &p->decompress_threads, &err);
>          break;
>      case MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD:
>          p->has_throttle_trigger_threshold = true;
> -        visit_type_int(v, param, &p->throttle_trigger_threshold, &err);
> +        visit_type_uint8(v, param, &p->throttle_trigger_threshold, &err);
>          break;
>      case MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL:
>          p->has_cpu_throttle_initial = true;
> -        visit_type_int(v, param, &p->cpu_throttle_initial, &err);
> +        visit_type_uint8(v, param, &p->cpu_throttle_initial, &err);
>          break;
>      case MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT:
>          p->has_cpu_throttle_increment = true;
> -        visit_type_int(v, param, &p->cpu_throttle_increment, &err);
> +        visit_type_uint8(v, param, &p->cpu_throttle_increment, &err);
>          break;
>      case MIGRATION_PARAMETER_CPU_THROTTLE_TAILSLOW:
>          p->has_cpu_throttle_tailslow = true;
> @@ -1324,7 +1324,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>          break;
>      case MIGRATION_PARAMETER_MAX_CPU_THROTTLE:
>          p->has_max_cpu_throttle = true;
> -        visit_type_int(v, param, &p->max_cpu_throttle, &err);
> +        visit_type_uint8(v, param, &p->max_cpu_throttle, &err);
>          break;
>      case MIGRATION_PARAMETER_TLS_CREDS:
>          p->has_tls_creds = true;
> @@ -1360,11 +1360,11 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>          break;
>      case MIGRATION_PARAMETER_DOWNTIME_LIMIT:
>          p->has_downtime_limit = true;
> -        visit_type_int(v, param, &p->downtime_limit, &err);
> +        visit_type_size(v, param, &p->downtime_limit, &err);
>          break;
>      case MIGRATION_PARAMETER_X_CHECKPOINT_DELAY:
>          p->has_x_checkpoint_delay = true;
> -        visit_type_int(v, param, &p->x_checkpoint_delay, &err);
> +        visit_type_uint32(v, param, &p->x_checkpoint_delay, &err);
>          break;
>      case MIGRATION_PARAMETER_BLOCK_INCREMENTAL:
>          p->has_block_incremental = true;
> @@ -1372,7 +1372,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>          break;
>      case MIGRATION_PARAMETER_MULTIFD_CHANNELS:
>          p->has_multifd_channels = true;
> -        visit_type_int(v, param, &p->multifd_channels, &err);
> +        visit_type_uint8(v, param, &p->multifd_channels, &err);
>          break;
>      case MIGRATION_PARAMETER_MULTIFD_COMPRESSION:
>          p->has_multifd_compression = true;
> @@ -1381,11 +1381,11 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>          break;
>      case MIGRATION_PARAMETER_MULTIFD_ZLIB_LEVEL:
>          p->has_multifd_zlib_level = true;
> -        visit_type_int(v, param, &p->multifd_zlib_level, &err);
> +        visit_type_uint8(v, param, &p->multifd_zlib_level, &err);
>          break;
>      case MIGRATION_PARAMETER_MULTIFD_ZSTD_LEVEL:
>          p->has_multifd_zstd_level = true;
> -        visit_type_int(v, param, &p->multifd_zstd_level, &err);
> +        visit_type_uint8(v, param, &p->multifd_zstd_level, &err);
>          break;
>      case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE:
>          p->has_xbzrle_cache_size = true;
> -- 
> 2.26.2
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 2/6] migration: Fix migrate-set-parameters argument validation

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> Commit 741d4086c8 "migration: Use proper types in json" (v2.12.0)
>> switched MigrationParameters to narrower integer types, and removed
>> the simplified qmp_migrate_set_parameters()'s argument checking
>> accordingly.
>> 
>> Good idea, except qmp_migrate_set_parameters() takes
>> MigrateSetParameters, not MigrationParameters.  Its job is updating
>> migrate_get_current()->parameters (which *is* of type
>> MigrationParameters) according to its argument.  The integers now get
>> truncated silently.  Reproducer:
>> 
>>     ---> {'execute': 'query-migrate-parameters'}
>>     <--- {"return": {[...] "compress-threads": 8, [...]}}
>>     ---> {"execute": "migrate-set-parameters", "arguments": {"compress-threads": 257}}
>>     <--- {"return": {}}
>>     ---> {'execute': 'query-migrate-parameters'}
>>     <--- {"return": {[...] "compress-threads": 1, [...]}}
>> 
>> Fix by resynchronizing MigrateSetParameters with MigrationParameters.
>
> Having those two separate types is a pain!

It is!

MigrateSetParameters is the argument of migrate-set-parameters,
MigrationParameters is the result of query-migrate-parameters and part
of the internal migration state.

Differences:

(1) Optional members

    For migrate-set-parameters, we need *all* members to be optional.

    For migration state and query-migrate-parameters, we want only some
    members to be optional (currently only @tls-authz, I think).

(2) Special values

    migrate-set-parameters has a "reset to default, whatever that may
    be" feature for some members (currently only @tls-creds,
    @tls-hostname, @tls-authz, I think).  Doing that cleanly requires an
    additonal value.

The first attempt to fuse the two types (commit de63ab6124 "migrate:
Share common MigrationParameters struct", 2016-10-13) took care of (1).
Introspection of query-migrate-parameters became mildly misleading (it
claims members are optional that aren't), and C code dealing with
migration state had to take care to set the has_FOO = true.  Tolerable.

I had to revert it to address (2) cleanly and in time: commit 01fa559826
"migration: Use JSON null instead of "" to reset parameter to default",
2017-07-24.

A second try needs to take care of (2) as well.  Messes up
query-migrate-parameters some more: introspection claims a few members
can be null that can't.

Is the "reset" feature is worth all that trouble?  Is overloading
migrate-set-parameters a good idea?

>> Fixes: 741d4086c856320807a2575389d7c0505578270b
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Thanks!

[PATCH 3/6] migration: Clean up signed vs. unsigned XBZRLE cache-size

[Markus Armbruster]

73af8dd8d7 "migration: Make xbzrle_cache_size a migration
parameter" (v2.11.0) made the new parameter unsigned (QAPI type
'size', uint64_t in C).  It neglected to update existing code, which
continues to use int64_t.

migrate_xbzrle_cache_size() returns the new parameter.  Adjust its
return type.

QMP query-migrate-cache-size returns migrate_xbzrle_cache_size().
Adjust its return type.

migrate-set-parameters passes the new parameter to
xbzrle_cache_resize().  Adjust its parameter type.

xbzrle_cache_resize() passes it on to cache_init().  Adjust its
parameter type.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qapi/migration.json    | 4 ++--
 migration/migration.h  | 2 +-
 migration/page_cache.h | 2 +-
 migration/ram.h        | 2 +-
 migration/migration.c  | 4 ++--
 migration/page_cache.c | 2 +-
 migration/ram.c        | 2 +-
 7 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index 3ad3720cf0..e8a4dfecce 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -78,7 +78,7 @@
 # Since: 1.2
 ##
 { 'struct': 'XBZRLECacheStats',
-  'data': {'cache-size': 'int', 'bytes': 'int', 'pages': 'int',
+  'data': {'cache-size': 'size', 'bytes': 'int', 'pages': 'int',
            'cache-miss': 'int', 'cache-miss-rate': 'number',
            'encoding-rate': 'number', 'overflow': 'int' } }
 
@@ -1465,7 +1465,7 @@
 # <- { "return": 67108864 }
 #
 ##
-{ 'command': 'query-migrate-cache-size', 'returns': 'int',
+{ 'command': 'query-migrate-cache-size', 'returns': 'size',
   'features': [ 'deprecated' ] }
 
 ##
diff --git a/migration/migration.h b/migration/migration.h
index d096b77f74..0387dc40d6 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -324,7 +324,7 @@ int migrate_multifd_zlib_level(void);
 int migrate_multifd_zstd_level(void);
 
 int migrate_use_xbzrle(void);
-int64_t migrate_xbzrle_cache_size(void);
+uint64_t migrate_xbzrle_cache_size(void);
 bool migrate_colo_enabled(void);
 
 bool migrate_use_block(void);
diff --git a/migration/page_cache.h b/migration/page_cache.h
index 0cb94498a0..8733b4df6e 100644
--- a/migration/page_cache.h
+++ b/migration/page_cache.h
@@ -28,7 +28,7 @@ typedef struct PageCache PageCache;
  * @page_size: cache page size
  * @errp: set *errp if the check failed, with reason
  */
-PageCache *cache_init(int64_t cache_size, size_t page_size, Error **errp);
+PageCache *cache_init(uint64_t cache_size, size_t page_size, Error **errp);
 /**
  * cache_fini: free all cache resources
  * @cache pointer to the PageCache struct
diff --git a/migration/ram.h b/migration/ram.h
index 011e85414e..016eaa3378 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -47,7 +47,7 @@ bool ramblock_is_ignored(RAMBlock *block);
     INTERNAL_RAMBLOCK_FOREACH(block)                   \
         if (!qemu_ram_is_migratable(block)) {} else
 
-int xbzrle_cache_resize(int64_t new_size, Error **errp);
+int xbzrle_cache_resize(uint64_t new_size, Error **errp);
 uint64_t ram_bytes_remaining(void);
 uint64_t ram_bytes_total(void);
 
diff --git a/migration/migration.c b/migration/migration.c
index cad56fbf8c..3daed2da0e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2226,7 +2226,7 @@ void qmp_migrate_set_cache_size(int64_t value, Error **errp)
     qmp_migrate_set_parameters(&p, errp);
 }
 
-int64_t qmp_query_migrate_cache_size(Error **errp)
+uint64_t qmp_query_migrate_cache_size(Error **errp)
 {
     return migrate_xbzrle_cache_size();
 }
@@ -2456,7 +2456,7 @@ int migrate_use_xbzrle(void)
     return s->enabled_capabilities[MIGRATION_CAPABILITY_XBZRLE];
 }
 
-int64_t migrate_xbzrle_cache_size(void)
+uint64_t migrate_xbzrle_cache_size(void)
 {
     MigrationState *s;
 
diff --git a/migration/page_cache.c b/migration/page_cache.c
index 098b436223..b384f265fb 100644
--- a/migration/page_cache.c
+++ b/migration/page_cache.c
@@ -38,7 +38,7 @@ struct PageCache {
     size_t num_items;
 };
 
-PageCache *cache_init(int64_t new_size, size_t page_size, Error **errp)
+PageCache *cache_init(uint64_t new_size, size_t page_size, Error **errp)
 {
     int64_t i;
     size_t num_pages = new_size / page_size;
diff --git a/migration/ram.c b/migration/ram.c
index add5396a62..a84425d04f 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -126,7 +126,7 @@ static void XBZRLE_cache_unlock(void)
  * @new_size: new cache size
  * @errp: set *errp if the check failed, with reason
  */
-int xbzrle_cache_resize(int64_t new_size, Error **errp)
+int xbzrle_cache_resize(uint64_t new_size, Error **errp)
 {
     PageCache *new_cache;
     int64_t ret = 0;
-- 
2.26.2

Re: [PATCH 3/6] migration: Clean up signed vs. unsigned XBZRLE cache-size

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> 73af8dd8d7 "migration: Make xbzrle_cache_size a migration
> parameter" (v2.11.0) made the new parameter unsigned (QAPI type
> 'size', uint64_t in C).  It neglected to update existing code, which
> continues to use int64_t.
> 
> migrate_xbzrle_cache_size() returns the new parameter.  Adjust its
> return type.
> 
> QMP query-migrate-cache-size returns migrate_xbzrle_cache_size().
> Adjust its return type.
> 
> migrate-set-parameters passes the new parameter to
> xbzrle_cache_resize().  Adjust its parameter type.
> 
> xbzrle_cache_resize() passes it on to cache_init().  Adjust its
> parameter type.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  qapi/migration.json    | 4 ++--
>  migration/migration.h  | 2 +-
>  migration/page_cache.h | 2 +-
>  migration/ram.h        | 2 +-
>  migration/migration.c  | 4 ++--
>  migration/page_cache.c | 2 +-
>  migration/ram.c        | 2 +-
>  7 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 3ad3720cf0..e8a4dfecce 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -78,7 +78,7 @@
>  # Since: 1.2
>  ##
>  { 'struct': 'XBZRLECacheStats',
> -  'data': {'cache-size': 'int', 'bytes': 'int', 'pages': 'int',
> +  'data': {'cache-size': 'size', 'bytes': 'int', 'pages': 'int',
>             'cache-miss': 'int', 'cache-miss-rate': 'number',
>             'encoding-rate': 'number', 'overflow': 'int' } }
>  
> @@ -1465,7 +1465,7 @@
>  # <- { "return": 67108864 }
>  #
>  ##
> -{ 'command': 'query-migrate-cache-size', 'returns': 'int',
> +{ 'command': 'query-migrate-cache-size', 'returns': 'size',
>    'features': [ 'deprecated' ] }
>  
>  ##
> diff --git a/migration/migration.h b/migration/migration.h
> index d096b77f74..0387dc40d6 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -324,7 +324,7 @@ int migrate_multifd_zlib_level(void);
>  int migrate_multifd_zstd_level(void);
>  
>  int migrate_use_xbzrle(void);
> -int64_t migrate_xbzrle_cache_size(void);
> +uint64_t migrate_xbzrle_cache_size(void);
>  bool migrate_colo_enabled(void);
>  
>  bool migrate_use_block(void);
> diff --git a/migration/page_cache.h b/migration/page_cache.h
> index 0cb94498a0..8733b4df6e 100644
> --- a/migration/page_cache.h
> +++ b/migration/page_cache.h
> @@ -28,7 +28,7 @@ typedef struct PageCache PageCache;
>   * @page_size: cache page size
>   * @errp: set *errp if the check failed, with reason
>   */
> -PageCache *cache_init(int64_t cache_size, size_t page_size, Error **errp);
> +PageCache *cache_init(uint64_t cache_size, size_t page_size, Error **errp);
>  /**
>   * cache_fini: free all cache resources
>   * @cache pointer to the PageCache struct
> diff --git a/migration/ram.h b/migration/ram.h
> index 011e85414e..016eaa3378 100644
> --- a/migration/ram.h
> +++ b/migration/ram.h
> @@ -47,7 +47,7 @@ bool ramblock_is_ignored(RAMBlock *block);
>      INTERNAL_RAMBLOCK_FOREACH(block)                   \
>          if (!qemu_ram_is_migratable(block)) {} else
>  
> -int xbzrle_cache_resize(int64_t new_size, Error **errp);
> +int xbzrle_cache_resize(uint64_t new_size, Error **errp);
>  uint64_t ram_bytes_remaining(void);
>  uint64_t ram_bytes_total(void);
>  
> diff --git a/migration/migration.c b/migration/migration.c
> index cad56fbf8c..3daed2da0e 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2226,7 +2226,7 @@ void qmp_migrate_set_cache_size(int64_t value, Error **errp)
>      qmp_migrate_set_parameters(&p, errp);
>  }
>  
> -int64_t qmp_query_migrate_cache_size(Error **errp)
> +uint64_t qmp_query_migrate_cache_size(Error **errp)
>  {
>      return migrate_xbzrle_cache_size();
>  }
> @@ -2456,7 +2456,7 @@ int migrate_use_xbzrle(void)
>      return s->enabled_capabilities[MIGRATION_CAPABILITY_XBZRLE];
>  }
>  
> -int64_t migrate_xbzrle_cache_size(void)
> +uint64_t migrate_xbzrle_cache_size(void)
>  {
>      MigrationState *s;
>  
> diff --git a/migration/page_cache.c b/migration/page_cache.c
> index 098b436223..b384f265fb 100644
> --- a/migration/page_cache.c
> +++ b/migration/page_cache.c
> @@ -38,7 +38,7 @@ struct PageCache {
>      size_t num_items;
>  };
>  
> -PageCache *cache_init(int64_t new_size, size_t page_size, Error **errp)
> +PageCache *cache_init(uint64_t new_size, size_t page_size, Error **errp)
>  {
>      int64_t i;
>      size_t num_pages = new_size / page_size;
> diff --git a/migration/ram.c b/migration/ram.c
> index add5396a62..a84425d04f 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -126,7 +126,7 @@ static void XBZRLE_cache_unlock(void)
>   * @new_size: new cache size
>   * @errp: set *errp if the check failed, with reason
>   */
> -int xbzrle_cache_resize(int64_t new_size, Error **errp)
> +int xbzrle_cache_resize(uint64_t new_size, Error **errp)
>  {
>      PageCache *new_cache;
>      int64_t ret = 0;
> -- 
> 2.26.2
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH 4/6] migration: Check xbzrle-cache-size more carefully

[Markus Armbruster]

migrate-set-parameters passes the size to xbzrle_cache_resize().
xbzrle_cache_resize() checks it fits into size_t before it passes it
on to cache_init().  cache_init() checks it is a power of two no
smaller than @page_size.

cache_init() is also called from xbzrle_init(), bypassing
xbzrle_cache_resize()'s check.

Drop xbzrle_cache_resize()'s check, and check more carefully in
cache_init().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 migration/page_cache.c | 15 ++++-----------
 migration/ram.c        |  7 -------
 2 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/migration/page_cache.c b/migration/page_cache.c
index b384f265fb..e07f4ad1dc 100644
--- a/migration/page_cache.c
+++ b/migration/page_cache.c
@@ -41,17 +41,10 @@ struct PageCache {
 PageCache *cache_init(uint64_t new_size, size_t page_size, Error **errp)
 {
     int64_t i;
-    size_t num_pages = new_size / page_size;
+    uint64_t num_pages = new_size / page_size;
     PageCache *cache;
 
-    if (new_size < page_size) {
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
-                   "is smaller than one target page size");
-        return NULL;
-    }
-
-    /* round down to the nearest power of 2 */
-    if (!is_power_of_2(num_pages)) {
+    if (num_pages != (size_t)num_pages || !is_power_of_2(num_pages)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
                    "is not a power of two number of pages");
         return NULL;
@@ -71,8 +64,8 @@ PageCache *cache_init(uint64_t new_size, size_t page_size, Error **errp)
     trace_migration_pagecache_init(cache->max_num_items);
 
     /* We prefer not to abort if there is no memory */
-    cache->page_cache = g_try_malloc((cache->max_num_items) *
-                                     sizeof(*cache->page_cache));
+    cache->page_cache = g_try_malloc_n(cache->max_num_items,
+                                       sizeof(*cache->page_cache));
     if (!cache->page_cache) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
                    "Failed to allocate page cache");
diff --git a/migration/ram.c b/migration/ram.c
index a84425d04f..d632ae694c 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -131,13 +131,6 @@ int xbzrle_cache_resize(uint64_t new_size, Error **errp)
     PageCache *new_cache;
     int64_t ret = 0;
 
-    /* Check for truncation */
-    if (new_size != (size_t)new_size) {
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
-                   "exceeding address space");
-        return -1;
-    }
-
     if (new_size == migrate_xbzrle_cache_size()) {
         /* nothing to do */
         return 0;
-- 
2.26.2

Re: [PATCH 4/6] migration: Check xbzrle-cache-size more carefully

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> migrate-set-parameters passes the size to xbzrle_cache_resize().
> xbzrle_cache_resize() checks it fits into size_t before it passes it
> on to cache_init().  cache_init() checks it is a power of two no
> smaller than @page_size.
> 
> cache_init() is also called from xbzrle_init(), bypassing
> xbzrle_cache_resize()'s check.
> 
> Drop xbzrle_cache_resize()'s check, and check more carefully in
> cache_init().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  migration/page_cache.c | 15 ++++-----------
>  migration/ram.c        |  7 -------
>  2 files changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/migration/page_cache.c b/migration/page_cache.c
> index b384f265fb..e07f4ad1dc 100644
> --- a/migration/page_cache.c
> +++ b/migration/page_cache.c
> @@ -41,17 +41,10 @@ struct PageCache {
>  PageCache *cache_init(uint64_t new_size, size_t page_size, Error **errp)
>  {
>      int64_t i;
> -    size_t num_pages = new_size / page_size;
> +    uint64_t num_pages = new_size / page_size;
>      PageCache *cache;
>  
> -    if (new_size < page_size) {
> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> -                   "is smaller than one target page size");
> -        return NULL;
> -    }
> -
> -    /* round down to the nearest power of 2 */
> -    if (!is_power_of_2(num_pages)) {
> +    if (num_pages != (size_t)num_pages || !is_power_of_2(num_pages)) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
>                     "is not a power of two number of pages");

That error message is now wrong since it includes a whole bunch of
reasons.
Also, the comparison is now on the divided num_pages, it's not that
obvious to me that checking the num_pages makes sense in acomparison to
checking the actual cache size.

(Arguably the check should also happen in migrate_params_test_apply)

Dave

>          return NULL;
> @@ -71,8 +64,8 @@ PageCache *cache_init(uint64_t new_size, size_t page_size, Error **errp)
>      trace_migration_pagecache_init(cache->max_num_items);
>  
>      /* We prefer not to abort if there is no memory */
> -    cache->page_cache = g_try_malloc((cache->max_num_items) *
> -                                     sizeof(*cache->page_cache));
> +    cache->page_cache = g_try_malloc_n(cache->max_num_items,
> +                                       sizeof(*cache->page_cache));
>      if (!cache->page_cache) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
>                     "Failed to allocate page cache");
> diff --git a/migration/ram.c b/migration/ram.c
> index a84425d04f..d632ae694c 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -131,13 +131,6 @@ int xbzrle_cache_resize(uint64_t new_size, Error **errp)
>      PageCache *new_cache;
>      int64_t ret = 0;
>  
> -    /* Check for truncation */
> -    if (new_size != (size_t)new_size) {
> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> -                   "exceeding address space");
> -        return -1;
> -    }
> -
>      if (new_size == migrate_xbzrle_cache_size()) {
>          /* nothing to do */
>          return 0;
> -- 
> 2.26.2
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 4/6] migration: Check xbzrle-cache-size more carefully

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> migrate-set-parameters passes the size to xbzrle_cache_resize().
>> xbzrle_cache_resize() checks it fits into size_t before it passes it
>> on to cache_init().  cache_init() checks it is a power of two no
>> smaller than @page_size.
>> 
>> cache_init() is also called from xbzrle_init(), bypassing
>> xbzrle_cache_resize()'s check.
>> 
>> Drop xbzrle_cache_resize()'s check, and check more carefully in
>> cache_init().
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  migration/page_cache.c | 15 ++++-----------
>>  migration/ram.c        |  7 -------
>>  2 files changed, 4 insertions(+), 18 deletions(-)
>> 
>> diff --git a/migration/page_cache.c b/migration/page_cache.c
>> index b384f265fb..e07f4ad1dc 100644
>> --- a/migration/page_cache.c
>> +++ b/migration/page_cache.c
>> @@ -41,17 +41,10 @@ struct PageCache {
>>  PageCache *cache_init(uint64_t new_size, size_t page_size, Error **errp)
>>  {
>>      int64_t i;
>> -    size_t num_pages = new_size / page_size;
>> +    uint64_t num_pages = new_size / page_size;
>>      PageCache *cache;
>>  
>> -    if (new_size < page_size) {
>> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
>> -                   "is smaller than one target page size");
>> -        return NULL;
>> -    }
>> -
>> -    /* round down to the nearest power of 2 */
>> -    if (!is_power_of_2(num_pages)) {
>> +    if (num_pages != (size_t)num_pages || !is_power_of_2(num_pages)) {
>>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
>>                     "is not a power of two number of pages");
>
> That error message is now wrong since it includes a whole bunch of
> reasons.

We could argue about "wrong", but I readily concedede it needs
improvement:

    Parameter 'cache size' expects is not a power of two number of pages

is crap.  I fixed similar crap in my "[PATCH 00/10] Chipping away at
qerror.h", but missed this one.

What about

    Parameter 'xbzrle-cache-size' expects a power of two larger than $page_size

?

> Also, the comparison is now on the divided num_pages, it's not that
> obvious to me that checking the num_pages makes sense in acomparison to
> checking the actual cache size.

Would you accept

    if (!is_power_of_2(new_size)
        || !num_pages || num_pages != (size_t)num_pages) {

?

If not, please propose something you like better.

> (Arguably the check should also happen in migrate_params_test_apply)

Feels like one bridge too far for this patch.

Re: [PATCH 4/6] migration: Check xbzrle-cache-size more carefully

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> migrate-set-parameters passes the size to xbzrle_cache_resize().
> >> xbzrle_cache_resize() checks it fits into size_t before it passes it
> >> on to cache_init().  cache_init() checks it is a power of two no
> >> smaller than @page_size.
> >> 
> >> cache_init() is also called from xbzrle_init(), bypassing
> >> xbzrle_cache_resize()'s check.
> >> 
> >> Drop xbzrle_cache_resize()'s check, and check more carefully in
> >> cache_init().
> >> 
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  migration/page_cache.c | 15 ++++-----------
> >>  migration/ram.c        |  7 -------
> >>  2 files changed, 4 insertions(+), 18 deletions(-)
> >> 
> >> diff --git a/migration/page_cache.c b/migration/page_cache.c
> >> index b384f265fb..e07f4ad1dc 100644
> >> --- a/migration/page_cache.c
> >> +++ b/migration/page_cache.c
> >> @@ -41,17 +41,10 @@ struct PageCache {
> >>  PageCache *cache_init(uint64_t new_size, size_t page_size, Error **errp)
> >>  {
> >>      int64_t i;
> >> -    size_t num_pages = new_size / page_size;
> >> +    uint64_t num_pages = new_size / page_size;
> >>      PageCache *cache;
> >>  
> >> -    if (new_size < page_size) {
> >> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> >> -                   "is smaller than one target page size");
> >> -        return NULL;
> >> -    }
> >> -
> >> -    /* round down to the nearest power of 2 */
> >> -    if (!is_power_of_2(num_pages)) {
> >> +    if (num_pages != (size_t)num_pages || !is_power_of_2(num_pages)) {
> >>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> >>                     "is not a power of two number of pages");
> >
> > That error message is now wrong since it includes a whole bunch of
> > reasons.
> 
> We could argue about "wrong", but I readily concedede it needs
> improvement:
> 
>     Parameter 'cache size' expects is not a power of two number of pages
> 
> is crap.  I fixed similar crap in my "[PATCH 00/10] Chipping away at
> qerror.h"

The wording may be crap, but it does at least talk about the correct
problem.

> but missed this one.
> 
> What about
> 
>     Parameter 'xbzrle-cache-size' expects a power of two larger than $page_size

Yes, although you've also put a too-large check in ther=e with that
size_t cast.

> ?
> 
> > Also, the comparison is now on the divided num_pages, it's not that
> > obvious to me that checking the num_pages makes sense in acomparison to
> > checking the actual cache size.
> 
> Would you accept
> 
>     if (!is_power_of_2(new_size)
>         || !num_pages || num_pages != (size_t)num_pages) {

Well, why is it not  || new_size != (size_t)new_size   like in the
original?

> ?
> 
> If not, please propose something you like better.
> 
> > (Arguably the check should also happen in migrate_params_test_apply)
> 
> Feels like one bridge too far for this patch.

Sure.

Dave

-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 4/6] migration: Check xbzrle-cache-size more carefully

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>> 
>> > * Markus Armbruster (armbru@redhat.com) wrote:
>> >> migrate-set-parameters passes the size to xbzrle_cache_resize().
>> >> xbzrle_cache_resize() checks it fits into size_t before it passes it
>> >> on to cache_init().  cache_init() checks it is a power of two no
>> >> smaller than @page_size.
>> >> 
>> >> cache_init() is also called from xbzrle_init(), bypassing
>> >> xbzrle_cache_resize()'s check.
>> >> 
>> >> Drop xbzrle_cache_resize()'s check, and check more carefully in
>> >> cache_init().
>> >> 
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> ---
>> >>  migration/page_cache.c | 15 ++++-----------
>> >>  migration/ram.c        |  7 -------
>> >>  2 files changed, 4 insertions(+), 18 deletions(-)
>> >> 
>> >> diff --git a/migration/page_cache.c b/migration/page_cache.c
>> >> index b384f265fb..e07f4ad1dc 100644
>> >> --- a/migration/page_cache.c
>> >> +++ b/migration/page_cache.c
>> >> @@ -41,17 +41,10 @@ struct PageCache {
>> >>  PageCache *cache_init(uint64_t new_size, size_t page_size, Error **errp)
>> >>  {
>> >>      int64_t i;
>> >> -    size_t num_pages = new_size / page_size;
>> >> +    uint64_t num_pages = new_size / page_size;
>> >>      PageCache *cache;
>> >>  
>> >> -    if (new_size < page_size) {
>> >> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
>> >> -                   "is smaller than one target page size");
>> >> -        return NULL;
>> >> -    }
>> >> -
>> >> -    /* round down to the nearest power of 2 */
>> >> -    if (!is_power_of_2(num_pages)) {
>> >> +    if (num_pages != (size_t)num_pages || !is_power_of_2(num_pages)) {
>> >>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
>> >>                     "is not a power of two number of pages");
>> >
>> > That error message is now wrong since it includes a whole bunch of
>> > reasons.
>> 
>> We could argue about "wrong", but I readily concedede it needs
>> improvement:
>> 
>>     Parameter 'cache size' expects is not a power of two number of pages
>> 
>> is crap.  I fixed similar crap in my "[PATCH 00/10] Chipping away at
>> qerror.h"
>
> The wording may be crap, but it does at least talk about the correct
> problem.
>
>> but missed this one.
>> 
>> What about
>> 
>>     Parameter 'xbzrle-cache-size' expects a power of two larger than $page_size
>
> Yes, although you've also put a too-large check in ther=e with that
> size_t cast.

I'm not sure I understand what you want.

Personally, I hate it when a computer tells me "your value doesn't
satisfy X", and when I adjust my value, it tells me "now it does, but it
also has to satify Y, so there!"  Compute, just tell me straight what
you want!

I hasten to add there are exceptions, say when "X" or "Y" are so
complicated that "X and Y" becomes bad UI.

That said, this is just a small drive-by cleanup.  I'm happy to adjust
it to your liking, I'm happy to drop it, just tell me what you want :)

>> ?
>> 
>> > Also, the comparison is now on the divided num_pages, it's not that
>> > obvious to me that checking the num_pages makes sense in acomparison to
>> > checking the actual cache size.
>> 
>> Would you accept
>> 
>>     if (!is_power_of_2(new_size)
>>         || !num_pages || num_pages != (size_t)num_pages) {
>
> Well, why is it not  || new_size != (size_t)new_size   like in the
> original?

Because nothing in this function actually depends on new_size fitting
into size_t.

My num_pages != (size_t)num_pages guards

    cache->max_num_items = num_pages;

>> ?
>> 
>> If not, please propose something you like better.
>> 
>> > (Arguably the check should also happen in migrate_params_test_apply)
>> 
>> Feels like one bridge too far for this patch.
>
> Sure.
>
> Dave

[PATCH 5/6] migration: Fix cache_init()'s "Failed to allocate" error messages

[Markus Armbruster]

cache_init() attempts to handle allocation failure..  The two error
messages are garbage, as untested error messages commonly are:

    Parameter 'cache size' expects Failed to allocate cache
    Parameter 'cache size' expects Failed to allocate page cache

Fix them to just

    Failed to allocate cache
    Failed to allocate page cache

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 migration/page_cache.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/migration/page_cache.c b/migration/page_cache.c
index e07f4ad1dc..ed979eeb45 100644
--- a/migration/page_cache.c
+++ b/migration/page_cache.c
@@ -53,8 +53,7 @@ PageCache *cache_init(uint64_t new_size, size_t page_size, Error **errp)
     /* We prefer not to abort if there is no memory */
     cache = g_try_malloc(sizeof(*cache));
     if (!cache) {
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
-                   "Failed to allocate cache");
+        error_setg(errp, "Failed to allocate cache");
         return NULL;
     }
     cache->page_size = page_size;
@@ -67,8 +66,7 @@ PageCache *cache_init(uint64_t new_size, size_t page_size, Error **errp)
     cache->page_cache = g_try_malloc_n(cache->max_num_items,
                                        sizeof(*cache->page_cache));
     if (!cache->page_cache) {
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
-                   "Failed to allocate page cache");
+        error_setg(errp, "Failed to allocate page cache");
         g_free(cache);
         return NULL;
     }
-- 
2.26.2

Re: [PATCH 5/6] migration: Fix cache_init()'s "Failed to allocate" error messages

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> cache_init() attempts to handle allocation failure..  The two error
> messages are garbage, as untested error messages commonly are:
> 
>     Parameter 'cache size' expects Failed to allocate cache
>     Parameter 'cache size' expects Failed to allocate page cache
> 
> Fix them to just
> 
>     Failed to allocate cache
>     Failed to allocate page cache
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/page_cache.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/page_cache.c b/migration/page_cache.c
> index e07f4ad1dc..ed979eeb45 100644
> --- a/migration/page_cache.c
> +++ b/migration/page_cache.c
> @@ -53,8 +53,7 @@ PageCache *cache_init(uint64_t new_size, size_t page_size, Error **errp)
>      /* We prefer not to abort if there is no memory */
>      cache = g_try_malloc(sizeof(*cache));
>      if (!cache) {
> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> -                   "Failed to allocate cache");
> +        error_setg(errp, "Failed to allocate cache");
>          return NULL;
>      }
>      cache->page_size = page_size;
> @@ -67,8 +66,7 @@ PageCache *cache_init(uint64_t new_size, size_t page_size, Error **errp)
>      cache->page_cache = g_try_malloc_n(cache->max_num_items,
>                                         sizeof(*cache->page_cache));
>      if (!cache->page_cache) {
> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> -                   "Failed to allocate page cache");
> +        error_setg(errp, "Failed to allocate page cache");
>          g_free(cache);
>          return NULL;
>      }
> -- 
> 2.26.2
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH 6/6] migration: Fix a few absurdly defective error messages

[Markus Armbruster]

migrate_params_check() has a number of error messages of the form

    Parameter 'NAME' expects is invalid, it should be ...

Fix them to something like

    Parameter 'NAME' expects a ...

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 migration/migration.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 3daed2da0e..5f9a10909d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1240,21 +1240,21 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
     if (params->has_compress_level &&
         (params->compress_level > 9)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
-                   "is invalid, it should be in the range of 0 to 9");
+                   "a value between 0 and 9");
         return false;
     }
 
     if (params->has_compress_threads && (params->compress_threads < 1)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    "compress_threads",
-                   "is invalid, it should be in the range of 1 to 255");
+                   "a value between 1 and 255");
         return false;
     }
 
     if (params->has_decompress_threads && (params->decompress_threads < 1)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    "decompress_threads",
-                   "is invalid, it should be in the range of 1 to 255");
+                   "a value between 1 and 255");
         return false;
     }
 
@@ -1307,21 +1307,21 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
     if (params->has_multifd_channels && (params->multifd_channels < 1)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    "multifd_channels",
-                   "is invalid, it should be in the range of 1 to 255");
+                   "a value between 1 and 255");
         return false;
     }
 
     if (params->has_multifd_zlib_level &&
         (params->multifd_zlib_level > 9)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "multifd_zlib_level",
-                   "is invalid, it should be in the range of 0 to 9");
+                   "a value between 0 and 9");
         return false;
     }
 
     if (params->has_multifd_zstd_level &&
         (params->multifd_zstd_level > 20)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "multifd_zstd_level",
-                   "is invalid, it should be in the range of 0 to 20");
+                   "a value between 0 and 20");
         return false;
     }
 
@@ -1330,8 +1330,7 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
          !is_power_of_2(params->xbzrle_cache_size))) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    "xbzrle_cache_size",
-                   "is invalid, it should be bigger than target page size"
-                   " and a power of 2");
+                   "a power of two no less than the target page size");
         return false;
     }
 
@@ -1348,21 +1347,21 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
         params->announce_initial > 100000) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    "announce_initial",
-                   "is invalid, it must be less than 100000 ms");
+                   "a value between 0 and 100000");
         return false;
     }
     if (params->has_announce_max &&
         params->announce_max > 100000) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    "announce_max",
-                   "is invalid, it must be less than 100000 ms");
+                   "a value between 0 and 100000");
        return false;
     }
     if (params->has_announce_rounds &&
         params->announce_rounds > 1000) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    "announce_rounds",
-                   "is invalid, it must be in the range of 0 to 1000");
+                   "a value between 0 and 1000");
        return false;
     }
     if (params->has_announce_step &&
@@ -1370,7 +1369,7 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
         params->announce_step > 10000)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    "announce_step",
-                   "is invalid, it must be in the range of 1 to 10000 ms");
+                   "a value between 0 and 10000");
        return false;
     }
 
-- 
2.26.2

Re: [PATCH 6/6] migration: Fix a few absurdly defective error messages

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> migrate_params_check() has a number of error messages of the form
> 
>     Parameter 'NAME' expects is invalid, it should be ...
> 
> Fix them to something like
> 
>     Parameter 'NAME' expects a ...
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/migration.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 3daed2da0e..5f9a10909d 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1240,21 +1240,21 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
>      if (params->has_compress_level &&
>          (params->compress_level > 9)) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
> -                   "is invalid, it should be in the range of 0 to 9");
> +                   "a value between 0 and 9");
>          return false;
>      }
>  
>      if (params->has_compress_threads && (params->compress_threads < 1)) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                     "compress_threads",
> -                   "is invalid, it should be in the range of 1 to 255");
> +                   "a value between 1 and 255");
>          return false;
>      }
>  
>      if (params->has_decompress_threads && (params->decompress_threads < 1)) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                     "decompress_threads",
> -                   "is invalid, it should be in the range of 1 to 255");
> +                   "a value between 1 and 255");
>          return false;
>      }
>  
> @@ -1307,21 +1307,21 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
>      if (params->has_multifd_channels && (params->multifd_channels < 1)) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                     "multifd_channels",
> -                   "is invalid, it should be in the range of 1 to 255");
> +                   "a value between 1 and 255");
>          return false;
>      }
>  
>      if (params->has_multifd_zlib_level &&
>          (params->multifd_zlib_level > 9)) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "multifd_zlib_level",
> -                   "is invalid, it should be in the range of 0 to 9");
> +                   "a value between 0 and 9");
>          return false;
>      }
>  
>      if (params->has_multifd_zstd_level &&
>          (params->multifd_zstd_level > 20)) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "multifd_zstd_level",
> -                   "is invalid, it should be in the range of 0 to 20");
> +                   "a value between 0 and 20");
>          return false;
>      }
>  
> @@ -1330,8 +1330,7 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
>           !is_power_of_2(params->xbzrle_cache_size))) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                     "xbzrle_cache_size",
> -                   "is invalid, it should be bigger than target page size"
> -                   " and a power of 2");
> +                   "a power of two no less than the target page size");
>          return false;
>      }
>  
> @@ -1348,21 +1347,21 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
>          params->announce_initial > 100000) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                     "announce_initial",
> -                   "is invalid, it must be less than 100000 ms");
> +                   "a value between 0 and 100000");
>          return false;
>      }
>      if (params->has_announce_max &&
>          params->announce_max > 100000) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                     "announce_max",
> -                   "is invalid, it must be less than 100000 ms");
> +                   "a value between 0 and 100000");
>         return false;
>      }
>      if (params->has_announce_rounds &&
>          params->announce_rounds > 1000) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                     "announce_rounds",
> -                   "is invalid, it must be in the range of 0 to 1000");
> +                   "a value between 0 and 1000");
>         return false;
>      }
>      if (params->has_announce_step &&
> @@ -1370,7 +1369,7 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
>          params->announce_step > 10000)) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                     "announce_step",
> -                   "is invalid, it must be in the range of 1 to 10000 ms");
> +                   "a value between 0 and 10000");
>         return false;
>      }
>  
> -- 
> 2.26.2
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 0/6] migration: Fixes and cleanups aroung migrate-set-parameters

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Not sure about 5.2.  The bugs aren't recent regressions.

Lets leave it till 5.3

> Markus Armbruster (6):
>   migration: Fix and clean up around @tls-authz
>   migration: Fix migrate-set-parameters argument validation
>   migration: Clean up signed vs. unsigned XBZRLE cache-size
>   migration: Check xbzrle-cache-size more carefully
>   migration: Fix cache_init()'s "Failed to allocate" error messages
>   migration: Fix a few absurdly defective error messages
> 
>  qapi/migration.json    | 34 ++++++++++++++++----------------
>  migration/migration.h  |  2 +-
>  migration/page_cache.h |  2 +-
>  migration/ram.h        |  2 +-
>  migration/migration.c  | 44 ++++++++++++++++++++++++++----------------
>  migration/page_cache.c | 23 +++++++---------------
>  migration/ram.c        |  9 +--------
>  monitor/hmp-cmds.c     | 26 ++++++++++++-------------
>  8 files changed, 68 insertions(+), 74 deletions(-)
> 
> -- 
> 2.26.2
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK