Andrea Bolognani <abologna@xxxxxxxxxx> [2020-06-25, 07:43PM +0200]:
> From 82197ea6d794144e51b72f97df2315a65134b385 Mon Sep 17 00:00:00 2001
> From: Andrea Bolognani <abologna@xxxxxxxxxx>
> Date: Thu, 25 Jun 2020 18:37:27 +0200
> Subject: [libvirt PATCH 1/2] conf: Move id->{value,isSet} handling further
> down the stack
>
> Signed-off-by: Andrea Bolognani <abologna@xxxxxxxxxx>
> ---
> src/conf/domain_addr.c | 61 ++++++++++++++++++++++++------------------
> 1 file changed, 35 insertions(+), 26 deletions(-)
>
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index 90ed31ef4e..493b155129 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -33,20 +33,27 @@ VIR_LOG_INIT("conf.domain_addr");
>
> static int
> virDomainZPCIAddressReserveId(virHashTablePtr set,
> - unsigned int id,
> + virZPCIDeviceAddressID *id,
> const char *name)
> {
> - if (virHashLookup(set, &id)) {
> + if (!id->isSet) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("No zPCI %s to reserve"),
> + name);
Maybe it's better to fail silently here (or not fail at all and just
make it no-op)? This is more of an assert that concerns the developer
and not something the user can understand.
> + return -1;
> + }
> +
> + if (virHashLookup(set, &id->value)) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("zPCI %s %o is already reserved"),
> - name, id);
> + name, id->value);
> return -1;
> }
>
> - if (virHashAddEntry(set, &id, (void*)1) < 0) {
> + if (virHashAddEntry(set, &id->value, (void*)1) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to reserve %s %o"),
> - name, id);
> + name, id->value);
> return -1;
> }
>
> [...]
>
> static void
> virDomainZPCIAddressReleaseId(virHashTablePtr set,
> - unsigned int *id,
> + virZPCIDeviceAddressID *id,
> const char *name)
> {
> - if (virHashRemoveEntry(set, id) < 0) {
> + if (!id->isSet)
> + return;
> +
> + if (virHashRemoveEntry(set, &id->value) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Release %s %o failed"),
> - name, *id);
> + name, id->value);
> }
>
> - *id = 0;
> + id->value = 0;
> + id->isSet = false;
Not sure if I don't want to leave this to the caller. 99% of time it
shouldn't matter because the released device is deleted from the domain
anyways, but this tight coupling would prevent hypothetical re-use of
the id.
Reviewed-by: Bjoern Walk <bwalk@xxxxxxxxxxxxx>
--
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
Attachment:
signature.asc
Description: PGP signature
