src/conf/domain_conf.c | 13 ++--- src/conf/snapshot_conf.c | 3 +- src/qemu/qemu_blockjob.c | 43 ++++++-------- src/qemu/qemu_domain.c | 6 +- src/qemu/qemu_driver.c | 14 ++--- src/qemu/qemu_hotplug.c | 3 +- src/qemu/qemu_migration.c | 2 +- src/storage/storage_backend_gluster.c | 2 +- src/storage/storage_util.c | 4 +- src/util/virstoragefile.c | 84 +++++++++++++-------------- src/util/virstoragefile.h | 9 ++- tests/qemublocktest.c | 6 +- tests/virstoragetest.c | 12 ++-- 13 files changed, 98 insertions(+), 103 deletions(-)
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. Daniel Henrique Barboza (1): util: convert virStorageSource class to use GObject src/conf/domain_conf.c | 13 ++--- src/conf/snapshot_conf.c | 3 +- src/qemu/qemu_blockjob.c | 43 ++++++-------- src/qemu/qemu_domain.c | 6 +- src/qemu/qemu_driver.c | 14 ++--- src/qemu/qemu_hotplug.c | 3 +- src/qemu/qemu_migration.c | 2 +- src/storage/storage_backend_gluster.c | 2 +- src/storage/storage_util.c | 4 +- src/util/virstoragefile.c | 84 +++++++++++++-------------- src/util/virstoragefile.h | 9 ++- tests/qemublocktest.c | 6 +- tests/virstoragetest.c | 12 ++-- 13 files changed, 98 insertions(+), 103 deletions(-) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
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é <berrange@redhat.com>
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
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é <berrange@redhat.com> > 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
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é <berrange@redhat.com> > > 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
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é <berrange@redhat.com> >>> 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
© 2016 - 2024 Red Hat, Inc.