[libvirt] [RFC 0/1] convert virStorageSource to GObject

[Daniel Henrique Barboza]

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