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.