From: Jiri Denemark <jdenemar@redhat.com>

Refactor weight calculation to a separate virCPUx86WeightFeatures
function to avoid code duplication.

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---
 src/cpu/cpu_x86.c | 83 ++++++++++++++++++++++++++---------------------
 1 file changed, 46 insertions(+), 37 deletions(-)

diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index 32aa01bc14..b0fe2bed4c 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -2090,63 +2090,72 @@ virCPUx86Compare(virCPUDef *host,
 /* Base penalty for disabled features. */
 #define BASE_PENALTY 2
 
+struct virCPUx86Weight {
+    size_t total;
+    size_t enabled;
+    size_t disabled;
+};
+
+static void
+virCPUx86WeightFeatures(const virCPUDef *cpu,
+                        struct virCPUx86Weight *weight)
+{
+    int penalty = BASE_PENALTY;
+    size_t i;
+
+    weight->enabled = cpu->nfeatures;
+    weight->disabled = 0;
+
+    if (cpu->type == VIR_CPU_TYPE_HOST) {
+        weight->total = cpu->nfeatures;
+        return;
+    }
+
+    for (i = 0; i < weight->enabled; i++) {
+        if (cpu->features[i].policy == VIR_CPU_FEATURE_DISABLE) {
+            weight->enabled--;
+            weight->disabled += penalty;
+            penalty++;
+        }
+    }
+
+    weight->total = weight->enabled + weight->disabled;
+}
+
+
 static int
 virCPUx86CompareCandidateFeatureList(virCPUDef *cpuCurrent,
                                      virCPUDef *cpuCandidate,
                                      bool isPreferred)
 {
-    size_t current = cpuCurrent->nfeatures;
-    size_t enabledCurrent = current;
-    size_t disabledCurrent = 0;
-    size_t candidate = cpuCandidate->nfeatures;
-    size_t enabled = candidate;
-    size_t disabled = 0;
+    struct virCPUx86Weight current = { 0 };
+    struct virCPUx86Weight candidate = { 0 };
 
-    if (cpuCandidate->type != VIR_CPU_TYPE_HOST) {
-        size_t i;
-        int penalty = BASE_PENALTY;
+    virCPUx86WeightFeatures(cpuCurrent, &current);
+    virCPUx86WeightFeatures(cpuCandidate, &candidate);
 
-        for (i = 0; i < enabledCurrent; i++) {
-            if (cpuCurrent->features[i].policy == VIR_CPU_FEATURE_DISABLE) {
-                enabledCurrent--;
-                disabledCurrent += penalty;
-                penalty++;
-            }
-        }
-        current = enabledCurrent + disabledCurrent;
-
-        penalty = BASE_PENALTY;
-        for (i = 0; i < enabled; i++) {
-            if (cpuCandidate->features[i].policy == VIR_CPU_FEATURE_DISABLE) {
-                enabled--;
-                disabled += penalty;
-                penalty++;
-            }
-        }
-        candidate = enabled + disabled;
-    }
-
-    if (candidate < current ||
-        (candidate == current && disabled < disabledCurrent)) {
+    if (candidate.total < current.total ||
+        (candidate.total == current.total &&
+         candidate.disabled < current.disabled)) {
         VIR_DEBUG("%s is better than %s: %zu (%zu, %zu) < %zu (%zu, %zu)",
                   cpuCandidate->model, cpuCurrent->model,
-                  candidate, enabled, disabled,
-                  current, enabledCurrent, disabledCurrent);
+                  candidate.total, candidate.enabled, candidate.disabled,
+                  current.total, current.enabled, current.disabled);
         return 1;
     }
 
-    if (isPreferred && disabled < disabledCurrent) {
+    if (isPreferred && candidate.disabled < current.disabled) {
         VIR_DEBUG("%s is in the list of preferred models and provides fewer "
                   "disabled features than %s: %zu < %zu",
                   cpuCandidate->model, cpuCurrent->model,
-                  disabled, disabledCurrent);
+                  candidate.disabled, current.disabled);
         return 1;
     }
 
     VIR_DEBUG("%s is not better than %s: %zu (%zu, %zu) >= %zu (%zu, %zu)",
               cpuCandidate->model, cpuCurrent->model,
-              candidate, enabled, disabled,
-              current, enabledCurrent, disabledCurrent);
+              candidate.total, candidate.enabled, candidate.disabled,
+              current.total, current.enabled, current.disabled);
     return 0;
 }
 
-- 
2.49.0

