Re: [Xen-devel] [PATCH v2 2/5] xl: improve return and exit codes of scheduling related functions

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Sat, 2015-10-24 at 11:01 +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.
> 
"turning scheduling related xl functions exit codes..."

However, it is already clear from the "xl:" tag in the subject that
you'll be dealing with xl code, so I think you can just remove "xl"
from here.

Also, I'd (quickly) mention the fact that you are actually doing two
conversions in this patch:
 - for main_*: arbitrary --> EXIT_SUCCESS|EXIT_FAILURe
 - for internal fucntion: arbitrary --> 0/1

Finally, when sending new versions of a patch, it is rather useful that
you include:
 - in the cover letter, a summary of what changed, overall, in that
particular new version of the series (big-ish changes, and/or changes
related on the series structure, like new patch being added, etc.);
 - in each patch, a summary of what changed in that particular patch.
That's helpful for people doing the reviews, as it make quite easi to
check whether you actually considered all points that where raised
during previous iteration.

The latter, you can put (I usually do it in bulleted list form) in...
> Signed-off-by: Harmandeep Kaur <write.harmandeep@xxxxxxxxx>
> ---
>
...here, i.e., after the "---" mark just below the Signed-off-by: tag.
This way, git will throw it away when committing the patch, which is a
good thing (in fact, we want the summary to be there during review, but
we don't want it in `git log').

About the patch, this is really good, and we're almost there (AFAICT).
I think I found an issue, though, here:

> @@ -5975,13 +5965,12 @@ static int
> sched_domain_output(libxl_scheduler sched, int (*output)(int),
>      libxl_cpupoolinfo *poolinfo = NULL;
>      uint32_t poolid;
>      int nb_domain, n_pools = 0, i, p;
> -    int rc = 0;
>  
>      if (cpupool) {
>          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 1;
>          }
>      }
>  
> @@ -5994,10 +5983,10 @@ static int
> sched_domain_output(libxl_scheduler sched, int (*output)(int),
>      if (!poolinfo) {
>          fprintf(stderr, "error getting cpupool info\n");
>          libxl_dominfo_list_free(info, nb_domain);
> -        return -ERROR_NOMEM;
> +        return 1;
>      }
>  
> -    for (p = 0; !rc && (p < n_pools); p++) {
> +    for (p = 0; p < n_pools; p++) {
>          if ((poolinfo[p].sched != sched) ||
>              (cpupool && (poolid != poolinfo[p].poolid)))
>              continue;
>
I don't think we can just get rid of rc in this way. In fact...


> @@ -6008,8 +5997,7 @@ static int sched_domain_output(libxl_scheduler
> sched, int (*output)(int),
>          for (i = 0; i < nb_domain; i++) {
>              if (info[i].cpupool != poolinfo[p].poolid)
>                  continue;
> -            rc = output(info[i].domid);
> -            if (rc)
> +            if (output(info[i].domid))
>                  break;
>
... if the call to whatever output points fails, we break out from the
inner loop, but not from the outer one, while the original code wanted
us to leave that one too.

So, I think in this case you should keep rc, or use another strategy,
involving 'return'-s or 'goto'-s (but I think just keeping rc is the
easiest and better solution).

It is quite possible that I suggested you to do this during v1's
review. If yes, that was because I must have missed the "!rc" part of
the outer loop condition... Sorry for that. :-P

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