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

Michal Privoznik mprivozn at redhat.com
Mon Feb 1 09:50:47 UTC 2021


On 2/1/21 10:47 AM, 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.
> 

But as can be seen from the context, the whole object is freed 
immediately afterwards. IOW, this is what's happening:

free(obj->ptr);
obj->ptr = NULL;
free(obj);

Is the pointer clearing necessary?

Michal




More information about the libvir-list mailing list