[GEDI] [PATCH v6 11/11] qapi: Use QAPI_LIST_ADD() where possible
Eric Blake
eblake at redhat.com
Tue Oct 27 12:28:48 UTC 2020
On 10/27/20 5:09 AM, Markus Armbruster wrote:
> Eric Blake <eblake at redhat.com> writes:
>
>> Anywhere we create a list of just one item or by prepending items
>> (typically because order doesn't matter), we can use the now-public
>> macro. But places where we must keep the list in order by appending
>> remain open-coded.
>
> Should we rename the macro to QAPI_LIST_PREPEND()?
That would make sense if we add a counterpart QAPI_LIST_APPEND.
>
> How many places append? If it's more than just a few, an attempt to
> factor out the common code is in order. Not in this patch, of course.
> Not even in this series.
Quite a few. The most common pattern for appending is like this from
qemu-img.c:
ImageInfoList *head = NULL, *elem;
ImageInfoList **last = &head;
...
while (...) {
elem = g_new0(ImageInfoList, 1);
elem->value = info;
*last = elem;
last = &elem->next;
}
although I saw several other patterns as well. And we frequently have
this comment, such as from block/qapi.c:
/* XXX: waiting for the qapi to support qemu-queue.h types */
Several of the existing append spots could be switched to prepend with
no change to semantics (the resulting list would be presented to the
user in the opposite order, but the semantics of that item were a set
rather than an ordered list so other than tweaking the testsuite, it
would not matter), while others absolutely have to append to maintain
correct order.
Part of me wonders if it would be worth adjusting the QAPI generator to
create a head and tail pointer for _every_ FOOList member, rather than
just a head pointer. Or to create a function for an O(n) reversal of an
existing list, then flipping spots to construct lists in reverse order
followed by a list reverse (no change in big-O complexity, more code
reuse, but slightly more CPU time). But as you observe, that quickly
goes beyond the scope of this series.
>> +++ b/docs/devel/writing-qmp-commands.txt
>> @@ -531,15 +531,10 @@ TimerAlarmMethodList *qmp_query_alarm_methods(Error **errp)
>> bool current = true;
>>
>> for (p = alarm_timers; p->name; p++) {
>> - TimerAlarmMethodList *info = g_malloc0(sizeof(*info));
[1]
>> - info->value = g_malloc0(sizeof(*info->value));
[2]
>> - info->value->method_name = g_strdup(p->name);
>> - info->value->current = current;
>> -
>> - current = false;
>> -
>> - info->next = method_list;
>> - method_list = info;
>> + TimerAlarmMethod *value = g_new0(TimerAlarmMethod, 1);
>
> Can just as well use g_new(), as QAPI_LIST_ADD() will set both members
> of @value. Same elsewhere.
Not quite. Allocation [1] can use g_new() instead of g_malloc0()
because we fill both members of info, but allocation [2] is unchanged by
this code transformation (I did not want to research whether the code
was filling all members of info->value (probably true, but it was
unrelated to my rewrite). Switching to QAPI_LIST_ADD is what moves
allocation [1] into the macro (where it indeed uses g_new), but
QAPI_LIST_ADD has no impact on the contents of value in allocation [2]
(which is the only allocation left locally in this hunk).
However, the fact that I changed from g_malloc0(sizeof(*info->value)) to
g_new0(TimerAlarmMethod, 1), instead of keeping it as
g_malloc0(sizeof(*value)), is indeed a case of me doing a bit more than
a strict mechanical conversion; this was one of the hunks I touched
earlier in my audit.
>> @@ -655,15 +656,9 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>> qemu_opts_del(opts);
>> }
>>
>> - if (gconf->server == NULL) {
>> - gconf->server = g_new0(SocketAddressList, 1);
>> - gconf->server->value = gsconf;
>> - curr = gconf->server;
>> - } else {
>> - curr->next = g_new0(SocketAddressList, 1);
>> - curr->next->value = gsconf;
>> - curr = curr->next;
>> - }
>> + *curr = g_new0(SocketAddressList, 1);
>> + (*curr)->value = gsconf;
>> + curr = &(*curr)->next;
>> gsconf = NULL;
>>
>> qobject_unref(backing_options);
>
> The change to qemu_gluster_parse_json() looks unrelated.
Indeed, this is also one of the earlier files I touched, where I saw
that our 'append' pattern can be simplified, but it's separate from the
'prepend' pattern that this patch should be touching. I guess I'll need
to clean this patch up to be more strictly mechanical.
>
>> diff --git a/chardev/char.c b/chardev/char.c
>> index 78553125d311..8dd7ef4c5935 100644
>> --- a/chardev/char.c
>> +++ b/chardev/char.c
>> @@ -776,15 +776,14 @@ static int qmp_query_chardev_foreach(Object *obj, void *data)
>> {
>> Chardev *chr = CHARDEV(obj);
>> ChardevInfoList **list = data;
>> - ChardevInfoList *info = g_malloc0(sizeof(*info));
>> + ChardevInfo *value;
>>
>> - info->value = g_malloc0(sizeof(*info->value));
>> - info->value->label = g_strdup(chr->label);
>> - info->value->filename = g_strdup(chr->filename);
>> - info->value->frontend_open = chr->be && chr->be->fe_open;
>> + value = g_malloc0(sizeof(*value));
>
> You could use an initializer instead, like you do in the next hunk. Up
> to you.
Yeah, the further I got into the manual audit, the less I was rewriting
code to minimize lines, such as consistently initializing variables at
their declaration.
>> +++ b/hw/core/machine.c
>> @@ -492,11 +492,7 @@ static void machine_set_nvdimm_persistence(Object *obj, const char *value,
>>
>> void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type)
>> {
>> - strList *item = g_new0(strList, 1);
>> -
>> - item->value = g_strdup(type);
>> - item->next = mc->allowed_dynamic_sysbus_devices;
>> - mc->allowed_dynamic_sysbus_devices = item;
>> + QAPI_LIST_ADD(mc->allowed_dynamic_sysbus_devices, g_strdup(type));
>
> Side effect in a macro argument. Works, because QAPI_LIST_ADD() expands
> @element exactly once. Sure we want to rely on it?
>
> If yes, please add a contract to QAPI_LIST_ADD() that documents it.
Multiple instances depend on it, so yes, I think it's worth documenting.
>
> More instances below.
Indeed.
>> +++ b/hw/net/rocker/rocker_fp.c
>> @@ -51,14 +51,14 @@ bool fp_port_get_link_up(FpPort *port)
>> return !qemu_get_queue(port->nic)->link_down;
>> }
>>
>> -void fp_port_get_info(FpPort *port, RockerPortList *info)
>> +void fp_port_get_info(FpPort *port, RockerPort *value)
>> {
>> - info->value->name = g_strdup(port->name);
>> - info->value->enabled = port->enabled;
>> - info->value->link_up = fp_port_get_link_up(port);
>> - info->value->speed = port->speed;
>> - info->value->duplex = port->duplex;
>> - info->value->autoneg = port->autoneg;
>> + value->name = g_strdup(port->name);
>> + value->enabled = port->enabled;
>> + value->link_up = fp_port_get_link_up(port);
>> + value->speed = port->speed;
>> + value->duplex = port->duplex;
>> + value->autoneg = port->autoneg;
>> }
>
> This cleanup of fp_port_get_info() could be a separate patch. Up to
> you.
>
> You could move the allocation into fp_port_get_info(), like this:
>
> RockerPort *fp_port_get_info(FpPort *port)
> {
> RockerPort *value = g_malloc0(sizeof(*value));
>
> value->name = g_strdup(port->name);
> value->enabled = port->enabled;
> value->link_up = fp_port_get_link_up(port);
> value->speed = port->speed;
> value->duplex = port->duplex;
> value->autoneg = port->autoneg;
> return value;
> }
>
> Also up to you.
Yeah, I thought about splitting that one out. I think you've convinced
me it is worth it.
>> +++ b/monitor/hmp-cmds.c
>> @@ -1248,7 +1248,8 @@ void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict)
>> const char *cap = qdict_get_str(qdict, "capability");
>> bool state = qdict_get_bool(qdict, "state");
>> Error *err = NULL;
>> - MigrationCapabilityStatusList *caps = g_malloc0(sizeof(*caps));
>> + MigrationCapabilityStatusList *caps = NULL;
>> + MigrationCapabilityStatus *value = NULL;
>
> No need to initialize @value.
>
>> int val;
>>
>> val = qapi_enum_parse(&MigrationCapability_lookup, cap, -1, &err);
>> @@ -1256,10 +1257,10 @@ void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict)
>> goto end;
>> }
>>
>> - caps->value = g_malloc0(sizeof(*caps->value));
>> - caps->value->capability = val;
>> - caps->value->state = state;
>> - caps->next = NULL;
>> + value = g_malloc0(sizeof(*value));
>> + value->capability = val;
>> + value->state = state;
>> + QAPI_LIST_ADD(caps, value);
>> qmp_migrate_set_capabilities(caps, &err);
>>
>> end:
> qapi_free_MigrationCapabilityStatusList(caps);
>
> This could be moved before the label now. No need to initialize @caps
> to null then. Up to you.
>
> hmp_handle_error(mon, err);
> }
>
Since the conversion was all manual, I don't mind making it look nicer,
when it is still minimally invasive.
>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 2103507936ea..4cfa8bccc5e7 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1643,14 +1643,13 @@ static void do_dirty_bitmap_merge(const char *dst_node, const char *dst_name,
>> Error **errp)
>> {
>> BlockDirtyBitmapMergeSource *merge_src;
>> - BlockDirtyBitmapMergeSourceList *list;
>> + BlockDirtyBitmapMergeSourceList *list = NULL;
>>
>> merge_src = g_new0(BlockDirtyBitmapMergeSource, 1);
>> merge_src->type = QTYPE_QDICT;
>> merge_src->u.external.node = g_strdup(src_node);
>> merge_src->u.external.name = g_strdup(src_name);
>> - list = g_new0(BlockDirtyBitmapMergeSourceList, 1);
>> - list->value = merge_src;
>> + QAPI_LIST_ADD(list, merge_src);
>> qmp_block_dirty_bitmap_merge(dst_node, dst_name, list, errp);
>> qapi_free_BlockDirtyBitmapMergeSourceList(list);
>> }
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 3bffee99d4c9..06540425ded2 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -1211,7 +1211,7 @@ GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
>> {
>> FsMountList mounts;
>> struct FsMount *mount;
>> - GuestFilesystemInfoList *new, *ret = NULL;
>> + GuestFilesystemInfoList *ret = NULL;
>> Error *local_err = NULL;
>>
>> QTAILQ_INIT(&mounts);
>> @@ -1224,10 +1224,7 @@ GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
>> QTAILQ_FOREACH(mount, &mounts, next) {
>> g_debug("Building guest fsinfo for '%s'", mount->dirname);
>>
>> - new = g_malloc0(sizeof(*ret));
>
> Ugh! Glad you get rid of this.
Yep, C++ reserved words as a C variable name is always awkward. It was
fun cleaning that up (several places in this patch).
>> +++ b/qga/commands-win32.c
>> @@ -926,10 +926,8 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
>> error_free(local_err);
>> goto out;
>> }
>> - list = g_malloc0(sizeof(*list));
>> - list->value = disk;
>> + QAPI_LIST_ADD(list, disk);
>> disk = NULL;
>> - list->next = NULL;
>> goto out;
>
> Both old and new code tacitly rely on @list being empty. Okay.
This was population of a single element into list (so prepending vs.
appending doesn't matter, and we can use the prepend macro), while...
>
>> } else {
>> error_setg_win32(errp, GetLastError(),
>
> Did you miss the spot where we add to this list?
>
> /* Go through each extent */
> for (i = 0; i < extents->NumberOfDiskExtents; i++) {
> disk = g_malloc0(sizeof(GuestDiskAddress));
>
> /* Disk numbers directly correspond to numbers used in UNCs
> *
> * See documentation for DISK_EXTENT:
> * https://docs.microsoft.com/en-us/windows/desktop/api/winioctl/ns-winioctl-_disk_extent
> *
> * See also Naming Files, Paths and Namespaces:
> * https://docs.microsoft.com/en-us/windows/desktop/FileIO/naming-a-file#win32-device-namespaces
> */
> disk->has_dev = true;
> disk->dev = g_strdup_printf("\\\\.\\PhysicalDrive%lu",
> extents->Extents[i].DiskNumber);
>
> get_single_disk_info(extents->Extents[i].DiskNumber, disk, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> goto out;
> }
> cur_item = g_malloc0(sizeof(*list));
> cur_item->value = disk;
> disk = NULL;
> cur_item->next = list;
> ---> list = cur_item;
> }
This is appending, not prepending. Using the macro here would have
reversed the multi-element list seen by the user. I did not check
whether the QMP is documenting that particular list more as a set (where
order does not matter, so prepending is fine) or as an ordered list, but
instead conservatively left it to appending. As we said earlier,
further cleanups of all append places may be worth its own later series.
>> +++ b/qom/qom-qmp-cmds.c
>> @@ -46,14 +46,12 @@ ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
>>
>> object_property_iter_init(&iter, obj);
>> while ((prop = object_property_iter_next(&iter))) {
>> - ObjectPropertyInfoList *entry = g_malloc0(sizeof(*entry));
>> + ObjectPropertyInfo *value = g_malloc0(sizeof(ObjectPropertyInfo));
>>
>> - entry->value = g_malloc0(sizeof(ObjectPropertyInfo));
>> - entry->next = props;
>> - props = entry;
>> + QAPI_LIST_ADD(props, value);
>>
>> - entry->value->name = g_strdup(prop->name);
>> - entry->value->type = g_strdup(prop->type);
>> + value->name = g_strdup(prop->name);
>> + value->type = g_strdup(prop->type);
>
> This is the minimally invasive patch. Best to stick to minimal in a big
> series like this one, to ease review as much as possible.
>
> If that wasn't an issue, I'd suggest finishing the list element before
> inserting it into the list:
>
> ObjectPropertyInfo *value = g_malloc0(sizeof(ObjectPropertyInfo));
>
> value->name = g_strdup(prop->name);
> value->type = g_strdup(prop->type);
> QAPI_LIST_ADD(props, value);
>
> There might be more instances.
Agreed (both that sticking to minimally invasive is good, and on the
fact that this patch points out a number of future potential cleanups).
>
> The macro definitely makes the code easier to read. Yes, please!
>
> The patch feels almost ready.
Thanks for the careful review!
Right now, my thoughts are to get 1-10 into 5.2, then turn this one
patch into a more-complete series for post-5.2 that address more cases
of QAPI list management (splitting this patch into at least 3 based on
the comments above, then new patches to make appending use more
consistent patterns before adding macros to ease that as well).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
More information about the integration
mailing list