Re: [libvirt] [RFC 0/1] convert virStorageSource to GObject
On 10/15/19 2:15 PM, Daniel P. Berrangé wrote: On Tue, Oct 15, 2019 at 10:51:39AM -0300, Daniel Henrique Barboza wrote: On 10/15/19 9:55 AM, Daniel P. Berrangé wrote: On Tue, Oct 15, 2019 at 09:42:45AM -0300, Daniel Henrique Barboza wrote: I was hoping to quickly re-send the qemu_driver cleanups I've sent some time ago, now using Glib. I started by attempting to change the first VIR_AUTOUNREF() call in qemu_driver.c to g_autoptr(), which happens to be a virStorageSourcePtr type, then I realized that it wasn't that simple. It should be that simple with this commit: commit 667ff797e8eb8d82f30ab430216a8d2eef6b915a Author: Daniel P. Berrangé Date: Fri Oct 4 17:14:10 2019 +0100 src: add support for g_autoptr with virObject instances we should be able to use g_autoptr for any virObject, without having to lock-step convert to GObject. What actual problem did you find ? I failed to notice this commit. Just tried it again and it worked. What happened yesterday was that I attempted to do a simple VIR_AUTOUNREF -> g_autopt replace, faced compile errors and then, since I didn't notice this commit about, I assumed "I guess I need to convert this guy to GObject". In fact, the compile error happened because g_autoptr() does not operate with a 'Ptr' type - something that I learned only during the conversion process. Yeah, you need to drop the 'Ptr' suffix in the type name when converting to g_autoptr, as it adds the pointer itself. This is being sent as RFC because x-I am aware that docs/hacking.html mentions that we shouldn't mix up certain GLib macros with Libvirt ones, thus I am uncertain of whether I have messed up or not. 'make check' works, did a few sanity checks with libvirtd as well. Yes, the need to not mix g_auto* with VIR_AUTO*, is why I did commit 667ff797e8eb8d82f30ab430216a8d2eef6b915a to let you use g_autoptr with virObject, without first converting to GObject. What if there are other object types in the same function using the VIR macros? For example, inside qemu_driver.c: qemuDomainBlockCopyCommon: VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); const char *format = NULL; bool mirror_reuse = !!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT); bool mirror_shallow = !!(flags & VIR_DOMAIN_BLOCK_COPY_SHALLOW); bool existing = mirror_reuse; qemuBlockJobDataPtr job = NULL; VIR_AUTOUNREF(virStorageSourcePtr) mirror = mirrorsrc; bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); bool mirror_initialized = false; VIR_AUTOPTR(qemuBlockStorageSourceChainData) data = NULL; VIR_AUTOPTR(qemuBlockStorageSourceChainData) crdata = NULL; Let's say that I change the virStorageSourcePtr up there to g_autoptr(virStorageSource) mirror = mirrorsrc; As long as there are no VIR macros acting in the 'mirror' variable, is it to use g_autoptr there even when everyone else is using VIR_AUTO* macros? You should change all variables in the method at the same time. Both the VIR_AUTOUNEF calls here can use g_autoptr, as can the two VIR_AUTOPTR calls. Thanks for clarifying. I was going to re-send the patches adding GLib macros instead of VIR_AUTO* ones, which would end up breaking this rule because these patches are changing stuff in smaller steps. What I'll end up is to basically re-send them as they are now, but with an extra patch to change everything to GLib at once. That way we'll stay compliant every step of the way. DHB Regards, Daniel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC 0/1] convert virStorageSource to GObject
On Tue, Oct 15, 2019 at 10:51:39AM -0300, Daniel Henrique Barboza wrote: > > > On 10/15/19 9:55 AM, Daniel P. Berrangé wrote: > > On Tue, Oct 15, 2019 at 09:42:45AM -0300, Daniel Henrique Barboza wrote: > > > I was hoping to quickly re-send the qemu_driver cleanups I've > > > sent some time ago, now using Glib. I started by attempting to > > > change the first VIR_AUTOUNREF() call in qemu_driver.c to > > > g_autoptr(), which happens to be a virStorageSourcePtr type, > > > then I realized that it wasn't that simple. > > It should be that simple with this commit: > > > >commit 667ff797e8eb8d82f30ab430216a8d2eef6b915a > >Author: Daniel P. Berrangé > >Date: Fri Oct 4 17:14:10 2019 +0100 > > > > src: add support for g_autoptr with virObject instances > > > > we should be able to use g_autoptr for any virObject, without > > having to lock-step convert to GObject. > > > > What actual problem did you find ? > > I failed to notice this commit. Just tried it again and it worked. > > What happened yesterday was that I attempted to do a simple > VIR_AUTOUNREF -> g_autopt replace, faced compile errors and then, since > I didn't notice this commit about, I assumed "I guess I need to convert > this guy to GObject". > > In fact, the compile error happened because g_autoptr() does not operate > with a 'Ptr' type - something that I learned only during the conversion > process. Yeah, you need to drop the 'Ptr' suffix in the type name when converting to g_autoptr, as it adds the pointer itself. > > > This is being sent as RFC because x-I am aware that docs/hacking.html > > > mentions that we shouldn't mix up certain GLib macros with Libvirt > > > ones, thus I am uncertain of whether I have messed up or not. > > > 'make check' works, did a few sanity checks with libvirtd as > > > well. > > Yes, the need to not mix g_auto* with VIR_AUTO*, is why I did commit > > 667ff797e8eb8d82f30ab430216a8d2eef6b915a to let you use g_autoptr > > with virObject, without first converting to GObject. > > What if there are other object types in the same function using the VIR > macros? > For example, inside qemu_driver.c: qemuDomainBlockCopyCommon: > > > VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = > virQEMUDriverGetConfig(driver); > const char *format = NULL; > bool mirror_reuse = !!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT); > bool mirror_shallow = !!(flags & VIR_DOMAIN_BLOCK_COPY_SHALLOW); > bool existing = mirror_reuse; > qemuBlockJobDataPtr job = NULL; > VIR_AUTOUNREF(virStorageSourcePtr) mirror = mirrorsrc; > bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); > bool mirror_initialized = false; > VIR_AUTOPTR(qemuBlockStorageSourceChainData) data = NULL; > VIR_AUTOPTR(qemuBlockStorageSourceChainData) crdata = NULL; > > > Let's say that I change the virStorageSourcePtr up there to > > g_autoptr(virStorageSource) mirror = mirrorsrc; > > > As long as there are no VIR macros acting in the 'mirror' variable, is it to > use g_autoptr > there even when everyone else is using VIR_AUTO* macros? You should change all variables in the method at the same time. Both the VIR_AUTOUNEF calls here can use g_autoptr, as can the two VIR_AUTOPTR calls. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC 0/1] convert virStorageSource to GObject
On 10/15/19 9:55 AM, Daniel P. Berrangé wrote: On Tue, Oct 15, 2019 at 09:42:45AM -0300, Daniel Henrique Barboza wrote: I was hoping to quickly re-send the qemu_driver cleanups I've sent some time ago, now using Glib. I started by attempting to change the first VIR_AUTOUNREF() call in qemu_driver.c to g_autoptr(), which happens to be a virStorageSourcePtr type, then I realized that it wasn't that simple. It should be that simple with this commit: commit 667ff797e8eb8d82f30ab430216a8d2eef6b915a Author: Daniel P. Berrangé Date: Fri Oct 4 17:14:10 2019 +0100 src: add support for g_autoptr with virObject instances we should be able to use g_autoptr for any virObject, without having to lock-step convert to GObject. What actual problem did you find ? I failed to notice this commit. Just tried it again and it worked. What happened yesterday was that I attempted to do a simple VIR_AUTOUNREF -> g_autopt replace, faced compile errors and then, since I didn't notice this commit about, I assumed "I guess I need to convert this guy to GObject". In fact, the compile error happened because g_autoptr() does not operate with a 'Ptr' type - something that I learned only during the conversion process. Well, hopefully this patch can serve as a baseline for a future conversion for this object type. Guess I can go back safely to re-send the cleanup patches tha are already pending in the ML hehehe Following up the instructions found on commit 16121a88a7, I started the conversion. Then 'make check' started to fail because some calls to virObjectRef/virObjectUnref were still remaining in the code, messing up stuff related with mirrorChain in qemu_blockjob.c. Turns out it was easier to burn through all the instances and change them to use GLib. Yes, if you convert from virObject to GObject, you *must* convert all virObjectRef/Unref calls at that time. This is being sent as RFC because x-I am aware that docs/hacking.html mentions that we shouldn't mix up certain GLib macros with Libvirt ones, thus I am uncertain of whether I have messed up or not. 'make check' works, did a few sanity checks with libvirtd as well. Yes, the need to not mix g_auto* with VIR_AUTO*, is why I did commit 667ff797e8eb8d82f30ab430216a8d2eef6b915a to let you use g_autoptr with virObject, without first converting to GObject. What if there are other object types in the same function using the VIR macros? For example, inside qemu_driver.c: qemuDomainBlockCopyCommon: VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); const char *format = NULL; bool mirror_reuse = !!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT); bool mirror_shallow = !!(flags & VIR_DOMAIN_BLOCK_COPY_SHALLOW); bool existing = mirror_reuse; qemuBlockJobDataPtr job = NULL; VIR_AUTOUNREF(virStorageSourcePtr) mirror = mirrorsrc; bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); bool mirror_initialized = false; VIR_AUTOPTR(qemuBlockStorageSourceChainData) data = NULL; VIR_AUTOPTR(qemuBlockStorageSourceChainData) crdata = NULL; Let's say that I change the virStorageSourcePtr up there to g_autoptr(virStorageSource) mirror = mirrorsrc; As long as there are no VIR macros acting in the 'mirror' variable, is it to use g_autoptr there even when everyone else is using VIR_AUTO* macros? Thanks, DHB Regards, Daniel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC 0/1] convert virStorageSource to GObject
On Tue, Oct 15, 2019 at 09:42:45 -0300, Daniel Henrique Barboza wrote: > I was hoping to quickly re-send the qemu_driver cleanups I've > sent some time ago, now using Glib. I started by attempting to > change the first VIR_AUTOUNREF() call in qemu_driver.c to > g_autoptr(), which happens to be a virStorageSourcePtr type, > then I realized that it wasn't that simple. > > Following up the instructions found on commit 16121a88a7, I started > the conversion. Then 'make check' started to fail because some > calls to virObjectRef/virObjectUnref were still remaining > in the code, messing up stuff related with mirrorChain in > qemu_blockjob.c. Turns out it was easier to burn through all the > instances and change them to use GLib. > > This is being sent as RFC because x-I am aware that docs/hacking.html > mentions that we shouldn't mix up certain GLib macros with Libvirt > ones, thus I am uncertain of whether I have messed up or not. > 'make check' works, did a few sanity checks with libvirtd as > well. > > Hopefully this is somewhere near the mark. I intend to do such > convertions from time to time, based on the cleanups I wanted to > make in the qemu_driver file prior to the GLib introduction. I'd prefer if this kind of experiments is done on something simpler than virStorageSource. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC 0/1] convert virStorageSource to GObject
On Tue, Oct 15, 2019 at 09:42:45AM -0300, Daniel Henrique Barboza wrote: > I was hoping to quickly re-send the qemu_driver cleanups I've > sent some time ago, now using Glib. I started by attempting to > change the first VIR_AUTOUNREF() call in qemu_driver.c to > g_autoptr(), which happens to be a virStorageSourcePtr type, > then I realized that it wasn't that simple. It should be that simple with this commit: commit 667ff797e8eb8d82f30ab430216a8d2eef6b915a Author: Daniel P. Berrangé Date: Fri Oct 4 17:14:10 2019 +0100 src: add support for g_autoptr with virObject instances we should be able to use g_autoptr for any virObject, without having to lock-step convert to GObject. What actual problem did you find ? > Following up the instructions found on commit 16121a88a7, I started > the conversion. Then 'make check' started to fail because some > calls to virObjectRef/virObjectUnref were still remaining > in the code, messing up stuff related with mirrorChain in > qemu_blockjob.c. Turns out it was easier to burn through all the > instances and change them to use GLib. Yes, if you convert from virObject to GObject, you *must* convert all virObjectRef/Unref calls at that time. > This is being sent as RFC because x-I am aware that docs/hacking.html > mentions that we shouldn't mix up certain GLib macros with Libvirt > ones, thus I am uncertain of whether I have messed up or not. > 'make check' works, did a few sanity checks with libvirtd as > well. Yes, the need to not mix g_auto* with VIR_AUTO*, is why I did commit 667ff797e8eb8d82f30ab430216a8d2eef6b915a to let you use g_autoptr with virObject, without first converting to GObject. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
