Re: [PATCH v6 11/11] qapi: Use QAPI_LIST_ADD() where possible

[Eric Blake]

On 10/27/20 10:36 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 10/27/20 5:09 AM, Markus Armbruster wrote:
>>> Eric Blake <eblake@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.
> 
> It may make sense even if we don't.  QAPI_LIST_ADD() leaves the reader
> guessing whether we prepend or append.

That's a strong enough argument for me to make the rename in patch 2/11,
with minor rebase fallout in the rest of the series, and then this patch
gets a major rewrite (but I'm already not trying to get this patch into
5.2).


>>>> @@ -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).
> 
> I don't give a rat's ass about C++, actually.  I'm glad you got rid of
> the tacit "@new points to the same type as @ret does".
> 
> Clean:
> 
>              new = g_malloc0(sizeof(*new));
>              new = g_new0(GuestFilesystemInfoList, 1);
> 
> Clean (but I'd use g_new0() instead):
> 
>              new = g_malloc0(sizeof(GuestFilesystemInfoList));
> 
> Dirty:
> 
>              new = g_malloc0(sizeof(X));
> 
> where X is anything else.

Ah, I hadn't even spotted what you disliked, but yes, it makes total
sense that allocating for assignment to one variable by utilizing the
type from another puts unnecessary linkage that the two variables must
have the same type.


>>> 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.
> 
> One of us is blind, and it might be me :)

Oh, I indeed misread this.  Yes, this is prepending after all, so I'll
use the macro here.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org