[libvirt] [PATCH 06/11] util: use glib base64 encoding/decoding APIs

[Laine Stump]

On 9/30/19 10:05 AM, Andrea Bolognani wrote:
> On Mon, 2019-09-30 at 13:41 +0100, Daniel P. Berrangé wrote:
>> On Mon, Sep 30, 2019 at 02:18:17PM +0200, Andrea Bolognani wrote:
>>> On Mon, 2019-09-30 at 13:56 +0200, Pavel Hrdina wrote:
>>>> Agreed, for now let's keep all the wrappers but eventually we can remove
>>>> them to make the code cleaner.
>>> Note that we should be able to use VIR_AUTOPTR() instead of
>>> g_autoptr() even for objects, by simply registering the same
>>> GLib-provided free function with our own macros.
>>>
>>> That way we could keep using VIR_AUTO* everywhere and then, when
>>> we're ready, mechanically switch everything to g_auto* in one fell
>>> swoop, without having any point in time where the two styles are
>>> coexisting.
>> That creates an even bigger conversion later. Such big conversions
>> cause more pain for backports, than doing an incremental conversion
>> at appropriate times.
>>
>> Converting everything to g_autofree right now doesn't give style
>> consistency as we'd still be matching VIR_ALLOC + g_autofree,
>> so I don't see a benefit to a big conversion in 1 step.
>>
>> Incrementally converting VIR_ALLOC + VIR_AUTOFREE at the same
>> time, makes more sense stylewise, as then within the scope of a
>> single method we'd be consistent.
> I see your point about backports being more painful when you have
> a bunch of unrelated changes mixed in, but I would still prefer if
> we converted everything at once and at the same time introduced a
> suitable syntax-check rule preventing more instances of whatever
> function we just removed all callers of from creeping back in, or
> actually just dropping the function altogether.


Don't forget that make syntax-check doesn't work properly for many 
downstream maintenance branches that would be backported to (it has to 
be disabled due to copyright date checks failing, or something like 
that). Of course that wouldn't be a problem until some future 
maintenance branch based on a release that uses glib but still has 
VIR_AUTO* defined.


On the other hand, many *current* maintenance branches that must be 
targeted by backports don't even have the VIR_AUTO* macros (which were 
introduced in 4.6.0, but *still* aren't used in a lot of the code), so 
there are going to be backport headaches anyway, and they are going to 
be of equal pain regardless of whether we do a big bang conversion or 
gradual conversion. The differences in pain level will be noticed in 
downstream maintenance branches >= 4.6.0 and <= 5.9.0 (or whatever version).


> Doing the conversion incrementally will IMHO result in dragging it
> for much longer, causing more pain in the long run than ripping the
> bandaid would.


In order to allay Andrea's fears of new usage of VIR_AUTO* that just 
draws out the conversion, maybe we could (temporarily, until the 
conversion is complete) put a commit hook in place to disallow new use 
of VIR_AUTO ? Or just, you know, pay attention in reviews (but of course 
part of the point of all of this is to eliminate the potential for human 
error, by depending less on humans paying attention, so... :-P)


(BTW, I'm not firmly in *either* camp, although I may lean a bit more 
towards a gradual change (but with a *very* steep slope to minimize the 
period of confusion)