[v1] qapi: Elide redundant has_FOO in generated C

[PATCH 14/27] qapi migration: Elide redundant has_FOO in generated C

Posted by Markus Armbruster 3 years, 4 months ago

The has_FOO for pointer-valued FOO are redundant, except for arrays.
They are also a nuisance to work with.  Recent commit "qapi: Start to
elide redundant has_FOO in generated C" provided the means to elide
them step by step.  This is the step for qapi/migration.json.

Said commit explains the transformation in more detail.  The invariant
violations mentioned there do not occur here.

Cc: Juan Quintela <quintela@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 migration/block-dirty-bitmap.c |  4 ++--
 migration/colo.c               |  1 -
 migration/migration.c          | 27 ++++++++-------------------
 monitor/hmp-cmds.c             | 26 +++++++++++---------------
 monitor/misc.c                 |  2 +-
 scripts/qapi/schema.py         |  1 -
 6 files changed, 22 insertions(+), 39 deletions(-)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 9aba7d9c22..283017d7d3 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -551,7 +551,7 @@ static int add_bitmaps_to_list(DBMSaveState *s, BlockDriverState *bs,
             }
 
             bitmap_alias = bmap_inner->alias;
-            if (bmap_inner->has_transform) {
+            if (bmap_inner->transform) {
                 bitmap_transform = bmap_inner->transform;
             }
         } else {
@@ -821,7 +821,7 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s)
     }
 
     if (s->bmap_inner &&
-        s->bmap_inner->has_transform &&
+        s->bmap_inner->transform &&
         s->bmap_inner->transform->has_persistent) {
         persistent = s->bmap_inner->transform->persistent;
     } else {
diff --git a/migration/colo.c b/migration/colo.c
index 2b71722fd6..232c8d44b1 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -250,7 +250,6 @@ ReplicationStatus *qmp_query_xen_replication_status(Error **errp)
     replication_get_error_all(&err);
     if (err) {
         s->error = true;
-        s->has_desc = true;
         s->desc = g_strdup(error_get_pretty(err));
     } else {
         s->error = false;
diff --git a/migration/migration.c b/migration/migration.c
index bb8bbddfe4..3a73f7122e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -917,11 +917,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->cpu_throttle_increment = s->parameters.cpu_throttle_increment;
     params->has_cpu_throttle_tailslow = true;
     params->cpu_throttle_tailslow = s->parameters.cpu_throttle_tailslow;
-    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);
-    params->has_tls_authz = true;
     params->tls_authz = g_strdup(s->parameters.tls_authz ?
                                  s->parameters.tls_authz : "");
     params->has_max_bandwidth = true;
@@ -1046,7 +1043,6 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
 {
     size_t page_size = qemu_target_page_size();
 
-    info->has_ram = true;
     info->ram = g_malloc0(sizeof(*info->ram));
     info->ram->transferred = ram_counters.transferred;
     info->ram->total = ram_bytes_total();
@@ -1068,7 +1064,6 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
     info->ram->postcopy_bytes = ram_counters.postcopy_bytes;
 
     if (migrate_use_xbzrle()) {
-        info->has_xbzrle_cache = true;
         info->xbzrle_cache = g_malloc0(sizeof(*info->xbzrle_cache));
         info->xbzrle_cache->cache_size = migrate_xbzrle_cache_size();
         info->xbzrle_cache->bytes = xbzrle_counters.bytes;
@@ -1080,7 +1075,6 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
     }
 
     if (migrate_use_compression()) {
-        info->has_compression = true;
         info->compression = g_malloc0(sizeof(*info->compression));
         info->compression->pages = compression_counters.pages;
         info->compression->busy = compression_counters.busy;
@@ -1105,7 +1099,6 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
 static void populate_disk_info(MigrationInfo *info)
 {
     if (blk_mig_active()) {
-        info->has_disk = true;
         info->disk = g_malloc0(sizeof(*info->disk));
         info->disk->transferred = blk_mig_bytes_transferred();
         info->disk->remaining = blk_mig_bytes_remaining();
@@ -1170,7 +1163,6 @@ static void fill_source_migration_info(MigrationInfo *info)
     case MIGRATION_STATUS_FAILED:
         info->has_status = true;
         if (s->error) {
-            info->has_error_desc = true;
             info->error_desc = g_strdup(error_get_pretty(s->error));
         }
         break;
@@ -1556,7 +1548,7 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
 #ifdef CONFIG_LINUX
     if (migrate_use_zero_copy_send() &&
         ((params->has_multifd_compression && params->multifd_compression) ||
-         (params->has_tls_creds && params->tls_creds && *params->tls_creds))) {
+         (params->tls_creds && params->tls_creds && *params->tls_creds))) {
         error_setg(errp,
                    "Zero copy only available for non-compressed non-TLS multifd migration");
         return false;
@@ -1605,12 +1597,12 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
         dest->cpu_throttle_tailslow = params->cpu_throttle_tailslow;
     }
 
-    if (params->has_tls_creds) {
+    if (params->tls_creds) {
         assert(params->tls_creds->type == QTYPE_QSTRING);
         dest->tls_creds = params->tls_creds->u.s;
     }
 
-    if (params->has_tls_hostname) {
+    if (params->tls_hostname) {
         assert(params->tls_hostname->type == QTYPE_QSTRING);
         dest->tls_hostname = params->tls_hostname->u.s;
     }
@@ -1702,19 +1694,19 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
         s->parameters.cpu_throttle_tailslow = params->cpu_throttle_tailslow;
     }
 
-    if (params->has_tls_creds) {
+    if (params->tls_creds) {
         g_free(s->parameters.tls_creds);
         assert(params->tls_creds->type == QTYPE_QSTRING);
         s->parameters.tls_creds = g_strdup(params->tls_creds->u.s);
     }
 
-    if (params->has_tls_hostname) {
+    if (params->tls_hostname) {
         g_free(s->parameters.tls_hostname);
         assert(params->tls_hostname->type == QTYPE_QSTRING);
         s->parameters.tls_hostname = g_strdup(params->tls_hostname->u.s);
     }
 
-    if (params->has_tls_authz) {
+    if (params->tls_authz) {
         g_free(s->parameters.tls_authz);
         assert(params->tls_authz->type == QTYPE_QSTRING);
         s->parameters.tls_authz = g_strdup(params->tls_authz->u.s);
@@ -1791,14 +1783,14 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
     MigrationParameters tmp;
 
     /* TODO Rewrite "" to null instead */
-    if (params->has_tls_creds
+    if (params->tls_creds
         && params->tls_creds->type == QTYPE_QNULL) {
         qobject_unref(params->tls_creds->u.n);
         params->tls_creds->type = QTYPE_QSTRING;
         params->tls_creds->u.s = strdup("");
     }
     /* TODO Rewrite "" to null instead */
-    if (params->has_tls_hostname
+    if (params->tls_hostname
         && params->tls_hostname->type == QTYPE_QNULL) {
         qobject_unref(params->tls_hostname->u.n);
         params->tls_hostname->type = QTYPE_QSTRING;
@@ -4473,9 +4465,6 @@ static void migration_instance_init(Object *obj)
     params->has_announce_max = true;
     params->has_announce_rounds = true;
     params->has_announce_step = true;
-    params->has_tls_creds = true;
-    params->has_tls_hostname = true;
-    params->has_tls_authz = true;
 
     qemu_sem_init(&ms->postcopy_pause_sem, 0);
     qemu_sem_init(&ms->postcopy_pause_rp_sem, 0);
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 134c29698c..6ed9628071 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -238,8 +238,7 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
     if (info->has_status) {
         monitor_printf(mon, "Migration status: %s",
                        MigrationStatus_str(info->status));
-        if (info->status == MIGRATION_STATUS_FAILED &&
-            info->has_error_desc) {
+        if (info->status == MIGRATION_STATUS_FAILED && info->error_desc) {
             monitor_printf(mon, " (%s)\n", info->error_desc);
         } else {
             monitor_printf(mon, "\n");
@@ -261,7 +260,7 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
         }
     }
 
-    if (info->has_ram) {
+    if (info->ram) {
         monitor_printf(mon, "transferred ram: %" PRIu64 " kbytes\n",
                        info->ram->transferred >> 10);
         monitor_printf(mon, "throughput: %0.2f mbps\n",
@@ -314,7 +313,7 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
         }
     }
 
-    if (info->has_disk) {
+    if (info->disk) {
         monitor_printf(mon, "transferred disk: %" PRIu64 " kbytes\n",
                        info->disk->transferred >> 10);
         monitor_printf(mon, "remaining disk: %" PRIu64 " kbytes\n",
@@ -323,7 +322,7 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
                        info->disk->total >> 10);
     }
 
-    if (info->has_xbzrle_cache) {
+    if (info->xbzrle_cache) {
         monitor_printf(mon, "cache size: %" PRIu64 " bytes\n",
                        info->xbzrle_cache->cache_size);
         monitor_printf(mon, "xbzrle transferred: %" PRIu64 " kbytes\n",
@@ -340,7 +339,7 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
                        info->xbzrle_cache->overflow);
     }
 
-    if (info->has_compression) {
+    if (info->compression) {
         monitor_printf(mon, "compression pages: %" PRIu64 " pages\n",
                        info->compression->pages);
         monitor_printf(mon, "compression busy: %" PRIu64 "\n",
@@ -387,7 +386,7 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "]\n");
     }
 
-    if (info->has_vfio) {
+    if (info->vfio) {
         monitor_printf(mon, "vfio device transferred: %" PRIu64 " kbytes\n",
                        info->vfio->transferred >> 10);
     }
@@ -467,11 +466,11 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "%s: %u\n",
             MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_THROTTLE),
             params->max_cpu_throttle);
-        assert(params->has_tls_creds);
+        assert(params->tls_creds);
         monitor_printf(mon, "%s: '%s'\n",
             MigrationParameter_str(MIGRATION_PARAMETER_TLS_CREDS),
             params->tls_creds);
-        assert(params->has_tls_hostname);
+        assert(params->tls_hostname);
         monitor_printf(mon, "%s: '%s'\n",
             MigrationParameter_str(MIGRATION_PARAMETER_TLS_HOSTNAME),
             params->tls_hostname);
@@ -1256,19 +1255,16 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
         visit_type_uint8(v, param, &p->max_cpu_throttle, &err);
         break;
     case MIGRATION_PARAMETER_TLS_CREDS:
-        p->has_tls_creds = true;
         p->tls_creds = g_new0(StrOrNull, 1);
         p->tls_creds->type = QTYPE_QSTRING;
         visit_type_str(v, param, &p->tls_creds->u.s, &err);
         break;
     case MIGRATION_PARAMETER_TLS_HOSTNAME:
-        p->has_tls_hostname = true;
         p->tls_hostname = g_new0(StrOrNull, 1);
         p->tls_hostname->type = QTYPE_QSTRING;
         visit_type_str(v, param, &p->tls_hostname->u.s, &err);
         break;
     case MIGRATION_PARAMETER_TLS_AUTHZ:
-        p->has_tls_authz = true;
         p->tls_authz = g_new0(StrOrNull, 1);
         p->tls_authz->type = QTYPE_QSTRING;
         visit_type_str(v, param, &p->tls_authz->u.s, &err);
@@ -1380,7 +1376,7 @@ void hmp_client_migrate_info(Monitor *mon, const QDict *qdict)
 
     qmp_client_migrate_info(protocol, hostname,
                             has_port, port, has_tls_port, tls_port,
-                            !!cert_subject, cert_subject, &err);
+                            cert_subject, &err);
     hmp_handle_error(mon, err);
 }
 
@@ -1538,7 +1534,7 @@ static void hmp_migrate_status_cb(void *opaque)
     info = qmp_query_migrate(NULL);
     if (!info->has_status || info->status == MIGRATION_STATUS_ACTIVE ||
         info->status == MIGRATION_STATUS_SETUP) {
-        if (info->has_disk) {
+        if (info->disk) {
             int progress;
 
             if (info->disk->remaining) {
@@ -1556,7 +1552,7 @@ static void hmp_migrate_status_cb(void *opaque)
         if (status->is_block_migration) {
             monitor_printf(status->mon, "\n");
         }
-        if (info->has_error_desc) {
+        if (info->error_desc) {
             error_report("%s", info->error_desc);
         }
         monitor_resume(status->mon);
diff --git a/monitor/misc.c b/monitor/misc.c
index 3d2312ba8d..f18ae681da 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -391,7 +391,7 @@ static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
 void qmp_client_migrate_info(const char *protocol, const char *hostname,
                              bool has_port, int64_t port,
                              bool has_tls_port, int64_t tls_port,
-                             bool has_cert_subject, const char *cert_subject,
+                             const char *cert_subject,
                              Error **errp)
 {
     if (strcmp(protocol, "spice") == 0) {
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 1bc5b9f29d..94f97be53c 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -759,7 +759,6 @@ def need_has(self):
         assert self.type
         # Temporary hack to support dropping the has_FOO in reviewable chunks
         opt_out = [
-            'qapi/migration.json',
             'qapi/misc.json',
             'qapi/net.json',
             'qapi/pci.json',
-- 
2.37.2

Re: [PATCH 14/27] qapi migration: Elide redundant has_FOO in generated C

Posted by Philippe Mathieu-Daudé via 3 years, 4 months ago

On Thu, Sep 15, 2022 at 10:52 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> The has_FOO for pointer-valued FOO are redundant, except for arrays.
> They are also a nuisance to work with.  Recent commit "qapi: Start to
> elide redundant has_FOO in generated C" provided the means to elide
> them step by step.  This is the step for qapi/migration.json.
>
> Said commit explains the transformation in more detail.  The invariant
> violations mentioned there do not occur here.
>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  migration/block-dirty-bitmap.c |  4 ++--
>  migration/colo.c               |  1 -
>  migration/migration.c          | 27 ++++++++-------------------
>  monitor/hmp-cmds.c             | 26 +++++++++++---------------
>  monitor/misc.c                 |  2 +-
>  scripts/qapi/schema.py         |  1 -
>  6 files changed, 22 insertions(+), 39 deletions(-)

> @@ -1556,7 +1548,7 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
>  #ifdef CONFIG_LINUX
>      if (migrate_use_zero_copy_send() &&
>          ((params->has_multifd_compression && params->multifd_compression) ||
> -         (params->has_tls_creds && params->tls_creds && *params->tls_creds))) {
> +         (params->tls_creds && params->tls_creds && *params->tls_creds))) {
>          error_setg(errp,
>                     "Zero copy only available for non-compressed non-TLS multifd migration");
>          return false;

Checking params->tls_creds once should be enough ;)

Re: [PATCH 14/27] qapi migration: Elide redundant has_FOO in generated C

Posted by Markus Armbruster 3 years, 4 months ago

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On Thu, Sep 15, 2022 at 10:52 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> The has_FOO for pointer-valued FOO are redundant, except for arrays.
>> They are also a nuisance to work with.  Recent commit "qapi: Start to
>> elide redundant has_FOO in generated C" provided the means to elide
>> them step by step.  This is the step for qapi/migration.json.
>>
>> Said commit explains the transformation in more detail.  The invariant
>> violations mentioned there do not occur here.
>>
>> Cc: Juan Quintela <quintela@redhat.com>
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  migration/block-dirty-bitmap.c |  4 ++--
>>  migration/colo.c               |  1 -
>>  migration/migration.c          | 27 ++++++++-------------------
>>  monitor/hmp-cmds.c             | 26 +++++++++++---------------
>>  monitor/misc.c                 |  2 +-
>>  scripts/qapi/schema.py         |  1 -
>>  6 files changed, 22 insertions(+), 39 deletions(-)
>
>> @@ -1556,7 +1548,7 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
>>  #ifdef CONFIG_LINUX
>>      if (migrate_use_zero_copy_send() &&
>>          ((params->has_multifd_compression && params->multifd_compression) ||
>> -         (params->has_tls_creds && params->tls_creds && *params->tls_creds))) {
>> +         (params->tls_creds && params->tls_creds && *params->tls_creds))) {
>>          error_setg(errp,
>>                     "Zero copy only available for non-compressed non-TLS multifd migration");
>>          return false;
>
> Checking params->tls_creds once should be enough ;)

Can't deny that!

Will tidy up, thanks!

Re: [PATCH 14/27] qapi migration: Elide redundant has_FOO in generated C

Posted by Dr. David Alan Gilbert 3 years, 4 months ago

* Markus Armbruster (armbru@redhat.com) wrote:
> The has_FOO for pointer-valued FOO are redundant, except for arrays.
> They are also a nuisance to work with.  Recent commit "qapi: Start to
> elide redundant has_FOO in generated C" provided the means to elide
> them step by step.  This is the step for qapi/migration.json.
> 
> Said commit explains the transformation in more detail.  The invariant
> violations mentioned there do not occur here.
> 
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Yeh I'm OK with that, I do hate the has_'s:

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

Just cc'd in Dan though, wasn't there something special about the TLS
options?

Dave

> ---
>  migration/block-dirty-bitmap.c |  4 ++--
>  migration/colo.c               |  1 -
>  migration/migration.c          | 27 ++++++++-------------------
>  monitor/hmp-cmds.c             | 26 +++++++++++---------------
>  monitor/misc.c                 |  2 +-
>  scripts/qapi/schema.py         |  1 -
>  6 files changed, 22 insertions(+), 39 deletions(-)
> 
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 9aba7d9c22..283017d7d3 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -551,7 +551,7 @@ static int add_bitmaps_to_list(DBMSaveState *s, BlockDriverState *bs,
>              }
>  
>              bitmap_alias = bmap_inner->alias;
> -            if (bmap_inner->has_transform) {
> +            if (bmap_inner->transform) {
>                  bitmap_transform = bmap_inner->transform;
>              }
>          } else {
> @@ -821,7 +821,7 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s)
>      }
>  
>      if (s->bmap_inner &&
> -        s->bmap_inner->has_transform &&
> +        s->bmap_inner->transform &&
>          s->bmap_inner->transform->has_persistent) {
>          persistent = s->bmap_inner->transform->persistent;
>      } else {
> diff --git a/migration/colo.c b/migration/colo.c
> index 2b71722fd6..232c8d44b1 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -250,7 +250,6 @@ ReplicationStatus *qmp_query_xen_replication_status(Error **errp)
>      replication_get_error_all(&err);
>      if (err) {
>          s->error = true;
> -        s->has_desc = true;
>          s->desc = g_strdup(error_get_pretty(err));
>      } else {
>          s->error = false;
> diff --git a/migration/migration.c b/migration/migration.c
> index bb8bbddfe4..3a73f7122e 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -917,11 +917,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>      params->cpu_throttle_increment = s->parameters.cpu_throttle_increment;
>      params->has_cpu_throttle_tailslow = true;
>      params->cpu_throttle_tailslow = s->parameters.cpu_throttle_tailslow;
> -    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);
> -    params->has_tls_authz = true;
>      params->tls_authz = g_strdup(s->parameters.tls_authz ?
>                                   s->parameters.tls_authz : "");
>      params->has_max_bandwidth = true;
> @@ -1046,7 +1043,6 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
>  {
>      size_t page_size = qemu_target_page_size();
>  
> -    info->has_ram = true;
>      info->ram = g_malloc0(sizeof(*info->ram));
>      info->ram->transferred = ram_counters.transferred;
>      info->ram->total = ram_bytes_total();
> @@ -1068,7 +1064,6 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
>      info->ram->postcopy_bytes = ram_counters.postcopy_bytes;
>  
>      if (migrate_use_xbzrle()) {
> -        info->has_xbzrle_cache = true;
>          info->xbzrle_cache = g_malloc0(sizeof(*info->xbzrle_cache));
>          info->xbzrle_cache->cache_size = migrate_xbzrle_cache_size();
>          info->xbzrle_cache->bytes = xbzrle_counters.bytes;
> @@ -1080,7 +1075,6 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
>      }
>  
>      if (migrate_use_compression()) {
> -        info->has_compression = true;
>          info->compression = g_malloc0(sizeof(*info->compression));
>          info->compression->pages = compression_counters.pages;
>          info->compression->busy = compression_counters.busy;
> @@ -1105,7 +1099,6 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
>  static void populate_disk_info(MigrationInfo *info)
>  {
>      if (blk_mig_active()) {
> -        info->has_disk = true;
>          info->disk = g_malloc0(sizeof(*info->disk));
>          info->disk->transferred = blk_mig_bytes_transferred();
>          info->disk->remaining = blk_mig_bytes_remaining();
> @@ -1170,7 +1163,6 @@ static void fill_source_migration_info(MigrationInfo *info)
>      case MIGRATION_STATUS_FAILED:
>          info->has_status = true;
>          if (s->error) {
> -            info->has_error_desc = true;
>              info->error_desc = g_strdup(error_get_pretty(s->error));
>          }
>          break;
> @@ -1556,7 +1548,7 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
>  #ifdef CONFIG_LINUX
>      if (migrate_use_zero_copy_send() &&
>          ((params->has_multifd_compression && params->multifd_compression) ||
> -         (params->has_tls_creds && params->tls_creds && *params->tls_creds))) {
> +         (params->tls_creds && params->tls_creds && *params->tls_creds))) {
>          error_setg(errp,
>                     "Zero copy only available for non-compressed non-TLS multifd migration");
>          return false;
> @@ -1605,12 +1597,12 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
>          dest->cpu_throttle_tailslow = params->cpu_throttle_tailslow;
>      }
>  
> -    if (params->has_tls_creds) {
> +    if (params->tls_creds) {
>          assert(params->tls_creds->type == QTYPE_QSTRING);
>          dest->tls_creds = params->tls_creds->u.s;
>      }
>  
> -    if (params->has_tls_hostname) {
> +    if (params->tls_hostname) {
>          assert(params->tls_hostname->type == QTYPE_QSTRING);
>          dest->tls_hostname = params->tls_hostname->u.s;
>      }
> @@ -1702,19 +1694,19 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>          s->parameters.cpu_throttle_tailslow = params->cpu_throttle_tailslow;
>      }
>  
> -    if (params->has_tls_creds) {
> +    if (params->tls_creds) {
>          g_free(s->parameters.tls_creds);
>          assert(params->tls_creds->type == QTYPE_QSTRING);
>          s->parameters.tls_creds = g_strdup(params->tls_creds->u.s);
>      }
>  
> -    if (params->has_tls_hostname) {
> +    if (params->tls_hostname) {
>          g_free(s->parameters.tls_hostname);
>          assert(params->tls_hostname->type == QTYPE_QSTRING);
>          s->parameters.tls_hostname = g_strdup(params->tls_hostname->u.s);
>      }
>  
> -    if (params->has_tls_authz) {
> +    if (params->tls_authz) {
>          g_free(s->parameters.tls_authz);
>          assert(params->tls_authz->type == QTYPE_QSTRING);
>          s->parameters.tls_authz = g_strdup(params->tls_authz->u.s);
> @@ -1791,14 +1783,14 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
>      MigrationParameters tmp;
>  
>      /* TODO Rewrite "" to null instead */
> -    if (params->has_tls_creds
> +    if (params->tls_creds
>          && params->tls_creds->type == QTYPE_QNULL) {
>          qobject_unref(params->tls_creds->u.n);
>          params->tls_creds->type = QTYPE_QSTRING;
>          params->tls_creds->u.s = strdup("");
>      }
>      /* TODO Rewrite "" to null instead */
> -    if (params->has_tls_hostname
> +    if (params->tls_hostname
>          && params->tls_hostname->type == QTYPE_QNULL) {
>          qobject_unref(params->tls_hostname->u.n);
>          params->tls_hostname->type = QTYPE_QSTRING;
> @@ -4473,9 +4465,6 @@ static void migration_instance_init(Object *obj)
>      params->has_announce_max = true;
>      params->has_announce_rounds = true;
>      params->has_announce_step = true;
> -    params->has_tls_creds = true;
> -    params->has_tls_hostname = true;
> -    params->has_tls_authz = true;
>  
>      qemu_sem_init(&ms->postcopy_pause_sem, 0);
>      qemu_sem_init(&ms->postcopy_pause_rp_sem, 0);
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 134c29698c..6ed9628071 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -238,8 +238,7 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>      if (info->has_status) {
>          monitor_printf(mon, "Migration status: %s",
>                         MigrationStatus_str(info->status));
> -        if (info->status == MIGRATION_STATUS_FAILED &&
> -            info->has_error_desc) {
> +        if (info->status == MIGRATION_STATUS_FAILED && info->error_desc) {
>              monitor_printf(mon, " (%s)\n", info->error_desc);
>          } else {
>              monitor_printf(mon, "\n");
> @@ -261,7 +260,7 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>          }
>      }
>  
> -    if (info->has_ram) {
> +    if (info->ram) {
>          monitor_printf(mon, "transferred ram: %" PRIu64 " kbytes\n",
>                         info->ram->transferred >> 10);
>          monitor_printf(mon, "throughput: %0.2f mbps\n",
> @@ -314,7 +313,7 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>          }
>      }
>  
> -    if (info->has_disk) {
> +    if (info->disk) {
>          monitor_printf(mon, "transferred disk: %" PRIu64 " kbytes\n",
>                         info->disk->transferred >> 10);
>          monitor_printf(mon, "remaining disk: %" PRIu64 " kbytes\n",
> @@ -323,7 +322,7 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>                         info->disk->total >> 10);
>      }
>  
> -    if (info->has_xbzrle_cache) {
> +    if (info->xbzrle_cache) {
>          monitor_printf(mon, "cache size: %" PRIu64 " bytes\n",
>                         info->xbzrle_cache->cache_size);
>          monitor_printf(mon, "xbzrle transferred: %" PRIu64 " kbytes\n",
> @@ -340,7 +339,7 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>                         info->xbzrle_cache->overflow);
>      }
>  
> -    if (info->has_compression) {
> +    if (info->compression) {
>          monitor_printf(mon, "compression pages: %" PRIu64 " pages\n",
>                         info->compression->pages);
>          monitor_printf(mon, "compression busy: %" PRIu64 "\n",
> @@ -387,7 +386,7 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>          monitor_printf(mon, "]\n");
>      }
>  
> -    if (info->has_vfio) {
> +    if (info->vfio) {
>          monitor_printf(mon, "vfio device transferred: %" PRIu64 " kbytes\n",
>                         info->vfio->transferred >> 10);
>      }
> @@ -467,11 +466,11 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>          monitor_printf(mon, "%s: %u\n",
>              MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_THROTTLE),
>              params->max_cpu_throttle);
> -        assert(params->has_tls_creds);
> +        assert(params->tls_creds);
>          monitor_printf(mon, "%s: '%s'\n",
>              MigrationParameter_str(MIGRATION_PARAMETER_TLS_CREDS),
>              params->tls_creds);
> -        assert(params->has_tls_hostname);
> +        assert(params->tls_hostname);
>          monitor_printf(mon, "%s: '%s'\n",
>              MigrationParameter_str(MIGRATION_PARAMETER_TLS_HOSTNAME),
>              params->tls_hostname);
> @@ -1256,19 +1255,16 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>          visit_type_uint8(v, param, &p->max_cpu_throttle, &err);
>          break;
>      case MIGRATION_PARAMETER_TLS_CREDS:
> -        p->has_tls_creds = true;
>          p->tls_creds = g_new0(StrOrNull, 1);
>          p->tls_creds->type = QTYPE_QSTRING;
>          visit_type_str(v, param, &p->tls_creds->u.s, &err);
>          break;
>      case MIGRATION_PARAMETER_TLS_HOSTNAME:
> -        p->has_tls_hostname = true;
>          p->tls_hostname = g_new0(StrOrNull, 1);
>          p->tls_hostname->type = QTYPE_QSTRING;
>          visit_type_str(v, param, &p->tls_hostname->u.s, &err);
>          break;
>      case MIGRATION_PARAMETER_TLS_AUTHZ:
> -        p->has_tls_authz = true;
>          p->tls_authz = g_new0(StrOrNull, 1);
>          p->tls_authz->type = QTYPE_QSTRING;
>          visit_type_str(v, param, &p->tls_authz->u.s, &err);
> @@ -1380,7 +1376,7 @@ void hmp_client_migrate_info(Monitor *mon, const QDict *qdict)
>  
>      qmp_client_migrate_info(protocol, hostname,
>                              has_port, port, has_tls_port, tls_port,
> -                            !!cert_subject, cert_subject, &err);
> +                            cert_subject, &err);
>      hmp_handle_error(mon, err);
>  }
>  
> @@ -1538,7 +1534,7 @@ static void hmp_migrate_status_cb(void *opaque)
>      info = qmp_query_migrate(NULL);
>      if (!info->has_status || info->status == MIGRATION_STATUS_ACTIVE ||
>          info->status == MIGRATION_STATUS_SETUP) {
> -        if (info->has_disk) {
> +        if (info->disk) {
>              int progress;
>  
>              if (info->disk->remaining) {
> @@ -1556,7 +1552,7 @@ static void hmp_migrate_status_cb(void *opaque)
>          if (status->is_block_migration) {
>              monitor_printf(status->mon, "\n");
>          }
> -        if (info->has_error_desc) {
> +        if (info->error_desc) {
>              error_report("%s", info->error_desc);
>          }
>          monitor_resume(status->mon);
> diff --git a/monitor/misc.c b/monitor/misc.c
> index 3d2312ba8d..f18ae681da 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -391,7 +391,7 @@ static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
>  void qmp_client_migrate_info(const char *protocol, const char *hostname,
>                               bool has_port, int64_t port,
>                               bool has_tls_port, int64_t tls_port,
> -                             bool has_cert_subject, const char *cert_subject,
> +                             const char *cert_subject,
>                               Error **errp)
>  {
>      if (strcmp(protocol, "spice") == 0) {
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 1bc5b9f29d..94f97be53c 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -759,7 +759,6 @@ def need_has(self):
>          assert self.type
>          # Temporary hack to support dropping the has_FOO in reviewable chunks
>          opt_out = [
> -            'qapi/migration.json',
>              'qapi/misc.json',
>              'qapi/net.json',
>              'qapi/pci.json',
> -- 
> 2.37.2
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 14/27] qapi migration: Elide redundant has_FOO in generated C

Posted by Daniel P. Berrangé 3 years, 4 months ago

On Thu, Sep 22, 2022 at 12:28:55PM +0100, Dr. David Alan Gilbert wrote:
> * Markus Armbruster (armbru@redhat.com) wrote:
> > The has_FOO for pointer-valued FOO are redundant, except for arrays.
> > They are also a nuisance to work with.  Recent commit "qapi: Start to
> > elide redundant has_FOO in generated C" provided the means to elide
> > them step by step.  This is the step for qapi/migration.json.
> > 
> > Said commit explains the transformation in more detail.  The invariant
> > violations mentioned there do not occur here.
> > 
> > Cc: Juan Quintela <quintela@redhat.com>
> > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> 
> Yeh I'm OK with that, I do hate the has_'s:
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> Just cc'd in Dan though, wasn't there something special about the TLS
> options?

Yes, some of them use StrOrNull rather than just 'str', but I think
the change is OK

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

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