Re: [Xen-devel] [PATCH 1/3] xl: convert scheduling related return codes to EXIT_[SUCCESS|FAILURE]

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

Now, to the technical review...

On Fri, 2015-10-23 at 13:18 +0530, Harmandeep Kaur wrote:
> turning scheduling related functions xl exit codes towards using the
> EXIT_[SUCCESS|FAILURE] macros, instead of instead of arbitrary
> numbers
> or libxl return codes.
> 
Subject of the mail: something like "xl: convert exit codes of
scheduling operations to EXIT_[SUCCESS|FAILURE]"

Note the difference between "return codes", which makes one think to
all the 'return'-s off all the functions, and "exit codes", which, IMO,
expresses better that what we care is those 'return'-s and 'exit()'-s
that causes the program to actually exit.

Or, perhaps, given the kind of changes that I'm suggesting you to do in
this review of mine, even something like "xl: improve return and exit
codes of scheduling related functions".

(Yes, I know that the one you used, I kind of suggested it myself.
Still... :-D )

> Signed-off-by: Harmandeep Kaur <write.harmandeep@xxxxxxxxx>
> ---
>  tools/libxl/xl_cmdimpl.c | 67 ++++++++++++++++++++++++--------------
> ----------
>  1 file changed, 33 insertions(+), 34 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 365798b..c215c14 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -5851,13 +5851,13 @@ static int sched_credit_domain_output(int
> domid)
>  
>      if (domid < 0) {
>          printf("%-33s %4s %6s %4s\n", "Name", "ID", "Weight",
> "Cap");
> -        return 0;
> +        return EXIT_SUCCESS;
>      }
>  
In principle, this is ok. However, see below for a more general opinion
on this and similar functions.

>      libxl_domain_sched_params_init(&scinfo);
>      rc = sched_domain_get(LIBXL_SCHEDULER_CREDIT, domid, &scinfo);
>      if (rc)
> -        return rc;
> +        return EXIT_FAILURE;
>
This is right again. However, the actual value returned by
 sched_domain_get() is only used for being returned to the caller,
without your patch. With your patch, it's basically useless.

I think you can just get rid of it, and do something like:

 if (sched_domain_get(LIBXL_SCHEDULER_CREDIT, domid, &scinfo))
     return EXIT_SUCCESS;

(Or maybe just 'return -1', and you keep the 'return 0' in case
domid<0, but again, see below for more details on this.)

Actually, as you probably noticed yourself, when dealing with
sched_rtds_domain_output(), we're missing a call to the dispose
function, both here and in the sched_credit2_domain_output().

So, I think you either get rid of rc and do it like this (and that in
sched_credit2_domain_output() as well):

 if (sched_domain_get(LIBXL_SCHEDULER_CREDIT, domid, &scinfo)) {
     libxl_domain_sched_params_dispose();
     return -1;
 }

Or you do similarly to what sched_rtds_domain_output() does already,
and, again, do it for all the credit, credit2 and rtds variants:

    rc = 0
    ...
    if (sched_domain_get(LIBXL_SCHEDULER_CREDIT, domid, &scinfo)) {
        rc = -1;
        goto out;
    }
    ...
 out:
    libxl_domain_sched_param_dispose();
    return rc;

(again, see below on why -1 and not EXIT_FAILURE.)

Which variant to go for, I'll let you decide. If this were libxl (i.e.,
the library, not the xl program) code, the latter would be the way. In
xl, there are occurrences of both paradigms, and, at least in this
specific case, I personally like the former better.
But really, pick one, and then we'll see what tools maintainers
think. :-)

> @@ -5866,7 +5866,7 @@ static int sched_credit_domain_output(int
> domid)
>          scinfo.cap);
>      free(domname);
>      libxl_domain_sched_params_dispose(&scinfo);
> -    return 0;
> +    return EXIT_SUCCESS;
>  }
>  
Ok. So, about these sched_*_domain_output() functions.

I know that, right now, their return values may actually be used as
exit code, so you've done right changing all them to
EXIT_SUCCESS/FAILURE.

However, their return value is used as exit code only once (e.g., in
this case, in main_sched_credit()), while all the other times they're
called, they are just internal functions.

