[PATCH 3/8] conf: replace g_clear_pointer(..., g_hash_table_unref) with virHashFree()

[Laine Stump]

On 2/1/21 4:51 AM, Peter Krempa wrote:
> On Mon, Feb 01, 2021 at 10:47:47 +0100, Peter Krempa wrote:
>> On Mon, Feb 01, 2021 at 10:18:52 +0100, Michal Privoznik wrote:
>>> On 2/1/21 7:27 AM, Laine Stump wrote:
>>>> virHashFree() just calls g_hash_table_unref(), and it's more common
>>>> for libvirt code to call virHashFree() rather than the convoluted
>>>> calling of g_hash_table_unref() via g_clear_pointer().
>>>>
>>>> Since the object containing the hashes is g_freed immediately after
>>>> the hashes are freed, there is no functional difference.
>>>>
>>>> Signed-off-by: Laine Stump <laine at redhat.com>
>>>> ---
>>>>    src/conf/domain_addr.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
>>>> index 37dad20ade..a8648d5858 100644
>>>> --- a/src/conf/domain_addr.c
>>>> +++ b/src/conf/domain_addr.c
>>>> @@ -949,8 +949,8 @@ virDomainPCIAddressSetExtensionFree(virDomainPCIAddressSetPtr addrs)
>>>>        if (!addrs || !addrs->zpciIds)
>>>>            return;
>>>> -    g_clear_pointer(&addrs->zpciIds->uids, g_hash_table_unref);
>>>> -    g_clear_pointer(&addrs->zpciIds->fids, g_hash_table_unref);
>>>> +    virHashFree(addrs->zpciIds->uids);
>>>> +    virHashFree(addrs->zpciIds->fids);
>>>>        VIR_FREE(addrs->zpciIds);
>>>>    }
>>>>
>>> virHashFree documents itself as being deprecated in favor of
>>> g_hash_table_unref().
>>>
>>> While I like our virSomething wrappers (mostly because I'm used to them more
>>> than to their glib counterparts; but then you also have glib functions when
>>> one thinks that glib implementation is interchangeable with ours but it
>>> isn't - devil's in the details), I think our intent is to drop
>>> virHashFree().
>>>
>>> But then again - we didn't, instead we replaced virHash* internals with
>>> glib, so I can argue that being consistent is more important than being
>>> progressive.
>>>
>>> Your call, but since you build next patch on this one, I'm inclined to say
>>> it's okay to merge it.
>> It's a NACK from me. That was deliberate. Especially virHashFree doesn't
>> clear the pointer, the code which we have does.
> Oh, and one more reason. Those hash tables are allocated directly by the
> glib function since they don't use strings as keys which is the only
> wrapper that virHash provides, thus they should not be freed by the
> wrappers.


Yeah, I was looking too narrow and didn't notice that. (once again 
proving the utility of peer reviews!)


I do still think that we shouldn't be unnecessarily using 
g_clear_pointer(), but I'll just drop the patch for now, and maybe 
revisit the next time I pass this way.


(NB: If we really think that virHash should be deprecated, then we 
should start converting. And if we instead think that its new 
g_hash_table-based implementation is a useful value-add on top of the 
straight glib API, then we should remove the "deprecated" notes from 
virhash.c, and change this usage to (fully, not half-assed like I did) 
use virHash like everyone else. No, I'm not volunteering!)