On Tue, Apr 29, 2025 at 12:19:49 +0200, Jiri Denemark via Devel wrote:
> From: Jiri Denemark <jdenemar@redhat.com>
> 
> Refactor weight calculation to a separate virCPUx86WeightFeatures
> function to avoid code duplication.
> 
> Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> ---
>  src/cpu/cpu_x86.c | 83 ++++++++++++++++++++++++++---------------------
>  1 file changed, 46 insertions(+), 37 deletions(-)
> 
> diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
> index 32aa01bc14..b0fe2bed4c 100644
> --- a/src/cpu/cpu_x86.c
> +++ b/src/cpu/cpu_x86.c
> @@ -2090,63 +2090,72 @@ virCPUx86Compare(virCPUDef *host,
>  /* Base penalty for disabled features. */
>  #define BASE_PENALTY 2
>  
> +struct virCPUx86Weight {
> +    size_t total;
> +    size_t enabled;
> +    size_t disabled;
> +};
> +
> +static void
> +virCPUx86WeightFeatures(const virCPUDef *cpu,
> +                        struct virCPUx86Weight *weight)
> +{
> +    int penalty = BASE_PENALTY;
> +    size_t i;
> +
> +    weight->enabled = cpu->nfeatures;
> +    weight->disabled = 0;
> +
> +    if (cpu->type == VIR_CPU_TYPE_HOST) {
> +        weight->total = cpu->nfeatures;
> +        return;
> +    }
> +
> +    for (i = 0; i < weight->enabled; i++) {
> +        if (cpu->features[i].policy == VIR_CPU_FEATURE_DISABLE) {
> +            weight->enabled--;

I know that this is just moving code but the fact that you modify the
variable used to limit the iteration ought to be explained in a comment
because it's really non-obvious to a reader what's happening here.

> +            weight->disabled += penalty;
> +            penalty++;
> +        }
> +    }
> +
> +    weight->total = weight->enabled + weight->disabled;
> +}
> +
> +

The new version is much more readable!

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Tue, Apr 29, 2025 at 15:27:44 +0200, Peter Krempa via Devel wrote:
> On Tue, Apr 29, 2025 at 12:19:49 +0200, Jiri Denemark via Devel wrote:
> > From: Jiri Denemark <jdenemar@redhat.com>
> > 
> > Refactor weight calculation to a separate virCPUx86WeightFeatures
> > function to avoid code duplication.
> > 
> > Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> > ---
> >  src/cpu/cpu_x86.c | 83 ++++++++++++++++++++++++++---------------------
> >  1 file changed, 46 insertions(+), 37 deletions(-)
> > 
> > diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
> > index 32aa01bc14..b0fe2bed4c 100644
> > --- a/src/cpu/cpu_x86.c
> > +++ b/src/cpu/cpu_x86.c
> > @@ -2090,63 +2090,72 @@ virCPUx86Compare(virCPUDef *host,
> >  /* Base penalty for disabled features. */
> >  #define BASE_PENALTY 2
> >  
> > +struct virCPUx86Weight {
> > +    size_t total;
> > +    size_t enabled;
> > +    size_t disabled;
> > +};
> > +
> > +static void
> > +virCPUx86WeightFeatures(const virCPUDef *cpu,
> > +                        struct virCPUx86Weight *weight)
> > +{
> > +    int penalty = BASE_PENALTY;
> > +    size_t i;
> > +
> > +    weight->enabled = cpu->nfeatures;
> > +    weight->disabled = 0;
> > +
> > +    if (cpu->type == VIR_CPU_TYPE_HOST) {
> > +        weight->total = cpu->nfeatures;
> > +        return;
> > +    }
> > +
> > +    for (i = 0; i < weight->enabled; i++) {
> > +        if (cpu->features[i].policy == VIR_CPU_FEATURE_DISABLE) {
> > +            weight->enabled--;
> 
> I know that this is just moving code but the fact that you modify the
> variable used to limit the iteration ought to be explained in a comment
> because it's really non-obvious to a reader what's happening here.

Okay I see now that I've read 14/15 why this looks so confusing.

Please note in the commit message that the patch is faithfully
extracting the algorithm without modifying it even when it is wrong :D

> 
> > +            weight->disabled += penalty;
> > +            penalty++;
> > +        }
> > +    }
> > +
> > +    weight->total = weight->enabled + weight->disabled;
> > +}
> > +
> > +
> 
> The new version is much more readable!
> 
> Reviewed-by: Peter Krempa <pkrempa@redhat.com>
>