Re: [PATCH libvirt v2 2/5] conf: fix zPCI address auto-generation on s390

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux