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 +-
 src/libvirt_private.syms                |  35 +-
 src/libxl/libxl_capabilities.c          |  19 +-
 src/libxl/libxl_conf.c                  |  56 +-
 src/libxl/libxl_conf.h                  |  12 +-
 src/libxl/libxl_driver.c                | 175 +++----
 src/libxl/libxl_migration.c             |  92 ++--
 src/libxl/xen_common.c                  |   3 +-
 src/locking/lock_daemon.c               |  28 +-
 src/locking/lock_driver_lockd.c         |  19 +-
 src/locking/sanlock_helper.c            |   5 +-
 src/logging/log_daemon.c                |  28 +-
 src/logging/log_manager.c               |  15 +-
 src/lxc/lxc_conf.c                      |  46 +-
 src/lxc/lxc_conf.h                      |  10 +-
 src/lxc/lxc_controller.c                |  20 +-
 src/lxc/lxc_driver.c                    |  98 ++--
 src/lxc/lxc_monitor.c                   |   2 +-
 src/lxc/lxc_process.c                   |  35 +-
 src/network/bridge_driver.c             |  24 +-
 src/nwfilter/nwfilter_driver.c          |   3 +-
 src/openvz/openvz_conf.c                |   8 +-
 src/qemu/qemu_blockjob.c                | 119 ++---
 src/qemu/qemu_blockjob.h                |  15 +-
 src/qemu/qemu_capabilities.c            | 165 +++---
 src/qemu/qemu_capabilities.h            |   9 +-
 src/qemu/qemu_conf.c                    |  42 +-
 src/qemu/qemu_conf.h                    |  19 +-
 src/qemu/qemu_domain.c                  | 436 +++++++---------
 src/qemu/qemu_domain.h                  | 157 ++++--
 src/qemu/qemu_driver.c                  |  34 +-
 src/qemu/qemu_hotplug.c                 |   3 +-
 src/qemu/qemu_migration.c               |  34 +-
 src/qemu/qemu_process.c                 |  20 +-
 src/qemu/qemu_virtiofs.c                |  12 +-
 src/remote/remote_daemon.c              |  52 +-
 src/remote/remote_daemon_dispatch.c     | 188 +++----
 src/remote/remote_daemon_stream.c       |   6 +-
 src/remote/remote_driver.c              | 178 +++----
 src/rpc/gendispatch.pl                  |  90 ++--
 src/rpc/virnetclient.c                  |   7 +-
 src/rpc/virnetclientprogram.c           |  29 +-
 src/rpc/virnetclientprogram.h           |  10 +-
 src/rpc/virnetclientstream.c            |   4 +-
 src/rpc/virnetserver.c                  |  21 +-
 src/rpc/virnetserverprogram.c           |  30 +-
 src/rpc/virnetserverprogram.h           |  17 +-
 src/rpc/virnetserverservice.c           |  87 ++--
 src/security/virt-aa-helper.c           |   9 +-
 src/storage/storage_backend.c           |   3 +-
 src/storage/storage_driver.c            |   7 +-
 src/storage/storage_util.c              |   4 +-
 src/test/test_driver.c                  |  33 +-
 src/util/virdnsmasq.c                   |  56 +-
 src/util/virdnsmasq.h                   |   6 +-
 src/util/virfdstream.c                  |   4 +-
 src/util/virfilecache.c                 |  43 +-
 src/util/virfilecache.h                 |  12 +-
 src/util/virobject.c                    |  24 +
 src/util/virobject.h                    |   4 +
 src/util/virresctrl.c                   | 155 +++---
 src/util/virresctrl.h                   |  15 +-
 src/util/virsecret.c                    |   3 +-
 src/util/virstoragefile.c               |  42 +-
 src/util/virstoragefile.h               |   9 +-
 src/vbox/vbox_common.c                  |  19 +-
 src/vmware/vmware_conf.c                |  13 +-
 src/vz/vz_driver.c                      |  33 +-
 src/vz/vz_sdk.c                         |  22 +-
 tests/bhyveargv2xmltest.c               |   4 +-
 tests/bhyvexml2argvtest.c               |   4 +-
 tests/bhyvexml2xmltest.c                |   4 +-
 tests/cputest.c                         |  48 +-
 tests/domaincapstest.c                  |   5 +-
 tests/domainconftest.c                  |   4 +-
 tests/genericxml2xmltest.c              |   4 +-
 tests/networkxml2conftest.c             |  10 +-
 tests/networkxml2xmltest.c              |   3 +-
 tests/openvzutilstest.c                 |   4 +-
 tests/qemublocktest.c                   |   3 +-
 tests/qemucapabilitiestest.c            |   9 +-
 tests/qemucaps2xmltest.c                |  29 +-
 tests/qemucapsprobe.c                   |   4 +-
 tests/qemuhotplugtest.c                 |   6 +-
 tests/qemumemlocktest.c                 |   3 +-
 tests/testutils.c                       |   4 +-
 tests/testutilslxc.c                    |  22 +-
 tests/testutilsqemu.c                   |  31 +-
 tests/testutilsxen.c                    |   7 +-
 tests/vircaps2xmltest.c                 |   6 +-
 tests/vircapstest.c                     |  14 +-
 tests/virfilecachetest.c                |  69 ++-
 tests/virnetdaemontest.c                |   4 +-
 tests/virresctrltest.c                  |   6 +-
 tests/vmx2xmltest.c                     |  10 +-
 tests/xml2vmxtest.c                     |  12 +-
 152 files changed, 2411 insertions(+), 2660 deletions(-)

-- 
2.26.2

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

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 :|

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 :|

On Wed, Jun 3, 2020 at 11:38 AM Daniel P. Berrangé <berrange@redhat.com> 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