[libvirt] [PATCH] conf: rewrite filtering for capabilities lookup

[Daniel P. Berrangé]

ping

On Fri, Aug 03, 2018 at 03:28:16PM +0100, Daniel P. Berrangé wrote:
> The virCapabilitiesDomainDataLookupInternal() is given a list of
> parameters representing the desired domain characteristics. It then has
> to look throught the capabilities to identify an acceptable match.
> 
> The virCapsDomainDataCompare() method is used for filtering out
> candidates which don't match the desired criteria. It is called
> primarily from the innermost loops and as such is doing many repeated
> checks. For example if architcture and os type were checked at the top
> level loop the two inner loops could be avoided entirely. If emulator
> and domain type were checked in the 2nd level loop the 3rd level loop
> can be avoided too.
> 
> This change thus removes the virCapsDomainDataCompare() method and puts
> suitable checks at the start of each loop to ensure it executes the
> minimal number of loop iterations possible. The code becomes clearer to
> understand as a nice side-effect.
> 
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
>  src/conf/capabilities.c | 100 ++++++++++++++++++----------------------
>  1 file changed, 45 insertions(+), 55 deletions(-)
> 
> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> index 0f96500294..cfd5132329 100644
> --- a/src/conf/capabilities.c
> +++ b/src/conf/capabilities.c
> @@ -604,46 +604,6 @@ virCapabilitiesHostSecModelAddBaseLabel(virCapsHostSecModelPtr secmodel,
>      return -1;
>  }
>  
> -static bool
> -virCapsDomainDataCompare(virCapsGuestPtr guest,
> -                         virCapsGuestDomainPtr domain,
> -                         virCapsGuestMachinePtr machine,
> -                         int ostype,
> -                         virArch arch,
> -                         virDomainVirtType domaintype,
> -                         const char *emulator,
> -                         const char *machinetype)
> -{
> -    const char *check_emulator = NULL;
> -
> -    if (ostype != -1 && guest->ostype != ostype)
> -        return false;
> -    if ((arch != VIR_ARCH_NONE) && (guest->arch.id != arch))
> -        return false;
> -
> -    if (domaintype != VIR_DOMAIN_VIRT_NONE &&
> -        (!domain || domain->type != domaintype))
> -        return false;
> -
> -    if (emulator) {
> -        if (domain)
> -            check_emulator = domain->info.emulator;
> -        if (!check_emulator)
> -            check_emulator = guest->arch.defaultInfo.emulator;
> -        if (STRNEQ_NULLABLE(check_emulator, emulator))
> -            return false;
> -    }
> -
> -    if (machinetype) {
> -        if (!machine)
> -            return false;
> -        if (STRNEQ(machine->name, machinetype) &&
> -            (STRNEQ_NULLABLE(machine->canonical, machinetype)))
> -            return false;
> -    }
> -
> -    return true;
> -}
>  
>  static virCapsDomainDataPtr
>  virCapabilitiesDomainDataLookupInternal(virCapsPtr caps,
> @@ -659,13 +619,45 @@ virCapabilitiesDomainDataLookupInternal(virCapsPtr caps,
>      virCapsDomainDataPtr ret = NULL;
>      size_t i, j, k;
>  
> +    VIR_DEBUG("Lookup ostype=%d arch=%d domaintype=%d emulator=%s machine=%s",
> +              ostype, arch, domaintype, NULLSTR(emulator), NULLSTR(machinetype));
>      for (i = 0; i < caps->nguests; i++) {
>          virCapsGuestPtr guest = caps->guests[i];
>  
> +        if (ostype != -1 && guest->ostype != ostype) {
> +            VIR_DEBUG("Skip os type want=%d vs got=%d", ostype, guest->ostype);
> +            continue;
> +        }
> +        VIR_DEBUG("Match os type %d", ostype);
> +
> +        if ((arch != VIR_ARCH_NONE) && (guest->arch.id != arch)) {
> +            VIR_DEBUG("Skip arch want=%d vs got=%d", arch, guest->arch.id);
> +            continue;
> +        }
> +        VIR_DEBUG("Match arch %d", arch);
> +
>          for (j = 0; j < guest->arch.ndomains; j++) {
>              virCapsGuestDomainPtr domain = guest->arch.domains[j];
>              virCapsGuestMachinePtr *machinelist;
>              int nmachines;
> +            const char *check_emulator = NULL;
> +
> +            if (domaintype != VIR_DOMAIN_VIRT_NONE &&
> +                (domain->type != domaintype)) {
> +                VIR_DEBUG("Skip domain type want=%d vs got=%d", domaintype, domain->type);
> +                continue;
> +            }
> +            VIR_DEBUG("Match domain type %d", domaintype);
> +
> +            check_emulator = domain->info.emulator;
> +            if (!check_emulator)
> +                check_emulator = guest->arch.defaultInfo.emulator;
> +            if (emulator && STRNEQ_NULLABLE(check_emulator, emulator)) {
> +                VIR_DEBUG("Skip emulator got=%s vs want=%s",
> +                          emulator, NULLSTR(check_emulator));
> +                continue;
> +            }
> +            VIR_DEBUG("Match emulator %s", NULLSTR(emulator));
>  
>              if (domain->info.nmachines) {
>                  nmachines = domain->info.nmachines;
> @@ -677,32 +669,29 @@ virCapabilitiesDomainDataLookupInternal(virCapsPtr caps,
>  
>              for (k = 0; k < nmachines; k++) {
>                  virCapsGuestMachinePtr machine = machinelist[k];
> -                if (!virCapsDomainDataCompare(guest, domain, machine,
> -                                              ostype, arch, domaintype,
> -                                              emulator, machinetype))
> +
> +                if (machinetype &&
> +                    STRNEQ(machine->name, machinetype) &&
> +                    STRNEQ_NULLABLE(machine->canonical, machinetype)) {
> +                    VIR_DEBUG("Skip machine type want=%s vs got=%s got=%s",
> +                              machinetype, machine->name, NULLSTR(machine->canonical));
>                      continue;
> +                }
> +                VIR_DEBUG("Match machine type machine %s\n", NULLSTR(machinetype));
>  
>                  foundmachine = machine;
>                  break;
>              }
>  
> -            if (!foundmachine) {
> -                if (!virCapsDomainDataCompare(guest, domain, NULL,
> -                                              ostype, arch, domaintype,
> -                                              emulator, machinetype))
> -                    continue;
> -            }
> +            if (!foundmachine && nmachines)
> +                continue;
>  
>              founddomain = domain;
>              break;
>          }
>  
> -        if (!founddomain) {
> -            if (!virCapsDomainDataCompare(guest, NULL, NULL,
> -                                          ostype, arch, domaintype,
> -                                          emulator, machinetype))
> -                continue;
> -        }
> +        if (!founddomain)
> +            continue;
>  
>          foundguest = guest;
>          break;
> @@ -731,6 +720,7 @@ virCapabilitiesDomainDataLookupInternal(virCapsPtr caps,
>              goto error;
>          }
>  
> +        VIR_DEBUG("No match %s", virBufferCurrentContent(&buf));
>          virReportError(VIR_ERR_INVALID_ARG,
>                         _("could not find capabilities for %s"),
>                         virBufferCurrentContent(&buf));
> -- 
> 2.17.1
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|