On Tue, 2018-09-18 at 08:03 +0200, Juergen Gross wrote:
> Add a new xl command "cpupool-set-parameters" and cpupool config file
> support for setting per-cpupool generic parameters.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
>
Seems good to me. Couple of questions.

Question one: what about getting (and displaying, I guess in
cpupoolinfo) the cpupool parameters?

> --- a/tools/xl/xl_cpupool.c
> +++ b/tools/xl/xl_cpupool.c
> @@ -615,6 +625,35 @@ out:
>      return rc;
>  }
>  
> +int main_cpupoolsetparameters(int argc, char **argv)
> +{
> +    int opt;
> +    const char *pool;
> +    char *params;
> +    uint32_t poolid;
> +
> +    SWITCH_FOREACH_OPT(opt, "", NULL, "cpupool-set-parameters", 2) {
> +        /* No options */
> +    }
> +
> +    pool = argv[optind++];
> +    if (libxl_cpupool_qualifier_to_cpupoolid(ctx, pool, &poolid,
> NULL) ||
> +        !libxl_cpupoolid_is_valid(ctx, poolid)) {
> +        fprintf(stderr, "unknown cpupool '%s'\n", pool);
> +        return EXIT_FAILURE;
> +    }
> +
Since we know that we, AFAIUI, never allow changing the parameters for
a cpupool with domains in it already, shall we test this here, and bail
early, with a specific error message, without even trying going down in
Xen?

I know it's sort-of duplicating checks with what's in the hypervisor,
but I think it would be a common mistake, that it's worth trying to
prevent/address specifically.

> +    params = argv[optind];
> +
> +    if (libxl_cpupool_set_parameters(ctx, poolid, params)) {
> +        fprintf(stderr, "cannot set parameters: %s\n", params);
> +        fprintf(stderr, "Use \"xl dmesg\" to look for possible
> reason.\n");
> +        return EXIT_FAILURE;
> +    }
> +
> +    return EXIT_SUCCESS;
> +}
>
Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/