Re: [PATCH] libs/light: use the cpuid feature names from cpufeatureset.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.06.2023 12:36, Roger Pau Monne wrote:
> @@ -51,7 +53,7 @@ void libxl_cpuid_dispose(libxl_cpuid_policy_list 
> *p_cpuid_list)
>   * Used for the static structure describing all features.
>   */
>  struct cpuid_flags {
> -    char* name;
> +    const char* name;

Nit: Would you mind also moving * to its designated position at this
occasion?

> @@ -81,6 +83,14 @@ static libxl_cpuid_policy_list 
> cpuid_find_match(libxl_cpuid_policy_list *list,
>      return *list + i;
>  }
>  
> +static int search_feature(const void *a, const void *b)
> +{
> +    const char *key = a;
> +    const char *feat = ((struct { const char *n; } *)b)->n;

Urgh - what a cast. There's no connection at all between this and
struct feature_name in libxl_cpuid_parse_config(). I think you want
to move that type declaration out of the function, to also use it
here. But if not, comments on both sides are going to be necessary
imo.

> +    return strncmp(key, feat, strlen(key)) ?: strlen(key) - strlen(feat);

Why not simply

    return strcmp(key, feat);

? Both names are nul-terminated, first and foremost because
otherwise using strlen() wouldn't be appropriate either.

> @@ -342,6 +214,28 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list 
> *cpuid, const char* str)
>          if(!strncmp(str, flag->name, sep - str) && flag->name[sep - str] == 
> 0)
>              break;
>      }
> +    if (flag->name == NULL) {
> +        const struct feature_name *feat;
> +        /* Provide a NUL terminated feature name to the search helper. */
> +        char *name = strndup(str, sep - str);
> +
> +        if (name == NULL)
> +            return 4;
> +
> +        feat = bsearch(name, features, ARRAY_SIZE(features), 
> sizeof(features[0]),
> +                       search_feature);
> +        free(name);
> +
> +        if (feat != NULL) {
> +                f.name = feat->name;
> +                f.leaf = feature_to_cpuid[feat->bit / 32].leaf;
> +                f.subleaf = feature_to_cpuid[feat->bit / 32].subleaf;
> +                f.reg = feature_to_cpuid[feat->bit / 32].reg;
> +                f.bit = feat->bit % 32;
> +                f.length = 1,
> +                flag = &f;

Nit: Too deep indentation throughout here.

Jan