On Mon, Feb 01, 2021 at 10:50:47 +0100, Michal Privoznik wrote: > 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@xxxxxxxxxx> > > > > --- > > > > 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? Not really, but g_hash_table_unref doesn't tolerate NULL argument, while using g_clear_pointer makes it tolerate it, thus the code is a bit neater IMO than an explicit if (ptr).
