Re: [libvirt] [PATCH RFC 01/40] util: hash: Add possibility to use simpler data free function in virHash
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list