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

Daniel P. Berrangé posted 1 patch 5 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180803142816.12796-1-berrange@redhat.com
Test syntax-check passed
src/conf/capabilities.c | 100 ++++++++++++++++++----------------------
1 file changed, 45 insertions(+), 55 deletions(-)
[libvirt] [PATCH] conf: rewrite filtering for capabilities lookup
Posted by Daniel P. Berrangé 5 years, 7 months ago
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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: rewrite filtering for capabilities lookup
Posted by Michal Privoznik 5 years, 7 months ago
On 08/03/2018 04:28 PM, 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.

Unfinished sen... ;-)

> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/conf/capabilities.c | 100 ++++++++++++++++++----------------------
>  1 file changed, 45 insertions(+), 55 deletions(-)
> 


> @@ -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));
> 

This debug is pretty useless because the error message is the same and
will be in the logs too.

ACK

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: rewrite filtering for capabilities lookup
Posted by Daniel P. Berrangé 5 years, 7 months ago
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@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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list