Re: [PATCH 00/40] convert virObjects to GObject
On Wed, Jun 3, 2020 at 11:38 AM Daniel P. Berrangé wrote: > > On Fri, May 15, 2020 at 04:13:21PM +0100, Daniel P. Berrangé wrote: > > On Fri, May 15, 2020 at 02:34:01PM +0200, Michal Privoznik wrote: > > > On 5/13/20 1:56 PM, Rafael Fonseca wrote: > > > > This patch series convert various simple instances of virObject to a > > > > GObject equivalent. > > > > > > I'm sorry upfront for misusing your patchset and I'm also sorry for > > > bringing > > > this up again. > > > > > > I think we need to step back and re-evaluate whether this is worth it. > > > GObject is horrible and frightening part of GLib. Not only one has to > > > define > > > empty functions, but we can't mix virObject and GObject. For instance: > > > virObjectRef() called over GObject will corrupt memory. > > > > > > Worse, there is no way to check whether your patches converted everything > > > (and clearly they did not because I see couple of tests crashing; or maybe > > > they did at the time of send but now the code has changed - anyway, point > > > proven). > > > > > > I started reviewing and stopped at the first patch realizing, I have no > > > idea > > > whether you converted every virObjectRef()/virObjectUnref() with > > > corresponding glib call. > > > > > > I also wanted to write a cocci spatch that might at least identify places > > > where that is happening, but apparently my spatch skills are poor. > > > > > > Does anybody have an idea how to verify these patches? > > > > My preferred option was to make virObject be a subclass of GObject. > > > > I tried this initially but hit a key problem - g_object_unref is void > > and virObjectUnref returns bool - true if any refs still exist. We > > rely on that for the virConnectPtr and virFDStream objects. > > > > I couldn't come up with a way to solve that at the time, but I've > > just had another think and believe we can solve it using a thread > > local set by the finalize. So I'll have another go at doing this > > inheritance. > > My conversion to make virObject a subclass of GObject has now merged, > which eliminates the danger that Michael was concerned about. > > So we should be able to move forward with Rafael's patches now. Cool. I'll rebase on master, run the CI and send a new version when it's all green. Att. -- Rafael Fonseca
Re: [PATCH 00/40] convert virObjects to GObject
On Fri, May 15, 2020 at 04:13:21PM +0100, Daniel P. Berrangé wrote: > On Fri, May 15, 2020 at 02:34:01PM +0200, Michal Privoznik wrote: > > On 5/13/20 1:56 PM, Rafael Fonseca wrote: > > > This patch series convert various simple instances of virObject to a > > > GObject equivalent. > > > > I'm sorry upfront for misusing your patchset and I'm also sorry for bringing > > this up again. > > > > I think we need to step back and re-evaluate whether this is worth it. > > GObject is horrible and frightening part of GLib. Not only one has to define > > empty functions, but we can't mix virObject and GObject. For instance: > > virObjectRef() called over GObject will corrupt memory. > > > > Worse, there is no way to check whether your patches converted everything > > (and clearly they did not because I see couple of tests crashing; or maybe > > they did at the time of send but now the code has changed - anyway, point > > proven). > > > > I started reviewing and stopped at the first patch realizing, I have no idea > > whether you converted every virObjectRef()/virObjectUnref() with > > corresponding glib call. > > > > I also wanted to write a cocci spatch that might at least identify places > > where that is happening, but apparently my spatch skills are poor. > > > > Does anybody have an idea how to verify these patches? > > My preferred option was to make virObject be a subclass of GObject. > > I tried this initially but hit a key problem - g_object_unref is void > and virObjectUnref returns bool - true if any refs still exist. We > rely on that for the virConnectPtr and virFDStream objects. > > I couldn't come up with a way to solve that at the time, but I've > just had another think and believe we can solve it using a thread > local set by the finalize. So I'll have another go at doing this > inheritance. My conversion to make virObject a subclass of GObject has now merged, which eliminates the danger that Michael was concerned about. So we should be able to move forward with Rafael's patches now. 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 :|
Re: [PATCH 00/40] convert virObjects to GObject
On Fri, May 15, 2020 at 02:34:01PM +0200, Michal Privoznik wrote: > On 5/13/20 1:56 PM, Rafael Fonseca wrote: > > This patch series convert various simple instances of virObject to a > > GObject equivalent. > > I'm sorry upfront for misusing your patchset and I'm also sorry for bringing > this up again. > > I think we need to step back and re-evaluate whether this is worth it. > GObject is horrible and frightening part of GLib. Not only one has to define > empty functions, but we can't mix virObject and GObject. For instance: > virObjectRef() called over GObject will corrupt memory. > > Worse, there is no way to check whether your patches converted everything > (and clearly they did not because I see couple of tests crashing; or maybe > they did at the time of send but now the code has changed - anyway, point > proven). > > I started reviewing and stopped at the first patch realizing, I have no idea > whether you converted every virObjectRef()/virObjectUnref() with > corresponding glib call. > > I also wanted to write a cocci spatch that might at least identify places > where that is happening, but apparently my spatch skills are poor. > > Does anybody have an idea how to verify these patches? My preferred option was to make virObject be a subclass of GObject. I tried this initially but hit a key problem - g_object_unref is void and virObjectUnref returns bool - true if any refs still exist. We rely on that for the virConnectPtr and virFDStream objects. I couldn't come up with a way to solve that at the time, but I've just had another think and believe we can solve it using a thread local set by the finalize. So I'll have another go at doing this inheritance. 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 :|
Re: [PATCH 00/40] convert virObjects to GObject
On 5/13/20 1:56 PM, Rafael Fonseca wrote: This patch series convert various simple instances of virObject to a GObject equivalent. I'm sorry upfront for misusing your patchset and I'm also sorry for bringing this up again. I think we need to step back and re-evaluate whether this is worth it. GObject is horrible and frightening part of GLib. Not only one has to define empty functions, but we can't mix virObject and GObject. For instance: virObjectRef() called over GObject will corrupt memory. Worse, there is no way to check whether your patches converted everything (and clearly they did not because I see couple of tests crashing; or maybe they did at the time of send but now the code has changed - anyway, point proven). I started reviewing and stopped at the first patch realizing, I have no idea whether you converted every virObjectRef()/virObjectUnref() with corresponding glib call. I also wanted to write a cocci spatch that might at least identify places where that is happening, but apparently my spatch skills are poor. Does anybody have an idea how to verify these patches? Michal
[PATCH 00/40] convert virObjects to GObject
This patch series convert various simple instances of virObject to a GObject equivalent. virLockableObject and virObjects which are subclassed will be covered in future patchsets. New in v3: - merge *Dispose back into *Finalize - rebased on top of master Rafael Fonseca (40): util: virresctrl: convert classes to GObject conf: capabilities: convert virCaps to GOBject qemu: convert virQEMUCaps to GObject util: add function to unref array of GObjects rpc: convert virNetClientProgram to GObject rpc: convert virNetServerProgram to GObject conf: convert virDomainXMLOption to GObject bhyve: convert bhyveMonitor to GObject bhyve: convert virBhyveDriverConfig to GObject rpc: convert virNetServerService to GObject conf: convert virDomainCapsCPUModels to GObject util: convert dnsmasqCaps to GObject conf: convert virDomainChrSourceDef to GObject rpc: gendispatch: prepare for GObject conversion admin: convert virAdmServer to GObject admin: convert virAdmClient to GObject datatypes: convert virDomainCheckpoint to GObject datatypes: convert virDomainSnapshot to GObject datatypes: convert virNWFilter to GObject datatypes: convert virNWFilterBinding to GObject datatypes: convert virNetwork to GObject datatypes: convert virNetworkPort to GObject datatypes: convert virInterface to GObject datatypes: convert virStoragePool to GObject datatypes: convert virStorageVol to GObject datatypes: convert virNodeDevice to GObject datatypes: convert virSecret to GObject datatypes: convert virStream to GObject datatypes: convert virDomain to GObject rpc: gendispatch: use g_autoptr where possible conf: convert virNetworkXMLOption to GObject lxc: convert virLXCDriverConfig to GObject libxl: convert libxlDriverConfig to GObject hypervisor: convert virHostdevManager to GObject libxl: convert libxlMigrationDstArgs to GObject qemu: convert qemuBlockJobData to GObject qemu: convert virQEMUDriverConfig to GObject conf: convert virDomain*Private to GObject conf: convert virSaveCookie to GObject util: convert virStorageSource to GObject src/admin/admin_remote.c| 9 +- src/admin/libvirt-admin.c | 4 +- src/admin/libvirt_admin_private.syms| 4 +- src/bhyve/bhyve_capabilities.c | 19 +- src/bhyve/bhyve_conf.c | 36 +- src/bhyve/bhyve_driver.c| 33 +- src/bhyve/bhyve_monitor.c | 37 +- src/bhyve/bhyve_monitor.h | 3 +- src/bhyve/bhyve_utils.h | 11 +- src/conf/capabilities.c | 41 +- src/conf/capabilities.h | 6 +- src/conf/domain_capabilities.c | 48 +- src/conf/domain_capabilities.h | 10 +- src/conf/domain_conf.c | 117 ++--- src/conf/domain_conf.h | 36 +- src/conf/domain_event.c | 58 +-- src/conf/network_conf.c | 24 +- src/conf/network_conf.h | 12 +- src/conf/network_event.c| 7 +- src/conf/node_device_event.c| 11 +- src/conf/node_device_util.c | 4 +- src/conf/secret_event.c | 15 +- src/conf/snapshot_conf.c| 2 +- src/conf/snapshot_conf.h| 2 +- src/conf/storage_capabilities.c | 4 +- src/conf/storage_event.c| 15 +- src/conf/virchrdev.c| 4 +- src/conf/virconftypes.h | 3 +- src/conf/virdomaincheckpointobjlist.c | 7 +- src/conf/virdomainsnapshotobjlist.c | 7 +- src/conf/virinterfaceobj.c | 5 +- src/conf/virnetworkobj.c| 10 +- src/conf/virnodedeviceobj.c | 3 +- src/conf/virnwfilterbindingobjlist.c| 2 +- src/conf/virnwfilterobj.c | 7 +- src/conf/virsavecookie.c| 10 +- src/conf/virsavecookie.h| 14 +- src/conf/virsecretobj.c | 2 +- src/conf/virstorageobj.c| 4 +- src/datatypes.c | 658 +++- src/datatypes.h | 219 src/esx/esx_driver.c| 33 +- src/hyperv/hyperv_driver.c | 8 +- src/hypervisor/virhostdev.c | 31 +- src/hypervisor/virhostdev.h | 13 +- src/interface/interface_backend_netcf.c | 7 +- src/interface/interface_backend_udev.c | 8 +- src/libvirt-domain-checkpoint.c | 7 +- src/libvirt-domain-snapshot.c | 7 +- src/libvirt-domain.c| 6 +- src/libvirt-interface.c | 6 +- src/libvirt-network.c | 14 +- src/libvirt-nodedev.c | 6 +- src/libvirt-nwfilter.c | 14 +- src/libvirt-secret.c| 7 +- src/libvirt-storage.c | 12 +- src/libvirt-stream.c| 7