| Message ID | 1456318407-3635-2-git-send-email-write.harmandeep@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 24, Harmandeep Kaur wrote: > - exit(2); > + exit(EXIT_FAILURE); The commit message should state why changing the value is fine. I dont know if anyone actually cares about the exact value. Olaf
On Wed, 2016-02-24 at 14:55 +0100, Olaf Hering wrote: > On Wed, Feb 24, Harmandeep Kaur wrote: > > > - exit(2); > > + exit(EXIT_FAILURE); > > The commit message should state why changing the value is fine. > I dont know if anyone actually cares about the exact value. > This has been discussed before here on the mailing list, and there already are patches committed that do this same thing in a bunch of cases (and, of course, the aim is consistently doing it everywhere). Previous discussion is here: http://lists.xen.org/archives/html/xen-devel/2015-10/msg02497.html http://lists.xen.org/archives/html/xen-devel/2015-10/msg02501.html http://lists.xen.org/archives/html/xen-devel/2015-10/msg02509.html And, even before, here: http://lists.xenproject.org/archives/html/xen-devel/2015-03/msg01336.html http://lists.xenproject.org/archives/html/xen-devel/2015-03/msg01341.html I'm quit sure there is no one doing anything useful with exit codes right now, as they're so inconsistent that it would just be plain impossible! :-) Anyway, Harmandeep, can you summarize better (and maybe reference) these previous discussions and your previous related work in the cover letter of this series (and any other similar one that may follow)? Regards, Dario
On Wed, 2016-02-24 at 18:23 +0530, Harmandeep Kaur wrote: > Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com> > --- > tools/libxl/xl_cmdimpl.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > index 116363d..8f5a2f4 100644 > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -172,7 +172,7 @@ static uint32_t find_domain(const char *p) > rc = libxl_domain_qualifier_to_domid(ctx, p, &domid); > if (rc) { > fprintf(stderr, "%s is an invalid domain identifier > (rc=%d)\n", p, rc); > - exit(2); > + exit(EXIT_FAILURE); > I'm not sure find_domain() is a "memory related functions", so I wouldn't change it in this patch. On the other hand, there is parse_mem_size_kb(), which returns -1 on failure, or the amount of memory, on success. That is ok, IMO, so, don't change it. However, I think you can take the chance of this patch to add just a couple of line of comments, above its signature, explaining that it does exactly that. Also, there is freemem(), which also does look "memory related" to me. And in its case, it indeed uses libxl's error codes while it should -- being an internal function-- just use the 0/1 convention. I think you can 'fix' it in this patch. > @@ -3275,7 +3275,7 @@ static int set_memory_max(uint32_t domid, const > char *mem) > memorykb = parse_mem_size_kb(mem); > if (memorykb == -1) { > fprintf(stderr, "invalid memory size: %s\n", mem); > - exit(3); > + exit(EXIT_FAILURE); > } > > rc = libxl_domain_setmaxmem(ctx, domid, memorykb); > The statement right below this one is: return rc; set_memory_max() is an internal function so, again, it should retunr 0/1 to its caller, rather than a libxl error code. The rest of this patch looks ok to me. Thanks and Regards, Dario
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c index 116363d..8f5a2f4 100644 --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -172,7 +172,7 @@ static uint32_t find_domain(const char *p) rc = libxl_domain_qualifier_to_domid(ctx, p, &domid); if (rc) { fprintf(stderr, "%s is an invalid domain identifier (rc=%d)\n", p, rc); - exit(2); + exit(EXIT_FAILURE); } common_domname = libxl_domid_to_name(ctx, domid); return domid; @@ -3275,7 +3275,7 @@ static int set_memory_max(uint32_t domid, const char *mem) memorykb = parse_mem_size_kb(mem); if (memorykb == -1) { fprintf(stderr, "invalid memory size: %s\n", mem); - exit(3); + exit(EXIT_FAILURE); } rc = libxl_domain_setmaxmem(ctx, domid, memorykb); @@ -3300,10 +3300,10 @@ int main_memmax(int argc, char **argv) rc = set_memory_max(domid, mem); if (rc) { fprintf(stderr, "cannot set domid %d static max memory to : %s\n", domid, mem); - return 1; + return EXIT_FAILURE; } - return 0; + return EXIT_SUCCESS; } static void set_memory_target(uint32_t domid, const char *mem) @@ -3313,7 +3313,7 @@ static void set_memory_target(uint32_t domid, const char *mem) memorykb = parse_mem_size_kb(mem); if (memorykb == -1) { fprintf(stderr, "invalid memory size: %s\n", mem); - exit(3); + exit(EXIT_FAILURE); } libxl_set_memory_target(ctx, domid, memorykb, 0, /* enforce */ 1); @@ -3333,7 +3333,7 @@ int main_memset(int argc, char **argv) mem = argv[optind + 1]; set_memory_target(domid, mem); - return 0; + return EXIT_SUCCESS; } static int cd_insert(uint32_t domid, const char *virtdev, char *phys)
Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com> --- tools/libxl/xl_cmdimpl.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)