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);
}
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)) {
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")) {
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* 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.
>
> 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
> 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")
> 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>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"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!
* Markus Armbruster (armbru@redhat.com) wrote:
> "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
Oops yes, sorry these are the other way around as you point out.
Dave
> >> 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!
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"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:
>> >> 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>
[...]
>> >> 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
>
> Oops yes, sorry these are the other way around as you point out.
I fell into the exact same trap myself :)
[...]
© 2016 - 2026 Red Hat, Inc.