On Fri, 2015-07-10 at 23:52 -0500, Chong Li wrote:
> Change main_sched_rtds and related output functions to support
> per-VCPU settings.
> 
This patch also looks nice. A few comments provided inline.


> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index c858068..da7c8a6 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -5630,6 +5630,37 @@ static int sched_domain_set(int domid, const libxl_domain_sched_params *scinfo)
>      return rc;
>  }
>  
> +static int sched_vcpu_get(libxl_scheduler sched, int domid,
> +                            libxl_vcpu_sched_params *scinfo)
> +{
> +    int rc;
> +
> +    rc = libxl_vcpu_sched_params_get(ctx, domid, scinfo);
> +    if (rc) {
> +        fprintf(stderr, "libxl_vcpu_sched_params_get failed.\n");
> +        return rc;
> +    }
> +    if (scinfo->sched != sched) {
> +        fprintf(stderr, "libxl_vcpu_sched_params_get returned %s not %s.\n",
> +                libxl_scheduler_to_string(scinfo->sched),
> +                libxl_scheduler_to_string(sched));
> +        return ERROR_INVAL;
>
We are in xl, so there's no need to use libxl error code. It's probably
not strictly forbidden either, and xl itself is really inconsistent
about this, I know.

Personally, I never use them, and I think that using them may be
confusing, and create even more inconsistency.

> +static int sched_vcpu_set(int domid, const libxl_vcpu_sched_params *scinfo)
> +{
> +    int rc;
> +
> +    rc = libxl_vcpu_sched_params_set(ctx, domid, scinfo);
> +    if (rc)
> +        fprintf(stderr, "libxl_vcpu_sched_params_set failed.\n");
> +
> +    return rc;
>
In xl, when failing, most of the time we exit(-1), or something like
that. So, it's important that you log what's happening, but then there's
no need to save the return codes and propagate them.

>  static int sched_credit_params_set(int poolid, libxl_sched_credit_params *scinfo)
>  {
>      int rc;
> @@ -5786,6 +5817,38 @@ out:
>      return rc;
>  }
>  
> +static int sched_rtds_vcpu_output(
> +    int domid, libxl_vcpu_sched_params *scinfo)
> +{
> +    char *domname;
> +    int rc = 0;
> +    int i;
> +
> +   if (domid < 0) {
>
Style (indentation). This is not the only instance of something like
this in this patch.

> +        printf("%-33s %4s %4s %9s %9s\n", "Name", "ID",
> +                        "VCPU", "Period", "Budget");
> +        return 0;
> +    }
> +
> +    rc = sched_vcpu_get(LIBXL_SCHEDULER_RTDS, domid, scinfo);
> +    if (rc)
> +        goto out;
> +
> +    domname = libxl_domid_to_name(ctx, domid);
> +     for( i = 0; i < scinfo->num_vcpus; i++ ) {
>
Style:

  for (i = 0; i < scinfo->num_vcpus; i++) {

and indentation, I think.

> +        printf("%-33s %4d %4d %9"PRIu32" %9"PRIu32"\n",
> +            domname,
> +            domid,
> +            scinfo->vcpus[i].vcpuid,
> +            scinfo->vcpus[i].period,
> +            scinfo->vcpus[i].budget);
> +    }
> +    free(domname);
> +
> +out:
> +    return rc;
> +}
> +

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)