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

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(a)redhat.com&gt; >> --- >> 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.