So, the approach I like best would be as follows:

 - make all them be proper internal functions. That means make them 
   return either 0 (on success) or -1 (on failure). Never 
   EXIT_SUCCESS/FAILURE and never rc or ERROR_<something>

 - change the only case where they're used as "exit functions", e.g., 
   in case of credit, inside main_sched_credit():

     if (!opt_w && !opt_c) { /* output credit scheduler info */
         sched_credit_domain_output(-1);
         if (sched_credit_domain_output(domid))
             return EXIT_FAILURE;
     }

   and, of course, the same in main_sched_credit2() and
   main_sched_rtds().

What do you think?
          
>  static int sched_credit_pool_output(uint32_t poolid)
> @@ -5887,7 +5887,7 @@ static int sched_credit_pool_output(uint32_t
> poolid)
>                 scparam.ratelimit_us);
>      }
>      free(poolname);
> -    return 0;
> +    return EXIT_SUCCESS;
>  }
>  
While you're changing this function, there is another example here of
rc being defined and assigned for no useful reason, in this case, even
before your patch.

Do you mind fixing this as well? That would mean, as above, getting rid
of the rc variable, and putting the call to sched_credit_params_get()
inside the if(), as condition, directly.


Ok, now that we are done with the sched_credit_*_output() functions,
note that sched_credit2_domain_output(), sched_rtds_domain_output(),
sched_credit2_pool_output() and sched_rtds_pool_output() have almost
identical structure to these that we just discussed. So, I'd say that
everything we say, applies to all.

Can you do the suggested restructuring for the whole lot?

> @@ -5981,14 +5980,14 @@ static int
> sched_domain_output(libxl_scheduler sched, int (*output)(int),
>          if (libxl_cpupool_qualifier_to_cpupoolid(ctx, cpupool,
> &poolid, NULL) ||
>              !libxl_cpupoolid_is_valid(ctx, poolid)) {
>              fprintf(stderr, "unknown cpupool \'%s\'\n", cpupool);
> -            return -ERROR_FAIL;
> +            return EXIT_FAILURE;
>          }
>      }
>  
>      info = libxl_list_domain(ctx, &nb_domain);
>      if (!info) {
>          fprintf(stderr, "libxl_list_domain failed.\n");
> -        return 1;
> +        return EXIT_FAILURE;
>      }
>      poolinfo = libxl_list_cpupool(ctx, &n_pools);
>      if (!poolinfo) {
> @@ -6016,7 +6015,7 @@ static int sched_domain_output(libxl_scheduler
> sched, int (*output)(int),
>  
>      libxl_cpupoolinfo_list_free(poolinfo, n_pools);
>      libxl_dominfo_list_free(info, nb_domain);
> -    return 0;
> +    return EXIT_SUCCESS;
>  }
> 
This (sched_domain_output()) is ok, and it's so much better to see it
much more consistent. :-)

However, I don't think I see you changing the call site of this. Right
now it is:

        return -sched_domain_output(LIBXL_SCHEDULER_CREDIT,
                                    sched_credit_domain_output,
                                    sched_credit_pool_output,
                                    cpupool);

But since you are returning EXIT_FAILURE or EXIT_SUCCESS already, we
don't want the '-' sign in front (I know, it's probably harmless, but
it would be weird to keep it).

Likewise for the other times this is called, for RTDS and CREDIT2.

> @@ -6110,7 +6109,7 @@ int main_sched_credit(int argc, char **argv)
>          } else { /* Set scheduling parameters*/
>              rc = sched_credit_params_get(poolid, &scparam);
>              if (rc)
> -                return -rc;
> +                return EXIT_FAILURE;
>  
Here we, one more time, don't need to save the return value of
sched_credit_params_get() in rc.

>              if (opt_t)
>                  scparam.tslice_ms = tslice;
> @@ -6120,7 +6119,7 @@ int main_sched_credit(int argc, char **argv)
>  
>              rc = sched_credit_params_set(poolid, &scparam);
>              if (rc)
> -                return -rc;
> +                return EXIT_FAILURE;
>
And neither we do here.

>          }
>      } else if (!dom) { /* list all domain's credit scheduler info */
>          return -sched_domain_output(LIBXL_SCHEDULER_CREDIT,
> @@ -6144,11 +6143,11 @@ int main_sched_credit(int argc, char **argv)
>              rc = sched_domain_set(domid, &scinfo);
>              libxl_domain_sched_params_dispose(&scinfo);
>              if (rc)
> -                return -rc;
> +                return EXIT_FAILURE;
>
OTOH, here it is ok to use rc (i.e., this specific change is ok).

Thanks and 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)

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel