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