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 > --- > 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_ - 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 -- <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)