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.harmand...@gmail.com> > --- > ...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)
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel