Introduce a new type virHashDataFreeSimple which has only a void * as
argument for cases when knowing the name of the entry when freeing the
hash entry is not required.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/conf/domain_addr.c |  4 ++--
 src/util/vircgroup.c   |  2 +-
 src/util/virhash.c     | 15 ++++++++++++++-
 src/util/virhash.h     | 10 ++++++++++
 4 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index d0026942aa..dd8e04576a 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -1037,14 +1037,14 @@ virDomainPCIAddressSetExtensionAlloc(virDomainPCIAddressSetPtr addrs,
         if (VIR_ALLOC(addrs->zpciIds) < 0)
             return -1;

-        if (!(addrs->zpciIds->uids = virHashCreateFull(10, NULL,
+        if (!(addrs->zpciIds->uids = virHashCreateFull(10, NULL, NULL,
                                                        virZPCIAddrKeyCode,
                                                        virZPCIAddrKeyEqual,
                                                        virZPCIAddrKeyCopy,
                                                        virZPCIAddrKeyFree)))
             goto error;

-        if (!(addrs->zpciIds->fids = virHashCreateFull(10, NULL,
+        if (!(addrs->zpciIds->fids = virHashCreateFull(10, NULL, NULL,
                                                        virZPCIAddrKeyCode,
                                                        virZPCIAddrKeyEqual,
                                                        virZPCIAddrKeyCopy,
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index d824aee86d..21c0fe67e3 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -2600,7 +2600,7 @@ virCgroupKillRecursive(virCgroupPtr group, int signum)
     bool backendAvailable = false;
     virCgroupBackendPtr *backends = virCgroupBackendGetAll();
     virHashTablePtr pids = virHashCreateFull(100,
-                                             NULL,
+                                             NULL, NULL,
                                              virCgroupPidCode,
                                              virCgroupPidEqual,
                                              virCgroupPidCopy,
diff --git a/src/util/virhash.c b/src/util/virhash.c
index a7fc620567..4d90fa5333 100644
--- a/src/util/virhash.c
+++ b/src/util/virhash.c
@@ -56,6 +56,7 @@ struct _virHashTable {
     size_t size;
     size_t nbElems;
     virHashDataFree dataFree;
+    virHashDataFreeSimple dataFreeSimple;
     virHashKeyCode keyCode;
     virHashKeyEqual keyEqual;
     virHashKeyCopy keyCopy;
@@ -133,6 +134,7 @@ virHashComputeKey(const virHashTable *table, const void *name)
  */
 virHashTablePtr virHashCreateFull(ssize_t size,
                                   virHashDataFree dataFree,
+                                  virHashDataFreeSimple dataFreeSimple,
                                   virHashKeyCode keyCode,
                                   virHashKeyEqual keyEqual,
                                   virHashKeyCopy keyCopy,
@@ -149,7 +151,10 @@ virHashTablePtr virHashCreateFull(ssize_t size,
     table->seed = virRandomBits(32);
     table->size = size;
     table->nbElems = 0;
-    table->dataFree = dataFree;
+    if (dataFree)
+        table->dataFree = dataFree;
+    else
+        table->dataFreeSimple = dataFreeSimple;
     table->keyCode = keyCode;
     table->keyEqual = keyEqual;
     table->keyCopy = keyCopy;
@@ -177,6 +182,7 @@ virHashTablePtr virHashCreate(ssize_t size, virHashDataFree dataFree)
 {
     return virHashCreateFull(size,
                              dataFree,
+                             NULL,
                              virHashStrCode,
                              virHashStrEqual,
                              virHashStrCopy,
@@ -298,6 +304,8 @@ virHashFree(virHashTablePtr table)

             if (table->dataFree)
                 table->dataFree(iter->payload, iter->name);
+            if (table->dataFreeSimple)
+                table->dataFreeSimple(iter->payload);
             if (table->keyFree)
                 table->keyFree(iter->name);
             VIR_FREE(iter);
@@ -330,6 +338,8 @@ virHashAddOrUpdateEntry(virHashTablePtr table, const void *name,
             if (is_update) {
                 if (table->dataFree)
                     table->dataFree(entry->payload, entry->name);
+                if (table->dataFreeSimple)
+                    table->dataFreeSimple(entry->payload);
                 entry->payload = userdata;
                 return 0;
             } else {
@@ -456,9 +466,12 @@ void *virHashSteal(virHashTablePtr table, const void *name)
     void *data = virHashLookup(table, name);
     if (data) {
         virHashDataFree dataFree = table->dataFree;
+        virHashDataFreeSimple dataFreeSimple = table->dataFreeSimple;
         table->dataFree = NULL;
+        table->dataFreeSimple = NULL;
         virHashRemoveEntry(table, name);
         table->dataFree = dataFree;
+        table->dataFreeSimple = dataFreeSimple;
     }
     return data;
 }
diff --git a/src/util/virhash.h b/src/util/virhash.h
index b5e7c79260..94fe8e23e4 100644
--- a/src/util/virhash.h
+++ b/src/util/virhash.h
@@ -30,6 +30,15 @@ typedef virHashAtomic *virHashAtomicPtr;
  * Callback to free data from a hash.
  */
 typedef void (*virHashDataFree) (void *payload, const void *name);
+/**
+ * virHashDataFreeSimple:
+ * @payload:  the data in the hash
+ * @name:  the name associated
+ *
+ * Callback to free data from a hash.
+ */
+typedef void (*virHashDataFreeSimple) (void *payload);
+
 /**
  * virHashIterator:
  * @payload: the data in the hash
@@ -104,6 +113,7 @@ virHashAtomicPtr virHashAtomicNew(ssize_t size,
                                   virHashDataFree dataFree);
 virHashTablePtr virHashCreateFull(ssize_t size,
                                   virHashDataFree dataFree,
+                                  virHashDataFreeSimple dataFreeSimple,
                                   virHashKeyCode keyCode,
                                   virHashKeyEqual keyEqual,
                                   virHashKeyCopy keyCopy,
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 10/18/19 11:10 AM, Peter Krempa wrote:
> Introduce a new type virHashDataFreeSimple which has only a void * as
> argument for cases when knowing the name of the entry when freeing the
> hash entry is not required.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>   src/conf/domain_addr.c |  4 ++--
>   src/util/vircgroup.c   |  2 +-
>   src/util/virhash.c     | 15 ++++++++++++++-
>   src/util/virhash.h     | 10 ++++++++++
>   4 files changed, 27 insertions(+), 4 deletions(-)
> 

This shows there were not many callers to virHashCreateFull.  (Thankfully)


> @@ -133,6 +134,7 @@ virHashComputeKey(const virHashTable *table, const void *name)
>    */
>   virHashTablePtr virHashCreateFull(ssize_t size,
>                                     virHashDataFree dataFree,
> +                                  virHashDataFreeSimple dataFreeSimple,

Is there any way to create a union argument which takes either a 
dataFree or dataFreeSimple function, rather than having to have two 
separate parameters?  But as there are not many callers, this does not 
hurt too much.

>                                     virHashKeyCode keyCode,
>                                     virHashKeyEqual keyEqual,
>                                     virHashKeyCopy keyCopy,
> @@ -149,7 +151,10 @@ virHashTablePtr virHashCreateFull(ssize_t size,
>       table->seed = virRandomBits(32);
>       table->size = size;
>       table->nbElems = 0;
> -    table->dataFree = dataFree;
> +    if (dataFree)
> +        table->dataFree = dataFree;
> +    else
> +        table->dataFreeSimple = dataFreeSimple;

I guess I'll need to see later in the series why we need this instead of 
being able to use virHashValueFree().  Are there really that many places 
where it is just too much boilerplate to add a simple one-liner 
forwarding function that passes the virHashDataFree signature with two 
parameters and calls the real freeing function with one parameter?

Should this function fail if the user passes non-NULL pointers for both 
dataFree and dataFreeSimple, rather than blindly favoring only dataFree?

But code-wise, the patch is correct.  So if its use later in the series 
proves useful, then consider this as an ACK.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Oct 18, 2019 at 13:22:56 -0500, Eric Blake wrote:
> On 10/18/19 11:10 AM, Peter Krempa wrote:
> > Introduce a new type virHashDataFreeSimple which has only a void * as
> > argument for cases when knowing the name of the entry when freeing the
> > hash entry is not required.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> >   src/conf/domain_addr.c |  4 ++--
> >   src/util/vircgroup.c   |  2 +-
> >   src/util/virhash.c     | 15 ++++++++++++++-
> >   src/util/virhash.h     | 10 ++++++++++
> >   4 files changed, 27 insertions(+), 4 deletions(-)
> > 
> 
> This shows there were not many callers to virHashCreateFull.  (Thankfully)
> 
> 
> > @@ -133,6 +134,7 @@ virHashComputeKey(const virHashTable *table, const void *name)
> >    */
> >   virHashTablePtr virHashCreateFull(ssize_t size,
> >                                     virHashDataFree dataFree,
> > +                                  virHashDataFreeSimple dataFreeSimple,
> 
> Is there any way to create a union argument which takes either a dataFree or
> dataFreeSimple function, rather than having to have two separate parameters?
> But as there are not many callers, this does not hurt too much.

I don't think there's a way that would result in simpler/cleaner code.

An ugly but working way would be to just call the unionified pointer
with two arguments as they share the first one.

Otherwise it would require to store an boolean to switch which variant
to use and that is basically equal to this implementation.

I don't want to add any additional usage error code paths since the
constructor reports only allocation errors and thus we will be able to
always assume that it returns a valid pointer.

> 
> >                                     virHashKeyCode keyCode,
> >                                     virHashKeyEqual keyEqual,
> >                                     virHashKeyCopy keyCopy,
> > @@ -149,7 +151,10 @@ virHashTablePtr virHashCreateFull(ssize_t size,
> >       table->seed = virRandomBits(32);
> >       table->size = size;
> >       table->nbElems = 0;
> > -    table->dataFree = dataFree;
> > +    if (dataFree)
> > +        table->dataFree = dataFree;
> > +    else
> > +        table->dataFreeSimple = dataFreeSimple;
> 
> I guess I'll need to see later in the series why we need this instead of
> being able to use virHashValueFree().  Are there really that many places
> where it is just too much boilerplate to add a simple one-liner forwarding
> function that passes the virHashDataFree signature with two parameters and
> calls the real freeing function with one parameter?

As said above I wanted to be correct code-wise. I could just typecast my
freeing function to virHashDataFree and it would work.

> 
> Should this function fail if the user passes non-NULL pointers for both
> dataFree and dataFreeSimple, rather than blindly favoring only dataFree?

I think it is ugly.

> 
> But code-wise, the patch is correct.  So if its use later in the series
> proves useful, then consider this as an ACK.
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list