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

[Daniel Henrique Barboza]

On 10/15/19 2:15 PM, Daniel P. Berrangé wrote:
> On Tue, Oct 15, 2019 at 10:51:39AM -0300, Daniel Henrique Barboza wrote:
>>
>> 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.
> Yeah, you need to drop the 'Ptr' suffix in the type name when
> converting to g_autoptr, as it adds the pointer itself.
>
>
>>>> 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?
> You should change all variables in the method at the same time.
> Both the VIR_AUTOUNEF calls here can use g_autoptr, as can the
> two VIR_AUTOPTR calls.

Thanks for clarifying. I was going to re-send the patches adding GLib
macros instead of VIR_AUTO* ones, which would end up breaking this
rule because these patches are changing stuff in smaller steps.

What I'll end up is to basically re-send them as they are now, but with an
extra patch to change everything to GLib at once. That way we'll stay
compliant every step of the way.




DHB


>
> Regards,
> Daniel