Re: [PATCH v2 15/28] qapi misc: Elide redundant has_FOO in generated C

[Markus Armbruster <armbru@redhat.com>]

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

> * 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/misc.json.
>> 
>> Said commit explains the transformation in more detail.  The invariant
>> violations mentioned there do not occur here.
>> 
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  include/monitor/monitor.h |  3 +--
>>  monitor/hmp-cmds.c        |  2 +-
>>  monitor/misc.c            | 19 +++++--------------
>>  monitor/qmp-cmds.c        |  1 -
>>  softmmu/vl.c              |  2 +-
>>  util/qemu-config.c        | 17 +++++------------
>>  scripts/qapi/schema.py    |  1 -
>>  7 files changed, 13 insertions(+), 32 deletions(-)
>> 
>> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
>> index 737e750670..1e6f4c9bd7 100644
>> --- a/include/monitor/monitor.h
>> +++ b/include/monitor/monitor.h
>> @@ -46,8 +46,7 @@ int monitor_read_password(MonitorHMP *mon, ReadLineFunc *readline_func,
>>                            void *opaque);
>>  
>>  AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
>> -                                bool has_opaque, const char *opaque,
>> -                                Error **errp);
>> +                                const char *opaque, Error **errp);
>>  int monitor_fdset_dup_fd_add(int64_t fdset_id, int flags);
>>  void monitor_fdset_dup_fd_remove(int dup_fd);
>>  int64_t monitor_fdset_dup_fd_find(int dup_fd);
>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>> index 8077ed82c9..63baf3f8c6 100644
>> --- a/monitor/hmp-cmds.c
>> +++ b/monitor/hmp-cmds.c
>> @@ -104,7 +104,7 @@ void hmp_info_name(Monitor *mon, const QDict *qdict)
>>      NameInfo *info;
>>  
>>      info = qmp_query_name(NULL);
>> -    if (info->has_name) {
>> +    if (info->name) {
>>          monitor_printf(mon, "%s\n", info->name);
>>      }
>>      qapi_free_NameInfo(info);
>> diff --git a/monitor/misc.c b/monitor/misc.c
>> index a51f0996cb..2663007dbc 100644
>> --- a/monitor/misc.c
>> +++ b/monitor/misc.c
>> @@ -1131,7 +1131,7 @@ void monitor_fdsets_cleanup(void)
>>      }
>>  }
>>  
>> -AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool has_opaque,
>> +AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id,
>>                        const char *opaque, Error **errp)
>>  {
>>      int fd;
>> @@ -1144,8 +1144,7 @@ AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool has_opaque,
>>          goto error;
>>      }
>>  
>> -    fdinfo = monitor_fdset_add_fd(fd, has_fdset_id, fdset_id,
>> -                                  has_opaque, opaque, errp);
>> +    fdinfo = monitor_fdset_add_fd(fd, has_fdset_id, fdset_id, opaque, errp);
>>      if (fdinfo) {
>>          return fdinfo;
>>      }
>> @@ -1213,12 +1212,7 @@ FdsetInfoList *qmp_query_fdsets(Error **errp)
>>  
>>              fdsetfd_info = g_malloc0(sizeof(*fdsetfd_info));
>>              fdsetfd_info->fd = mon_fdset_fd->fd;
>> -            if (mon_fdset_fd->opaque) {
>> -                fdsetfd_info->has_opaque = true;
>> -                fdsetfd_info->opaque = g_strdup(mon_fdset_fd->opaque);
>> -            } else {
>> -                fdsetfd_info->has_opaque = false;
>> -            }
>> +            fdsetfd_info->opaque = g_strdup(mon_fdset_fd->opaque);
>>  
>>              QAPI_LIST_PREPEND(fdset_info->fds, fdsetfd_info);
>>          }
>> @@ -1230,8 +1224,7 @@ FdsetInfoList *qmp_query_fdsets(Error **errp)
>>  }
>>  
>>  AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
>> -                                bool has_opaque, const char *opaque,
>> -                                Error **errp)
>> +                                const char *opaque, Error **errp)
>>  {
>>      MonFdset *mon_fdset = NULL;
>>      MonFdsetFd *mon_fdset_fd;
>> @@ -1299,9 +1292,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
>>      mon_fdset_fd = g_malloc0(sizeof(*mon_fdset_fd));
>>      mon_fdset_fd->fd = fd;
>>      mon_fdset_fd->removed = false;
>> -    if (has_opaque) {
>> -        mon_fdset_fd->opaque = g_strdup(opaque);
>> -    }
>> +    mon_fdset_fd->opaque = g_strdup(opaque);
>>      QLIST_INSERT_HEAD(&mon_fdset->fds, mon_fdset_fd, next);
>>  
>>      fdinfo = g_malloc0(sizeof(*fdinfo));
>> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
>> index 81c8fdadf8..2a0c919472 100644
>> --- a/monitor/qmp-cmds.c
>> +++ b/monitor/qmp-cmds.c
>> @@ -51,7 +51,6 @@ NameInfo *qmp_query_name(Error **errp)
>>      NameInfo *info = g_malloc0(sizeof(*info));
>>  
>>      if (qemu_name) {
>> -        info->has_name = true;
>>          info->name = g_strdup(qemu_name);
>>      }
>
> I think you can lose the if there and just always do the strdup.

Yes, that's better.

>>  
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index b464da25bc..e044979dfc 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -611,7 +611,7 @@ static int parse_add_fd(void *opaque, QemuOpts *opts, Error **errp)
>>      }
>>  
>>      /* add the duplicate fd, and optionally the opaque string, to the fd set */
>> -    fdinfo = monitor_fdset_add_fd(dupfd, true, fdset_id, !!fd_opaque, fd_opaque,
>> +    fdinfo = monitor_fdset_add_fd(dupfd, true, fdset_id, fd_opaque,
>>                                    &error_abort);
>>      g_free(fdinfo);
>>  
>> diff --git a/util/qemu-config.c b/util/qemu-config.c
>> index 5325f6bf80..95f61fc883 100644
>> --- a/util/qemu-config.c
>> +++ b/util/qemu-config.c
>> @@ -80,14 +80,8 @@ static CommandLineParameterInfoList *query_option_descs(const QemuOptDesc *desc)
>>              break;
>>          }
>>  
>> -        if (desc[i].help) {
>> -            info->has_help = true;
>> -            info->help = g_strdup(desc[i].help);
>> -        }
>> -        if (desc[i].def_value_str) {
>> -            info->has_q_default = true;
>> -            info->q_default = g_strdup(desc[i].def_value_str);
>> -        }
>> +        info->help = g_strdup(desc[i].help);
>> +        info->q_default = g_strdup(desc[i].def_value_str);
>>  
>>          QAPI_LIST_PREPEND(param_list, info);
>>      }
>> @@ -245,8 +239,7 @@ static QemuOptsList machine_opts = {
>>      }
>>  };
>>  
>> -CommandLineOptionInfoList *qmp_query_command_line_options(bool has_option,
>> -                                                          const char *option,
>> +CommandLineOptionInfoList *qmp_query_command_line_options(const char *option,
>>                                                            Error **errp)
>>  {
>>      CommandLineOptionInfoList *conf_list = NULL;
>> @@ -254,7 +247,7 @@ CommandLineOptionInfoList *qmp_query_command_line_options(bool has_option,
>>      int i;
>>  
>>      for (i = 0; vm_config_groups[i] != NULL; i++) {
>> -        if (!has_option || !strcmp(option, vm_config_groups[i]->name)) {
>> +        if (!option || !strcmp(option, vm_config_groups[i]->name)) {
>
> I think that can be g_strcmp0 if you can convince yourself ->name is
> non-null

vm_config_groups[i] must not be null.

However, replacing the whole condition by !g_strcmp0() would be wrong:

    option                             |  null   ->name  neither
    -----------------------------------+------------------------
    !option || !strcmp(option, ->name) |  true     true    false
    g_strcmp0(option, ->name)          | false     true    false

>>              info = g_malloc0(sizeof(*info));
>>              info->option = g_strdup(vm_config_groups[i]->name);
>>              if (!strcmp("drive", vm_config_groups[i]->name)) {
>> @@ -267,7 +260,7 @@ CommandLineOptionInfoList *qmp_query_command_line_options(bool has_option,
>>          }
>>      }
>>  
>> -    if (!has_option || !strcmp(option, "machine")) {
>> +    if (!option || !strcmp(option, "machine")) {
>
> g_strcmp0(option, "machine")

Likewise.

>>          info = g_malloc0(sizeof(*info));
>>          info->option = g_strdup("machine");
>>          info->parameters = query_option_descs(machine_opts.desc);
>> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>> index ad5b665212..a34e25fdd7 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/misc.json',
>>              'qapi/net.json',
>>              'qapi/pci.json',
>>              'qapi/qdev.json',
>> -- 
>> 2.37.2
>
> Still,
>
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Thanks!