[libvirt PATCH 0/2] Remove macros "VIR_INSERT_ELEMENT_COPY" and "VIR_INSERT_ELEMENT_COPY_INPLACE"

There was but one use left, and that is removed in patch 1. Tim Wiederhake (2): virQEMUCapsGetMachineTypesCaps: Use GPtrArray viralloc: Delete VIR_INSERT_ELEMENT_COPY and VIR_INSERT_ELEMENT_COPY_INPLACE src/qemu/qemu_capabilities.c | 37 ++++++++++++++++++------------------ src/util/viralloc.h | 22 ++++----------------- 2 files changed, 22 insertions(+), 37 deletions(-) -- 2.31.1

This simplyfies the code a bit and removes one "goto", one "VIR_FREE", and one "VIR_INSERT_ELEMENT_COPY". Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_capabilities.c | 37 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index d1cd8f11ac..04dae7d66e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -942,6 +942,7 @@ virQEMUCapsGetMachineTypesCaps(virQEMUCaps *qemuCaps, { size_t i; virQEMUCapsAccel *accel; + g_autoptr(GPtrArray) array = NULL; /* Guest capabilities do not report TCG vs. KVM caps separately. We just * take the set of machine types we probed first. */ @@ -953,13 +954,13 @@ virQEMUCapsGetMachineTypesCaps(virQEMUCaps *qemuCaps, *machines = NULL; *nmachines = accel->nmachineTypes; - if (*nmachines) - *machines = g_new0(virCapsGuestMachine *, accel->nmachineTypes); + if (*nmachines == 0) + return 0; + + array = g_ptr_array_sized_new(*nmachines); for (i = 0; i < accel->nmachineTypes; i++) { - virCapsGuestMachine *mach; - mach = g_new0(virCapsGuestMachine, 1); - (*machines)[i] = mach; + virCapsGuestMachine *mach = g_new0(virCapsGuestMachine, 1); if (accel->machineTypes[i].alias) { mach->name = g_strdup(accel->machineTypes[i].alias); mach->canonical = g_strdup(accel->machineTypes[i].name); @@ -968,6 +969,7 @@ virQEMUCapsGetMachineTypesCaps(virQEMUCaps *qemuCaps, } mach->maxCpus = accel->machineTypes[i].maxCpus; mach->deprecated = accel->machineTypes[i].deprecated; + g_ptr_array_add(array, mach); } /* Make sure all canonical machine types also have their own entry so that @@ -975,18 +977,19 @@ virQEMUCapsGetMachineTypesCaps(virQEMUCaps *qemuCaps, * supported machine types. */ i = 0; - while (i < *nmachines) { + while (i < array->len) { size_t j; bool found = false; - virCapsGuestMachine *machine = (*machines)[i]; + virCapsGuestMachine *machine = g_ptr_array_index(array, i); if (!machine->canonical) { i++; continue; } - for (j = 0; j < *nmachines; j++) { - if (STREQ(machine->canonical, (*machines)[j]->name)) { + for (j = 0; j < array->len; j++) { + virCapsGuestMachine *mach = g_ptr_array_index(array, j); + if (STREQ(machine->canonical, mach->name)) { found = true; break; } @@ -995,25 +998,21 @@ virQEMUCapsGetMachineTypesCaps(virQEMUCaps *qemuCaps, if (!found) { virCapsGuestMachine *mach; mach = g_new0(virCapsGuestMachine, 1); - if (VIR_INSERT_ELEMENT_COPY(*machines, i, *nmachines, mach) < 0) { - VIR_FREE(mach); - goto error; - } mach->name = g_strdup(machine->canonical); mach->maxCpus = machine->maxCpus; mach->deprecated = machine->deprecated; + g_ptr_array_insert(array, i, mach); i++; } i++; } - return 0; + *nmachines = array->len; + *machines = g_new0(virCapsGuestMachine *, accel->nmachineTypes); + for (i = 0; i < array->len; ++i) + (*machines)[i] = g_ptr_array_remove_index(array, i); - error: - virCapabilitiesFreeMachines(*machines, *nmachines); - *nmachines = 0; - *machines = NULL; - return -1; + return 0; } -- 2.31.1

On 7/8/21 4:28 PM, Tim Wiederhake wrote:
This simplyfies the code a bit and removes one "goto", one "VIR_FREE", and one "VIR_INSERT_ELEMENT_COPY".
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_capabilities.c | 37 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 19 deletions(-)
I'm not exactly sure what is causing this, but with this patch libvirtd crashes for me when I try to fetch capabilities: ==16567== Thread 3 rpc-worker: ==16567== Invalid read of size 8 ==16567== at 0x49CB01A: virCapabilitiesFormatGuestXML (capabilities.c:1259) ==16567== by 0x49CB6AB: virCapabilitiesFormatXML (capabilities.c:1355) ==16567== by 0xAE898B1: qemuConnectGetCapabilities (qemu_driver.c:1316) ==16567== by 0x4C47014: virConnectGetCapabilities (libvirt-host.c:467) ==16567== by 0x1328FD: remoteDispatchConnectGetCapabilities (remote_daemon_dispatch_stubs.h:766) ==16567== by 0x1328A5: remoteDispatchConnectGetCapabilitiesHelper (remote_daemon_dispatch_stubs.h:748) ==16567== by 0x4AB4C0F: virNetServerProgramDispatchCall (virnetserverprogram.c:428) ==16567== by 0x4AB478A: virNetServerProgramDispatch (virnetserverprogram.c:302) ==16567== by 0x4ABBE71: virNetServerProcessMsg (virnetserver.c:135) ==16567== by 0x4ABBF31: virNetServerHandleJob (virnetserver.c:152) ==16567== by 0x49AC6D5: virThreadPoolWorker (virthreadpool.c:159) ==16567== by 0x49ABBEB: virThreadHelper (virthread.c:241) ==16567== Address 0x8 is not stack'd, malloc'd or (recently) free'd Michal

On Fri, 2021-07-09 at 12:05 +0200, Michal Prívozník wrote:
On 7/8/21 4:28 PM, Tim Wiederhake wrote:
This simplyfies the code a bit and removes one "goto", one "VIR_FREE", and one "VIR_INSERT_ELEMENT_COPY".
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_capabilities.c | 37 ++++++++++++++++++---------------- -- 1 file changed, 18 insertions(+), 19 deletions(-)
I'm not exactly sure what is causing this, but with this patch libvirtd crashes for me when I try to fetch capabilities:
==16567== Thread 3 rpc-worker: ==16567== Invalid read of size 8 ==16567== at 0x49CB01A: virCapabilitiesFormatGuestXML (capabilities.c:1259) ==16567== by 0x49CB6AB: virCapabilitiesFormatXML (capabilities.c:1355) ==16567== by 0xAE898B1: qemuConnectGetCapabilities (qemu_driver.c:1316) ==16567== by 0x4C47014: virConnectGetCapabilities (libvirt- host.c:467) ==16567== by 0x1328FD: remoteDispatchConnectGetCapabilities (remote_daemon_dispatch_stubs.h:766) ==16567== by 0x1328A5: remoteDispatchConnectGetCapabilitiesHelper (remote_daemon_dispatch_stubs.h:748) ==16567== by 0x4AB4C0F: virNetServerProgramDispatchCall (virnetserverprogram.c:428) ==16567== by 0x4AB478A: virNetServerProgramDispatch (virnetserverprogram.c:302) ==16567== by 0x4ABBE71: virNetServerProcessMsg (virnetserver.c:135) ==16567== by 0x4ABBF31: virNetServerHandleJob (virnetserver.c:152) ==16567== by 0x49AC6D5: virThreadPoolWorker (virthreadpool.c:159) ==16567== by 0x49ABBEB: virThreadHelper (virthread.c:241) ==16567== Address 0x8 is not stack'd, malloc'd or (recently) free'd
Michal
Weird. Pipeline passed for me: https://gitlab.com/twiederh/libvirt/-/pipelines/333827544 Do you maybe have a reproducer for me, so I can investigate what's going on here? Thanks, Tim

On 7/9/21 1:05 PM, Tim Wiederhake wrote:
On Fri, 2021-07-09 at 12:05 +0200, Michal Prívozník wrote:
On 7/8/21 4:28 PM, Tim Wiederhake wrote:
This simplyfies the code a bit and removes one "goto", one "VIR_FREE", and one "VIR_INSERT_ELEMENT_COPY".
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_capabilities.c | 37 ++++++++++++++++++---------------- -- 1 file changed, 18 insertions(+), 19 deletions(-)
I'm not exactly sure what is causing this, but with this patch libvirtd crashes for me when I try to fetch capabilities:
==16567== Thread 3 rpc-worker: ==16567== Invalid read of size 8 ==16567== at 0x49CB01A: virCapabilitiesFormatGuestXML (capabilities.c:1259) ==16567== by 0x49CB6AB: virCapabilitiesFormatXML (capabilities.c:1355) ==16567== by 0xAE898B1: qemuConnectGetCapabilities (qemu_driver.c:1316) ==16567== by 0x4C47014: virConnectGetCapabilities (libvirt- host.c:467) ==16567== by 0x1328FD: remoteDispatchConnectGetCapabilities (remote_daemon_dispatch_stubs.h:766) ==16567== by 0x1328A5: remoteDispatchConnectGetCapabilitiesHelper (remote_daemon_dispatch_stubs.h:748) ==16567== by 0x4AB4C0F: virNetServerProgramDispatchCall (virnetserverprogram.c:428) ==16567== by 0x4AB478A: virNetServerProgramDispatch (virnetserverprogram.c:302) ==16567== by 0x4ABBE71: virNetServerProcessMsg (virnetserver.c:135) ==16567== by 0x4ABBF31: virNetServerHandleJob (virnetserver.c:152) ==16567== by 0x49AC6D5: virThreadPoolWorker (virthreadpool.c:159) ==16567== by 0x49ABBEB: virThreadHelper (virthread.c:241) ==16567== Address 0x8 is not stack'd, malloc'd or (recently) free'd
Michal
Weird. Pipeline passed for me: https://gitlab.com/twiederh/libvirt/-/pipelines/333827544
Do you maybe have a reproducer for me, so I can investigate what's going on here?
All I did was 'virsh capabilities'. I don't think that's something that our CI tests because we mostly construct capabilities structure from scratch. Michal

There are no users left. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/viralloc.h | 22 ++++------------------ 1 file changed, 4 insertions(+), 18 deletions(-) diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 553d2951cf..72e8f13bef 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -165,7 +165,7 @@ int virDeleteElementsN(void *ptrptr, size_t size, size_t at, size_t *countptr, sizeof(char[2 * (sizeof(*(a) = *(b)) == sizeof(*(b))) - 1]) /** - * VIR_INSERT_ELEMENT: + * VIR_INSERT_ELEMENT, VIR_INSERT_ELEMENT_INPLACE: * @ptr: pointer to array of objects (*not* ptr to ptr) * @at: index within array where new elements should be added * @count: variable tracking number of elements currently allocated @@ -179,18 +179,10 @@ int virDeleteElementsN(void *ptrptr, size_t size, size_t at, size_t *countptr, * item 'newelem' into ptr[at], then store the address of * allocated memory in 'ptr' and the new size in 'count'. * - * VIR_INSERT_ELEMENT_COPY is identical, but doesn't clear out the - * original element to 0 on success, so there are two copies of the - * element. This is useful if the "element" is actually just a - * pointer to the real data, and you want to maintain a reference to - * it for use after the insert is completed; but if the "element" is - * an object that points to other allocated memory, having multiple - * copies can cause problems (e.g. double free). + * VIR_INSERT_ELEMENT_INPLACE is identical, but assumes any necessary + * memory re-allocation has already been done. * - * VIR_INSERT_ELEMENT_*INPLACE are identical, but assume any necessary - * memory re-allocation has already been done. - * - * VIR_INSERT_ELEMENT_* all need to send "1" as the "add" argument to + * Both functions need to send "1" as the "add" argument to * virInsertElementsN (which has the currently-unused capability of * inserting multiple items at once). We use this to our advantage by * replacing it with VIR_TYPECHECK(ptr, &newelem) so that we can be @@ -203,15 +195,9 @@ int virDeleteElementsN(void *ptrptr, size_t size, size_t at, size_t *countptr, #define VIR_INSERT_ELEMENT(ptr, at, count, newelem) \ virInsertElementsN(&(ptr), sizeof(*(ptr)), at, &(count), \ VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), true, false) -#define VIR_INSERT_ELEMENT_COPY(ptr, at, count, newelem) \ - virInsertElementsN(&(ptr), sizeof(*(ptr)), at, &(count), \ - VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), false, false) #define VIR_INSERT_ELEMENT_INPLACE(ptr, at, count, newelem) \ virInsertElementsN(&(ptr), sizeof(*(ptr)), at, &(count), \ VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), true, true) -#define VIR_INSERT_ELEMENT_COPY_INPLACE(ptr, at, count, newelem) \ - virInsertElementsN(&(ptr), sizeof(*(ptr)), at, &(count), \ - VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), false, true) /** * VIR_APPEND_ELEMENT: -- 2.31.1
participants (2)
-
Michal Prívozník
-
Tim Wiederhake