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@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);
}
--
2.29.2
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@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. Michal
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@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.
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@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
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@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? 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).
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@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.
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@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!)
© 2016 - 2026 Red Hat, Inc.