[PATCH 05/15] conf: domain_addr: Refactor hash usage in zpci reservation code
Bjoern Walk
bwalk at linux.ibm.com
Thu Oct 22 11:44:10 UTC 2020
Peter Krempa <pkrempa at redhat.com> [2020-10-22, 11:34AM +0200]:
> Rewrite using GHashTable which already has interfaces for using a number
> as hash key.
>
> Signed-off-by: Peter Krempa <pkrempa at redhat.com>
> ---
> src/conf/domain_addr.c | 123 +++++++++--------------------------------
> src/conf/domain_addr.h | 4 +-
> 2 files changed, 27 insertions(+), 100 deletions(-)
>
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index c7419916aa..4e47c2547c 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -25,17 +25,18 @@
> #include "virlog.h"
> #include "virstring.h"
> #include "domain_addr.h"
> -#include "virhashcode.h"
>
> #define VIR_FROM_THIS VIR_FROM_DOMAIN
>
> VIR_LOG_INIT("conf.domain_addr");
>
> static int
> -virDomainZPCIAddressReserveId(virHashTablePtr set,
> +virDomainZPCIAddressReserveId(GHashTable *set,
> virZPCIDeviceAddressID *id,
> const char *name)
> {
> + int *idval;
> +
> if (!id->isSet) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("No zPCI %s to reserve"),
> @@ -43,26 +44,24 @@ virDomainZPCIAddressReserveId(virHashTablePtr set,
> return -1;
> }
>
> - if (virHashLookup(set, &id->value)) {
> + if (g_hash_table_lookup(set, &id->value)) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("zPCI %s %o is already reserved"),
> name, id->value);
> return -1;
> }
>
> - if (virHashAddEntry(set, &id->value, (void*)1) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Failed to reserve %s %o"),
> - name, id->value);
> - return -1;
> - }
> + idval = g_new0(int, 1);
> + *idval = (int) id->value;
> +
> + g_hash_table_add(set, idval);
g_hash_table_add() can't fail, only abort(), right? Then dropping the
error message should be fine.
>
> return 0;
> }
>
>
> static int
> -virDomainZPCIAddressReserveUid(virHashTablePtr set,
> +virDomainZPCIAddressReserveUid(GHashTable *set,
> virZPCIDeviceAddressPtr addr)
> {
> return virDomainZPCIAddressReserveId(set, &addr->uid, "uid");
> @@ -70,7 +69,7 @@ virDomainZPCIAddressReserveUid(virHashTablePtr set,
>
>
> static int
> -virDomainZPCIAddressReserveFid(virHashTablePtr set,
> +virDomainZPCIAddressReserveFid(GHashTable *set,
> virZPCIDeviceAddressPtr addr)
> {
> return virDomainZPCIAddressReserveId(set, &addr->fid, "fid");
> @@ -78,7 +77,7 @@ virDomainZPCIAddressReserveFid(virHashTablePtr set,
>
>
> static int
> -virDomainZPCIAddressAssignId(virHashTablePtr set,
> +virDomainZPCIAddressAssignId(GHashTable *set,
> virZPCIDeviceAddressID *id,
> unsigned int min,
> unsigned int max,
> @@ -87,7 +86,7 @@ virDomainZPCIAddressAssignId(virHashTablePtr set,
> if (id->isSet)
> return 0;
>
> - while (virHashLookup(set, &min)) {
> + while (g_hash_table_lookup(set, &min)) {
> if (min == max) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("There is no more free %s."),
> @@ -105,7 +104,7 @@ virDomainZPCIAddressAssignId(virHashTablePtr set,
>
>
> static int
> -virDomainZPCIAddressAssignUid(virHashTablePtr set,
> +virDomainZPCIAddressAssignUid(GHashTable *set,
> virZPCIDeviceAddressPtr addr)
> {
> return virDomainZPCIAddressAssignId(set, &addr->uid, 1,
> @@ -114,7 +113,7 @@ virDomainZPCIAddressAssignUid(virHashTablePtr set,
>
>
> static int
> -virDomainZPCIAddressAssignFid(virHashTablePtr set,
> +virDomainZPCIAddressAssignFid(GHashTable *set,
> virZPCIDeviceAddressPtr addr)
> {
> return virDomainZPCIAddressAssignId(set, &addr->fid, 0,
> @@ -123,40 +122,19 @@ virDomainZPCIAddressAssignFid(virHashTablePtr set,
>
>
> static void
> -virDomainZPCIAddressReleaseId(virHashTablePtr set,
> - virZPCIDeviceAddressID *id,
> - const char *name)
> +virDomainZPCIAddressReleaseId(GHashTable *set,
> + virZPCIDeviceAddressID *id)
> {
> if (!id->isSet)
> return;
>
> - if (virHashRemoveEntry(set, &id->value) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Release %s %o failed"),
> - name, id->value);
> - }
> + g_hash_table_remove(set, &id->value);
Here I'm not so sure, IIUC the original code reported an error when the
value was not found in the hash table. This will be dropped with the new
code. But since this should only occur in pathological cases anyways, so
I guess this is fine.
>
> id->value = 0;
> id->isSet = false;
> }
>
>
> -static void
> -virDomainZPCIAddressReleaseUid(virHashTablePtr set,
> - virZPCIDeviceAddressPtr addr)
> -{
> - virDomainZPCIAddressReleaseId(set, &addr->uid, "uid");
> -}
> -
> -
> -static void
> -virDomainZPCIAddressReleaseFid(virHashTablePtr set,
> - virZPCIDeviceAddressPtr addr)
> -{
> - virDomainZPCIAddressReleaseId(set, &addr->fid, "fid");
> -}
> -
> -
> static void
> virDomainZPCIAddressReleaseIds(virDomainZPCIAddressIdsPtr zpciIds,
> virZPCIDeviceAddressPtr addr)
> @@ -164,8 +142,8 @@ virDomainZPCIAddressReleaseIds(virDomainZPCIAddressIdsPtr zpciIds,
> if (!zpciIds)
> return;
>
> - virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> - virDomainZPCIAddressReleaseFid(zpciIds->fids, addr);
> + virDomainZPCIAddressReleaseId(zpciIds->uids, &addr->uid);
> + virDomainZPCIAddressReleaseId(zpciIds->fids, &addr->fid);
> }
>
>
> @@ -183,7 +161,7 @@ virDomainZPCIAddressEnsureAddr(virDomainZPCIAddressIdsPtr zpciIds,
> return -1;
>
> if (virDomainZPCIAddressReserveFid(zpciIds->fids, addr) < 0) {
> - virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> + virDomainZPCIAddressReleaseId(zpciIds->uids, &addr->uid);
> return -1;
> }
>
> @@ -965,55 +943,15 @@ virDomainPCIAddressReleaseAddr(virDomainPCIAddressSetPtr addrs,
> }
>
>
> -static uint32_t
> -virZPCIAddrKeyCode(const void *name,
> - uint32_t seed)
> -{
> - unsigned int value = *((unsigned int *)name);
> - return virHashCodeGen(&value, sizeof(value), seed);
> -}
> -
> -
> -static bool
> -virZPCIAddrKeyEqual(const void *namea,
> - const void *nameb)
> -{
> - return *((unsigned int *)namea) == *((unsigned int *)nameb);
> -}
> -
> -
> -static void *
> -virZPCIAddrKeyCopy(const void *name)
> -{
> - unsigned int *copy = g_new0(unsigned int, 1);
> -
> - *copy = *((unsigned int *)name);
> - return (void *)copy;
> -}
> -
> -
> -static char *
> -virZPCIAddrKeyPrintHuman(const void *name)
> -{
> - return g_strdup_printf("%u", *((unsigned int *)name));
> -}
> -
> -
> -static void
> -virZPCIAddrKeyFree(void *name)
> -{
> - VIR_FREE(name);
> -}
> -
> -
> static void
> virDomainPCIAddressSetExtensionFree(virDomainPCIAddressSetPtr addrs)
> {
> if (!addrs || !addrs->zpciIds)
> return;
>
> - virHashFree(addrs->zpciIds->uids);
> - virHashFree(addrs->zpciIds->fids);
> + g_clear_pointer(&addrs->zpciIds->uids, g_hash_table_unref);
> + g_clear_pointer(&addrs->zpciIds->fids, g_hash_table_unref);
> +
> VIR_FREE(addrs->zpciIds);
> }
>
> @@ -1028,19 +966,8 @@ virDomainPCIAddressSetExtensionAlloc(virDomainPCIAddressSetPtr addrs,
>
> addrs->zpciIds = g_new0(virDomainZPCIAddressIds, 1);
>
> - addrs->zpciIds->uids = virHashCreateFull(10, NULL,
> - virZPCIAddrKeyCode,
> - virZPCIAddrKeyEqual,
> - virZPCIAddrKeyCopy,
> - virZPCIAddrKeyPrintHuman,
> - virZPCIAddrKeyFree);
> -
> - addrs->zpciIds->fids = virHashCreateFull(10, NULL,
> - virZPCIAddrKeyCode,
> - virZPCIAddrKeyEqual,
> - virZPCIAddrKeyCopy,
> - virZPCIAddrKeyPrintHuman,
> - virZPCIAddrKeyFree);
> + addrs->zpciIds->uids = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, NULL);
> + addrs->zpciIds->fids = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, NULL);
> }
>
> return 0;
> diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
> index a0460b4030..77dd091fd3 100644
> --- a/src/conf/domain_addr.h
> +++ b/src/conf/domain_addr.h
> @@ -115,8 +115,8 @@ typedef struct {
> typedef virDomainPCIAddressBus *virDomainPCIAddressBusPtr;
>
> typedef struct {
> - virHashTablePtr uids;
> - virHashTablePtr fids;
> + GHashTable *uids;
> + GHashTable *fids;
> } virDomainZPCIAddressIds;
> typedef virDomainZPCIAddressIds *virDomainZPCIAddressIdsPtr;
>
> --
> 2.26.2
>
Rest looks good.
Reviewed-by: Bjoern Walk <bwalk at linux.ibm.com>
--
IBM Systems
Linux on Z & Virtualization Development
--------------------------------------------------
IBM Deutschland Research & Development GmbH
Schönaicher Str. 220, 71032 Böblingen
Phone: +49 7031 16 1819
--------------------------------------------------
Vorsitzende des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 902 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20201022/d5b06041/attachment-0001.sig>
More information about the libvir-list
mailing list