8:43 a.m.
On Wed, Oct 16, 2019 at 09:24:13AM -0300, Daniel Henrique Barboza wrote:
I just realized by reading a patch from Jano that VIR_AUTOFREE
is simply an alias for g_autofree. This means that it is not worth
to add more VIR_AUTOFREE() macros, and that it is ok to mix
g_autofree with other VIR_ macros such as VIR_AUTOUNREF().
I deleted VIR_AUTOUNREF already. All usage of VIR_AUTOFREE was also
removed, but I missed that the macro is definied in viralloc.h,
patch removing it is pending.
Per our HACKING style:
https://libvirt.org/hacking.html#glib
while the alloc/free functions should be interchangeable, it is bad
style to mix the VIR_ and g_ allocation macros in a single function.
I converted the VIR_AUTO_* group that had a GLib alternative,
leaving VIR_AUTOSTRINGLIST and VIR_AUTOCLOSE behind.
But to convert VIR_APPEND_ELEMENT usage a more fine grained approach
will be needed, since it might involve using the GLib container types.
Jano is
also removing VIR_AUTOFREE() from the code in his series, which
will make this whole series obsolete.
I'll re-send this series to not add more VIR_AUTOFREE() in any
step of the way.
DHB
On 10/15/19 5:08 PM, Daniel Henrique Barboza wrote:
>changes from v2:
>- rebased with newer master (67e72053c1)
>- added an extra patch to convert the existing VIR_AUTO* macros
>to g_auto* ones, all at once, to avoid the case where a method
>will have both VIR_AUTO* and g_auto* macros at the same time.
>
>
>Note: the conversion in patch 10 wasn't 100% due to a handful of
>methods that I was unable to use g_autoptr. Take for example
>the method qemuDomainSaveInternal:
>--
> qemuDomainObjPrivatePtr priv = vm->privateData;
> VIR_AUTOUNREF(virCapsPtr) caps = NULL;
> virQEMUSaveDataPtr data = NULL;
> VIR_AUTOUNREF(qemuDomainSaveCookiePtr) cookie = NULL;
>--
>
>Changing the 'cookie' variable to use g_autoptr() causes an error:
>
>make[3]: Entering directory '/home/danielhb/kvm-project/libvirt/src'
> CC qemu/libvirt_driver_qemu_impl_la-qemu_driver.lo
>In file included from /usr/lib64/glib-2.0/include/glibconfig.h:9,
> from /usr/include/glib-2.0/glib/gtypes.h:32,
> from /usr/include/glib-2.0/glib/galloca.h:32,
> from /usr/include/glib-2.0/glib.h:30,
> from ./internal.h:31,
> from qemu/qemu_agent.h:24,
> from qemu/qemu_driver.c:40:
>qemu/qemu_driver.c: In function 'qemuDomainSaveInternal':
>qemu/qemu_driver.c:3282:15: error: unknown type name
'qemuDomainSaveCookie_autoptr'
> 3282 | g_autoptr(qemuDomainSaveCookie) cookie = NULL;
>
IIUC the original plan was to use VIR_AUTOPTR for the types needing
unref so the cleanup function was defined for them, but then
VIR_AUTOUNREF was introduced which always used virObjectUnref.
To use g_autoptr with other types, you need to define the CLEANUP
function, see:
commit df4986b51bc88b65ae7f453814963e4340b168f3
Define G_DEFINE_AUTOPTR_CLEANUP_FUNC for virDomainCheckpointDef
My objective was to get rid of the VIR_AUTOUNREF usage, not sure
whether introducing g_autoptr for already existing objects until
we convert them to GObject.
>
>I tried doing it with the 'Ptr' in the name, same error:
>
>
> CC qemu/libvirt_driver_qemu_impl_la-qemu_driver.lo
>In file included from /usr/lib64/glib-2.0/include/glibconfig.h:9,
> from /usr/include/glib-2.0/glib/gtypes.h:32,
> from /usr/include/glib-2.0/glib/galloca.h:32,
> from /usr/include/glib-2.0/glib.h:30,
> from ./internal.h:31,
> from qemu/qemu_agent.h:24,
> from qemu/qemu_driver.c:40:
>qemu/qemu_driver.c: In function 'qemuDomainSaveInternal':
>qemu/qemu_driver.c:3282:15: error: unknown type name
'qemuDomainSaveCookiePtr_autoptr'
> 3282 | g_autoptr(qemuDomainSaveCookiePtr) cookie = NULL;
>
>
>Similar situation happens with qemuDomainSaveImageStartVM and with
>qemuSecurityInit methods. They are mentioned in the commit message
>of patch 10 for reference. These methods are still using VIR_AUTO*
>macros.
>
>
>changes from v1:
>- addressed review concerns made by Erik
>- added more cleanups, as suggested by Erik
>
>v2: https://www.redhat.com/archives/libvir-list/2019-September/msg01452.html
>v1: https://www.redhat.com/archives/libvir-list/2019-September/msg00719.html
>
>Daniel Henrique Barboza (10):
> qemu_driver: use VIR_AUTOUNREF() with virQEMUDriverConfigPtr 1/3
> qemu_driver: use VIR_AUTOUNREF() with virQEMUDriverConfigPtr 2/3
> qemu_driver: use VIR_AUTOUNREF() with virQEMUDriverConfigPtr 3/3
> qemu_driver: use VIR_AUTOUNREF() with virCapsPtr
> qemu_driver: use VIR_AUTOUNREF() with more pointer types
> qemu_driver: use VIR_AUTOFREE() with strings 1/3
> qemu_driver: use VIR_AUTOFREE() with strings 2/3
> qemu_driver: use VIR_AUTOFREE() with strings 3/3
> qemu_driver: VIR_AUTOFREE on other scalar pointers
> qemu_driver.c: use GLib macros
Fixing newly added lines seems odd, especially since you'll be replacing
the exact text that you added earlier.
Jano