Skip to contentSkip to conversation

xl: convert exit codes to EXIT_[SUCCESS|FAILURE]

PATCH Superseded Harmandeep Kaur 3 participants 14 messages Oct 24, 2015 – Oct 26, 2015

v2 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Harmandeep Kaur

author
#0 lore raw

v2 cover xl: convert exit codes to EXIT_[SUCCESS|FAILURE]

*Its my "bite size contribution" that is required for applying to the Outreachy program.

*main_foo() is treated somewhat as a regular main(), it is changed to return EXIT_SUCCESS or EXIT_FAILURE.

*functions that are not main_foo(), are changed to do 'return 0' or 'return 1', and then 0/1 is taken care in the main_foo() functions that calls them.

*fuctions in xl.c and xl_cmdimpl.c are changed.

*functions related to scheduling, vcpu, cpupool and parsing are currently changed excluding parse_config_data() which is big enough to deserve its own patch.

*Some discussions about this patch:
https://www.mail-archive.com/[email protected]/msg42850.html

*v1 of this patch
http://lists.xenproject.org/archives/html/xen-devel/2015-10/msg02497.html

Signed-off-by Harmandeep Kaur <[email protected]>

---

author
#1 lore raw

PATCH v2 1/5 xl: convert exit codes to EXIT_[SUCCESS|FAILURE]

turning main function xl exit codes towards using the EXIT_[SUCCESS|FAILURE] macros, instead of instead of arbitrary numbers or libxl return codes.

also includes a document comment in xl.h stating xl process should always return EXIT_FOO and main_* can be treated as main() as if they are returning a process exit status and not a function return value)

Signed-off-by Harmandeep Kaur <[email protected]>
---
 tools/libxl/xl.c | 12 ++++++------
 tools/libxl/xl.h |  6 ++++++
 2 files changed, 12 insertions(+), 6 deletions(-)
tools/libxl/xl.c +6 -6
index 5316ad9..dfae84a 100644
@@ -318,7 +318,7 @@ int main(int argc, char **argv)
             break;
         default:
             fprintf(stderr, "unknown global option\n");
-            exit(2);
+            exit(EXIT_FAILURE);
         }
     }
 

@@ -326,13 +326,13 @@ int main(int argc, char **argv)
 
     if (!cmd) {
         help(NULL);
-        exit(1);
+        exit(EXIT_FAILURE);
     }
     opterr = 0;
 
     logger = xtl_createlogger_stdiostream(stderr, minmsglevel,
         (progress_use_cr ? XTL_STDIOSTREAM_PROGRESS_USE_CR : 0));
-    if (!logger) exit(1);
+    if (!logger) exit(EXIT_FAILURE);
 
     atexit(xl_ctx_free);
 

@@ -355,16 +355,16 @@ int main(int argc, char **argv)
     if (cspec) {
         if (dryrun_only && !cspec->can_dryrun) {
             fprintf(stderr, "command does not implement -N (dryrun) option\n");
-            ret = 1;
+            ret = EXIT_FAILURE;
             goto xit;
         }
         ret = cspec->cmd_impl(argc, argv);
     } else if (!strcmp(cmd, "help")) {
         help(argv[1]);
-        ret = 0;
+        ret = EXIT_SUCCESS;
     } else {
         fprintf(stderr, "command not implemented\n");
-        ret = 1;
+        ret = EXIT_FAILURE;
     }
 
  xit:
tools/libxl/xl.h +6 -0
index 0021112..0533398 100644
@@ -30,6 +30,12 @@ struct cmd_spec {
     char *cmd_option;
 };
 
+/*
+*xl process should always return EXIT_FOO and main_* can be treated
+*as main() as if they are returning a process exit status and not a
+*function return value.
+*/
+
 int main_vcpulist(int argc, char **argv);
 int main_info(int argc, char **argv);
 int main_sharing(int argc, char **argv);
Signature · 1 line
1.9.1
Reviewed-by on v2 1/5
#2 lore raw
Quoted 5 lines · Harmandeep Kaur

turning main function xl exit codes towards using the EXIT_[SUCCESS|FAILURE] macros, instead of instead of arbitrary numbers
or libxl return codes.

I'd say "turning xl main() function exit codes..."

Also in the subject "xl: convert main() exit codes to..."

Quoted 7 lines

also includes a document comment in xl.h stating xl process should always return EXIT_FOO and main_* can be treated as main() as if they are returning a process exit status and not a function return value)

Signed-off-by Harmandeep Kaur <[email protected]>

I also have a (minor) comment to the code but, with the subject, changelog, and comment (see below) fixed, this patch can have:

Reviewed-by Dario Faggioli <[email protected]>

tools/libxl/xl.h struct cmd_spec { · line 38

Quoted 12 lines
tools/libxl/xl.h +6 -0
@@ -30,6 +30,12 @@ struct cmd_spec {
     char *cmd_option;
 };
 
+/*
+*xl process should always return EXIT_FOO and main_* can be treated
+*as main() as if they are returning a process exit status and not a
+*function return value.
+*/
+

I think it is important to be a bit more precise, here. After all, we're explaining people how they should do things, so let's do it properly. :-)

"The xl process should always return either EXIT_SUCCESS or EXIT_FAILURE. main_* functions, implementing the various xl commands, can be treated..."

Regards,
Dario

Signature · 4 lines
<<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)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

#3 lore raw
In reply to Harmandeep Kaur
Quoted 69 lines · Harmandeep Kaur

turning main function xl exit codes towards using the EXIT_[SUCCESS|FAILURE] macros, instead of instead of arbitrary numbers or libxl return codes.

also includes a document comment in xl.h stating xl process should always return EXIT_FOO and main_* can be treated as main() as if they are returning a process exit status and not a function return value)

Signed-off-by Harmandeep Kaur <[email protected]>
---
 tools/libxl/xl.c | 12 ++++++------
 tools/libxl/xl.h |  6 ++++++
 2 files changed, 12 insertions(+), 6 deletions(-)
tools/libxl/xl.c +6 -6
index 5316ad9..dfae84a 100644
@@ -318,7 +318,7 @@ int main(int argc, char **argv)
             break;
         default:
             fprintf(stderr, "unknown global option\n");
-            exit(2);
+            exit(EXIT_FAILURE);
         }
     }
 

@@ -326,13 +326,13 @@ int main(int argc, char **argv)
 
     if (!cmd) {
         help(NULL);
-        exit(1);
+        exit(EXIT_FAILURE);
     }
     opterr = 0;
 
     logger = xtl_createlogger_stdiostream(stderr, minmsglevel,
         (progress_use_cr ? XTL_STDIOSTREAM_PROGRESS_USE_CR : 0));
-    if (!logger) exit(1);
+    if (!logger) exit(EXIT_FAILURE);
 
     atexit(xl_ctx_free);
 

@@ -355,16 +355,16 @@ int main(int argc, char **argv)
     if (cspec) {
         if (dryrun_only && !cspec->can_dryrun) {
             fprintf(stderr, "command does not implement -N (dryrun) option\n");
-            ret = 1;
+            ret = EXIT_FAILURE;
             goto xit;
         }
         ret = cspec->cmd_impl(argc, argv);
     } else if (!strcmp(cmd, "help")) {
         help(argv[1]);
-        ret = 0;
+        ret = EXIT_SUCCESS;
     } else {
         fprintf(stderr, "command not implemented\n");
-        ret = 1;
+        ret = EXIT_FAILURE;
     }
 
  xit:
tools/libxl/xl.h +0 -0
index 0021112..0533398 100644

tools/libxl/xl.h struct cmd_spec { · line 36

Quoted 7 lines
tools/libxl/xl.h +4 -0
@@ -30,6 +30,12 @@ struct cmd_spec {
     char *cmd_option;
 };
 
+/*
+*xl process should always return EXIT_FOO and main_* can be treated
+*as main() as if they are returning a process exit status and not a
+*function return value.

Please correctly format this comment.

/*
* xl process should always return EXIT_FOO and main_* can be treated
* as main() as if they are returning a process exit status and not a
* function return value.
*/

Note the alignment.

Quoted 5 lines
tools/libxl/xl.h +0 -0
 int main_vcpulist(int argc, char **argv);
 int main_info(int argc, char **argv);
 int main_sharing(int argc, char **argv);
Signature · 1 line
1.9.1
author
#4 lore raw

PATCH v2 2/5 xl: improve return and exit codes of scheduling related functions

In reply to Harmandeep Kaur

turning scheduling related functions xl exit codes towards using the EXIT_[SUCCESS|FAILURE] macros, instead of instead of arbitrary numbers or libxl return codes.

Signed-off-by Harmandeep Kaur <[email protected]>
---
 tools/libxl/xl_cmdimpl.c | 147 ++++++++++++++++++++++-------------------------
 1 file changed, 70 insertions(+), 77 deletions(-)
tools/libxl/xl_cmdimpl.c +70 -77
index 646b281..718be54 100644
@@ -5793,18 +5793,15 @@ int main_sharing(int argc, char **argv)
 static int sched_domain_get(libxl_scheduler sched, int domid,
                             libxl_domain_sched_params *scinfo)
 {
-    int rc;
-
-    rc = libxl_domain_sched_params_get(ctx, domid, scinfo);
-    if (rc) {
+    if (libxl_domain_sched_params_get(ctx, domid, scinfo)){
         fprintf(stderr, "libxl_domain_sched_params_get failed.\n");
-        return rc;
+        return 1;
     }
     if (scinfo->sched != sched) {
         fprintf(stderr, "libxl_domain_sched_params_get returned %s not %s.\n",
                 libxl_scheduler_to_string(scinfo->sched),
                 libxl_scheduler_to_string(sched));
-        return ERROR_INVAL;
+        return 1;
     }
 
     return 0;

@@ -5812,42 +5809,38 @@ static int sched_domain_get(libxl_scheduler sched, int domid,
 
 static int sched_domain_set(int domid, const libxl_domain_sched_params *scinfo)
 {
-    int rc;
-
-    rc = libxl_domain_sched_params_set(ctx, domid, scinfo);
-    if (rc)
+    if (libxl_domain_sched_params_set(ctx, domid, scinfo)){
         fprintf(stderr, "libxl_domain_sched_params_set failed.\n");
+        return 1;
+    }
 
-    return rc;
+    return 0;
 }
 
 static int sched_credit_params_set(int poolid, libxl_sched_credit_params *scinfo)
 {
-    int rc;
-
-    rc = libxl_sched_credit_params_set(ctx, poolid, scinfo);
-    if (rc)
+    if (libxl_sched_credit_params_set(ctx, poolid, scinfo)){
         fprintf(stderr, "libxl_sched_credit_params_set failed.\n");
-
-    return rc;
+        return 1;
+    }
+    
+    return 0;
 }
 
 static int sched_credit_params_get(int poolid, libxl_sched_credit_params *scinfo)
 {
-    int rc;
-
-    rc = libxl_sched_credit_params_get(ctx, poolid, scinfo);
-    if (rc)
+    if (libxl_sched_credit_params_get(ctx, poolid, scinfo)){
         fprintf(stderr, "libxl_sched_credit_params_get failed.\n");
+        return 1;
+    }
 
-    return rc;
+    return 0;
 }
 
 static int sched_credit_domain_output(int domid)
 {
     char *domname;
     libxl_domain_sched_params scinfo;
-    int rc;
 
     if (domid < 0) {
         printf("%-33s %4s %6s %4s\n", "Name", "ID", "Weight", "Cap");

@@ -5855,9 +5848,10 @@ static int sched_credit_domain_output(int domid)
     }
 
     libxl_domain_sched_params_init(&scinfo);
-    rc = sched_domain_get(LIBXL_SCHEDULER_CREDIT, domid, &scinfo);
-    if (rc)
-        return rc;
+    if (sched_domain_get(LIBXL_SCHEDULER_CREDIT, domid, &scinfo)){
+        libxl_domain_sched_params_dispose(&scinfo);
+        return 1;
+    }
     domname = libxl_domid_to_name(ctx, domid);
     printf("%-33s %4d %6d %4d\n",
         domname,

@@ -5873,11 +5867,9 @@ static int sched_credit_pool_output(uint32_t poolid)
 {
     libxl_sched_credit_params scparam;
     char *poolname;
-    int rc;
 
     poolname = libxl_cpupoolid_to_name(ctx, poolid);
-    rc = sched_credit_params_get(poolid, &scparam);
-    if (rc) {
+    if (sched_credit_params_get(poolid, &scparam)){
         printf("Cpupool %s: [sched params unavailable]\n",
                poolname);
     } else {

@@ -5895,7 +5887,6 @@ static int sched_credit2_domain_output(
 {
     char *domname;
     libxl_domain_sched_params scinfo;
-    int rc;
 
     if (domid < 0) {
         printf("%-33s %4s %6s\n", "Name", "ID", "Weight");

@@ -5903,9 +5894,10 @@ static int sched_credit2_domain_output(
     }
 
     libxl_domain_sched_params_init(&scinfo);
-    rc = sched_domain_get(LIBXL_SCHEDULER_CREDIT2, domid, &scinfo);
-    if (rc)
-        return rc;
+    if (sched_domain_get(LIBXL_SCHEDULER_CREDIT2, domid, &scinfo)){
+        libxl_domain_sched_params_dispose(&scinfo);
+        return 1;
+    }
     domname = libxl_domid_to_name(ctx, domid);
     printf("%-33s %4d %6d\n",
         domname,

@@ -5921,7 +5913,6 @@ static int sched_rtds_domain_output(
 {
     char *domname;
     libxl_domain_sched_params scinfo;
-    int rc = 0;
 
     if (domid < 0) {
         printf("%-33s %4s %9s %9s\n", "Name", "ID", "Period", "Budget");

@@ -5929,9 +5920,10 @@ static int sched_rtds_domain_output(
     }
 
     libxl_domain_sched_params_init(&scinfo);
-    rc = sched_domain_get(LIBXL_SCHEDULER_RTDS, domid, &scinfo);
-    if (rc)
-        goto out;
+    if (sched_domain_get(LIBXL_SCHEDULER_RTDS, domid, &scinfo)){
+        libxl_domain_sched_params_dispose(&scinfo);
+        return 1;
+    }
 
     domname = libxl_domid_to_name(ctx, domid);
     printf("%-33s %4d %9d %9d\n",

@@ -5940,10 +5932,8 @@ static int sched_rtds_domain_output(
         scinfo.period,
         scinfo.budget);
     free(domname);
-
-out:
     libxl_domain_sched_params_dispose(&scinfo);
-    return rc;
+    return 0;
 }
 
 static int sched_rtds_pool_output(uint32_t poolid)

@@ -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;

@@ -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;
         }
     }

@@ -6080,16 +6068,16 @@ int main_sched_credit(int argc, char **argv)
     if ((cpupool || opt_s) && (dom || opt_w || opt_c)) {
         fprintf(stderr, "Specifying a cpupool or schedparam is not "
                 "allowed with domain options.\n");
-        return 1;
+        return EXIT_FAILURE;
     }
     if (!dom && (opt_w || opt_c)) {
         fprintf(stderr, "Must specify a domain.\n");
-        return 1;
+        return EXIT_FAILURE;
     }
     if (!opt_s && (opt_t || opt_r)) {
         fprintf(stderr, "Must specify schedparam to set schedule "
                 "parameter values.\n");
-        return 1;
+        return EXIT_FAILURE;
     }
 
     if (opt_s) {

@@ -6101,16 +6089,16 @@ int main_sched_credit(int argc, char **argv)
                                                      &poolid, NULL) ||
                 !libxl_cpupoolid_is_valid(ctx, poolid)) {
                 fprintf(stderr, "unknown cpupool \'%s\'\n", cpupool);
-                return -ERROR_FAIL;
+                return EXIT_FAILURE;
             }
         }
 
         if (!opt_t && !opt_r) { /* Output scheduling parameters */
-            return -sched_credit_pool_output(poolid);
+            if (sched_credit_pool_output(poolid))
+                return EXIT_FAILURE;
         } else { /* Set scheduling parameters*/
-            rc = sched_credit_params_get(poolid, &scparam);
-            if (rc)
-                return -rc;
+            if (sched_credit_params_get(poolid, &scparam))
+                return EXIT_FAILURE;
 
             if (opt_t)
                 scparam.tslice_ms = tslice;

@@ -6118,21 +6106,22 @@ int main_sched_credit(int argc, char **argv)
             if (opt_r)
                 scparam.ratelimit_us = ratelimit;
 
-            rc = sched_credit_params_set(poolid, &scparam);
-            if (rc)
-                return -rc;
+            if (sched_credit_params_set(poolid, &scparam))
+                return EXIT_FAILURE;
         }
     } else if (!dom) { /* list all domain's credit scheduler info */
-        return -sched_domain_output(LIBXL_SCHEDULER_CREDIT,
+        if (sched_domain_output(LIBXL_SCHEDULER_CREDIT,
                                     sched_credit_domain_output,
                                     sched_credit_pool_output,
-                                    cpupool);
+                                    cpupool))
+            return EXIT_FAILURE;
     } else {
         uint32_t domid = find_domain(dom);
 
         if (!opt_w && !opt_c) { /* output credit scheduler info */
             sched_credit_domain_output(-1);
-            return -sched_credit_domain_output(domid);
+            if (sched_credit_domain_output(domid))
+                return EXIT_FAILURE;
         } else { /* set credit scheduler paramaters */
             libxl_domain_sched_params scinfo;
             libxl_domain_sched_params_init(&scinfo);

@@ -6144,11 +6133,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;
         }
     }
 
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_sched_credit2(int argc, char **argv)

@@ -6180,24 +6169,26 @@ int main_sched_credit2(int argc, char **argv)
     if (cpupool && (dom || opt_w)) {
         fprintf(stderr, "Specifying a cpupool is not allowed with other "
                 "options.\n");
-        return 1;
+        return EXIT_FAILURE;
     }
     if (!dom && opt_w) {
         fprintf(stderr, "Must specify a domain.\n");
-        return 1;
+        return EXIT_FAILURE;
     }
 
     if (!dom) { /* list all domain's credit scheduler info */
-        return -sched_domain_output(LIBXL_SCHEDULER_CREDIT2,
+        if (sched_domain_output(LIBXL_SCHEDULER_CREDIT2,
                                     sched_credit2_domain_output,
                                     sched_default_pool_output,
-                                    cpupool);
+                                    cpupool))
+            return EXIT_FAILURE;
     } else {
         uint32_t domid = find_domain(dom);
 
         if (!opt_w) { /* output credit2 scheduler info */
             sched_credit2_domain_output(-1);
-            return -sched_credit2_domain_output(domid);
+            if (sched_credit2_domain_output(domid))
+                return EXIT_FAILURE;
         } else { /* set credit2 scheduler paramaters */
             libxl_domain_sched_params scinfo;
             libxl_domain_sched_params_init(&scinfo);

@@ -6207,11 +6198,11 @@ int main_sched_credit2(int argc, char **argv)
             rc = sched_domain_set(domid, &scinfo);
             libxl_domain_sched_params_dispose(&scinfo);
             if (rc)
-                return -rc;
+                return EXIT_FAILURE;
         }
     }
 
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 /*

@@ -6256,27 +6247,29 @@ int main_sched_rtds(int argc, char **argv)
     if (cpupool && (dom || opt_p || opt_b)) {
         fprintf(stderr, "Specifying a cpupool is not allowed with "
                 "other options.\n");
-        return 1;
+        return EXIT_FAILURE;
     }
     if (!dom && (opt_p || opt_b)) {
         fprintf(stderr, "Must specify a domain.\n");
-        return 1;
+        return EXIT_FAILURE;
     }
     if (opt_p != opt_b) {
         fprintf(stderr, "Must specify period and budget\n");
-        return 1;
+        return EXIT_FAILURE;
     }
 
     if (!dom) { /* list all domain's rt scheduler info */
-        return -sched_domain_output(LIBXL_SCHEDULER_RTDS,
+        if (sched_domain_output(LIBXL_SCHEDULER_RTDS,
                                     sched_rtds_domain_output,
                                     sched_rtds_pool_output,
-                                    cpupool);
+                                    cpupool))
+            return EXIT_FAILURE;
     } else {
         uint32_t domid = find_domain(dom);
         if (!opt_p && !opt_b) { /* output rt scheduler info */
             sched_rtds_domain_output(-1);
-            return -sched_rtds_domain_output(domid);
+            if (sched_rtds_domain_output(domid))
+                return EXIT_FAILURE;
         } else { /* set rt scheduler paramaters */
             libxl_domain_sched_params scinfo;
             libxl_domain_sched_params_init(&scinfo);

@@ -6287,11 +6280,11 @@ int main_sched_rtds(int argc, char **argv)
             rc = sched_domain_set(domid, &scinfo);
             libxl_domain_sched_params_dispose(&scinfo);
             if (rc)
-                return -rc;
+                return EXIT_FAILURE;
         }
     }
 
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_domid(int argc, char **argv)
Signature · 1 line
1.9.1
#5 lore raw

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

Quoted 5 lines · Harmandeep Kaur

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...

Quoted 3 lines
Signed-off-by Harmandeep Kaur <[email protected]>

---

...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:

Quoted 30 lines
tools/libxl/xl_cmdimpl.c +0 -1
@@ -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);
tools/libxl/xl_cmdimpl.c +3 -3
-            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)))
Quoted 2 lines
             continue;

I don't think we can just get rid of rc in this way. In fact...

tools/libxl/xl_cmdimpl.c static int sched_domain_output(libxl_scheduler · line 6003

Quoted 10 lines
tools/libxl/xl_cmdimpl.c +1 -2
@@ -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

Signature · 4 lines
<<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)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

author
#6 lore raw

PATCH v2 3/5 xl: improve return and exit codes of vcpu related functions

In reply to Harmandeep Kaur

turning vcpu manipulation functions xl exit codes toward using the EXIT_[SUCCESS|FAILURE] macros, instead of instead of arbitrary numbers or libxl return codes.

Signed-off-by Harmandeep Kaur <[email protected]>
---
 tools/libxl/xl_cmdimpl.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)
tools/libxl/xl_cmdimpl.c +10 -10
index 718be54..f71a7f0 100644
@@ -5316,7 +5316,7 @@ int main_vcpulist(int argc, char **argv)
     }
 
     vcpulist(argc - optind, argv + optind);
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_vcpupin(int argc, char **argv)

@@ -5332,7 +5332,7 @@ int main_vcpupin(int argc, char **argv)
     long vcpuid;
     const char *vcpu, *hard_str, *soft_str;
     char *endptr;
-    int opt, nb_cpu, nb_vcpu, rc = -1;
+    int opt, nb_cpu, nb_vcpu, rc = EXIT_FAILURE;
 
     libxl_bitmap_init(&cpumap_hard);
     libxl_bitmap_init(&cpumap_soft);

@@ -5407,10 +5407,10 @@ int main_vcpupin(int argc, char **argv)
 
         if (ferror(stdout) || fflush(stdout)) {
             perror("stdout");
-            exit(-1);
+            exit(EXIT_FAILURE);
         }
 
-        rc = 0;
+        rc = EXIT_SUCCESS;
         goto out;
     }
 

@@ -5430,7 +5430,7 @@ int main_vcpupin(int argc, char **argv)
         libxl_vcpuinfo_list_free(vcpuinfo, nb_vcpu);
     }
 
-    rc = 0;
+    rc = EXIT_SUCCESS;
  out:
     libxl_bitmap_dispose(&cpumap_soft);
     libxl_bitmap_dispose(&cpumap_hard);

@@ -5459,8 +5459,7 @@ static int vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
         unsigned int host_cpu = libxl_get_max_cpus(ctx);
         libxl_dominfo dominfo;
 
-        rc = libxl_domain_info(ctx, &dominfo, domid);
-        if (rc)
+        if (libxl_domain_info(ctx, &dominfo, domid))
             return 1;
 
         if (max_vcpus > dominfo.vcpu_online && max_vcpus > host_cpu) {

@@ -5473,8 +5472,7 @@ static int vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
         if (rc)
             return 1;
     }
-    rc = libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus);
-    if (rc) {
+    if (libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus)){
         fprintf(stderr, "libxl_cpu_bitmap_alloc failed, rc: %d\n", rc);
         return 1;
     }

@@ -5508,7 +5506,9 @@ int main_vcpuset(int argc, char **argv)
         break;
     }
 
-    return vcpuset(find_domain(argv[optind]), argv[optind + 1], check_host);
+    if (vcpuset(find_domain(argv[optind]), argv[optind + 1], check_host))
+        return EXIT_FAILURE;
+    else return EXIT_SUCCESS;
 }
 
 static void output_xeninfo(void)
Signature · 1 line
1.9.1
Reviewed-by on v2 3/5
#7 lore raw

Re: [PATCH v2 3/5] xl: improve return and exit codes of vcpu related functions

Quoted 5 lines · Harmandeep Kaur

turning vcpu manipulation functions xl exit codes toward using the EXIT_[SUCCESS|FAILURE] macros, instead of instead of arbitrary numbers
or libxl return codes.

Again, distch "xl" from the sentence above.

Again, just one small comment:

Quoted 4 lines

tools/libxl/xl_cmdimpl.c int main_vcpuset(int argc, char **argv) · line 5514

Quoted 10 lines
tools/libxl/xl_cmdimpl.c +3 -1
@@ -5508,7 +5506,9 @@ int main_vcpuset(int argc, char **argv)
         break;
     }
 
-    return vcpuset(find_domain(argv[optind]), argv[optind + 1],
 check_host);
+    if (vcpuset(find_domain(argv[optind]), argv[optind + 1],
 check_host))
+        return EXIT_FAILURE;
+    else return EXIT_SUCCESS;

There's no need of this 'else'.

With these things fixed, this patch is:

Reviewed-by Dario Faggioli <[email protected]>

Thanks and Regards,
Dario

Signature · 4 lines
<<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)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

author
#8 lore raw

PATCH v2 4/5 xl: improve return and exit codes of cpupool related functions

In reply to Harmandeep Kaur

turning cpupools related functions xl exit codes towards using the EXIT_[SUCCESS|FAILURE] macros, instead of instead of arbitrary numbers or libxl return codes.

Signed-off-by Harmandeep Kaur <[email protected]>
---
 tools/libxl/xl_cmdimpl.c | 52 ++++++++++++++++++++++++------------------------
 1 file changed, 26 insertions(+), 26 deletions(-)
tools/libxl/xl_cmdimpl.c +26 -26
index f71a7f0..07b2bcd 100644
@@ -7311,7 +7311,7 @@ int main_cpupoolcreate(int argc, char **argv)
     libxl_bitmap cpumap;
     libxl_uuid uuid;
     libxl_cputopology *topology;
-    int rc = 1;
+    int rc = EXIT_FAILURE;
 
     SWITCH_FOREACH_OPT(opt, "nf:", opts, "cpupool-create", 0) {
     case 'f':

@@ -7481,7 +7481,7 @@ int main_cpupoolcreate(int argc, char **argv)
         }
     }
     /* We made it! */
-    rc = 0;
+    rc = EXIT_SUCCESS;
    
 out_cfg:
     xlu_cfg_destroy(config);

@@ -7518,14 +7518,14 @@ int main_cpupoollist(int argc, char **argv)
         pool = argv[optind];
         if (libxl_name_to_cpupoolid(ctx, pool, &poolid)) {
             fprintf(stderr, "Pool \'%s\' does not exist\n", pool);
-            return 1;
+            return EXIT_FAILURE;
         }
     }
 
     poolinfo = libxl_list_cpupool(ctx, &n_pools);
     if (!poolinfo) {
         fprintf(stderr, "error getting cpupool info\n");
-        return 1;
+        return EXIT_FAILURE;
     }
 
     printf("%-19s", "Name");

@@ -7556,7 +7556,7 @@ int main_cpupoollist(int argc, char **argv)
 
     libxl_cpupoolinfo_list_free(poolinfo, n_pools);
 
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_cpupooldestroy(int argc, char **argv)

@@ -7574,13 +7574,13 @@ int main_cpupooldestroy(int argc, char **argv)
     if (libxl_cpupool_qualifier_to_cpupoolid(ctx, pool, &poolid, NULL) ||
         !libxl_cpupoolid_is_valid(ctx, poolid)) {
         fprintf(stderr, "unknown cpupool '%s'\n", pool);
-        return 1;
+        return EXIT_FAILURE;
     }
 
     if (libxl_cpupool_destroy(ctx, poolid))
-        return 1;
+        return EXIT_FAILURE;
 
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_cpupoolrename(int argc, char **argv)

@@ -7599,17 +7599,17 @@ int main_cpupoolrename(int argc, char **argv)
     if (libxl_cpupool_qualifier_to_cpupoolid(ctx, pool, &poolid, NULL) ||
         !libxl_cpupoolid_is_valid(ctx, poolid)) {
         fprintf(stderr, "unknown cpupool '%s'\n", pool);
-        return 1;
+        return EXIT_FAILURE;
     }
 
     new_name = argv[optind];
 
     if (libxl_cpupool_rename(ctx, new_name, poolid)) {
         fprintf(stderr, "Can't rename cpupool '%s'\n", pool);
-        return 1;
+        return EXIT_FAILURE;
     }
 
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_cpupoolcpuadd(int argc, char **argv)

@@ -7618,7 +7618,7 @@ int main_cpupoolcpuadd(int argc, char **argv)
     const char *pool;
     uint32_t poolid;
     libxl_bitmap cpumap;
-    int rc = 1;
+    int rc = EXIT_FAILURE;
 
     SWITCH_FOREACH_OPT(opt, "", NULL, "cpupool-cpu-add", 2) {
         /* No options */

@@ -7627,7 +7627,7 @@ int main_cpupoolcpuadd(int argc, char **argv)
     libxl_bitmap_init(&cpumap);
     if (libxl_cpu_bitmap_alloc(ctx, &cpumap, 0)) {
         fprintf(stderr, "Unable to allocate cpumap");
-        return 1;
+        return EXIT_FAILURE;
     }
 
     pool = argv[optind++];

@@ -7643,7 +7643,7 @@ int main_cpupoolcpuadd(int argc, char **argv)
     if (libxl_cpupool_cpuadd_cpumap(ctx, poolid, &cpumap))
         fprintf(stderr, "some cpus may not have been added to %s\n", pool);
 
-    rc = 0;
+    rc = EXIT_SUCCESS;
 
 out:
     libxl_bitmap_dispose(&cpumap);

@@ -7656,12 +7656,12 @@ int main_cpupoolcpuremove(int argc, char **argv)
     const char *pool;
     uint32_t poolid;
     libxl_bitmap cpumap;
-    int rc = 1;
+    int rc = EXIT_FAILURE;
 
     libxl_bitmap_init(&cpumap);
     if (libxl_cpu_bitmap_alloc(ctx, &cpumap, 0)) {
         fprintf(stderr, "Unable to allocate cpumap");
-        return 1;
+        return EXIT_FAILURE;
     }
 
     SWITCH_FOREACH_OPT(opt, "", NULL, "cpupool-cpu-remove", 2) {

@@ -7681,7 +7681,7 @@ int main_cpupoolcpuremove(int argc, char **argv)
     if (libxl_cpupool_cpuremove_cpumap(ctx, poolid, &cpumap))
         fprintf(stderr, "some cpus may not have been removed from %s\n", pool);
 
-    rc = 0;
+    rc = EXIT_SUCCESS;
 
 out:
     libxl_bitmap_dispose(&cpumap);

@@ -7706,19 +7706,19 @@ int main_cpupoolmigrate(int argc, char **argv)
     if (libxl_domain_qualifier_to_domid(ctx, dom, &domid) ||
         !libxl_domid_to_name(ctx, domid)) {
         fprintf(stderr, "unknown domain '%s'\n", dom);
-        return 1;
+        return EXIT_FAILURE;
     }
 
     if (libxl_cpupool_qualifier_to_cpupoolid(ctx, pool, &poolid, NULL) ||
         !libxl_cpupoolid_is_valid(ctx, poolid)) {
         fprintf(stderr, "unknown cpupool '%s'\n", pool);
-        return 1;
+        return EXIT_FAILURE;
     }
 
     if (libxl_cpupool_movedomain(ctx, poolid, domid))
-        return 1;
+        return EXIT_FAILURE;
 
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_cpupoolnumasplit(int argc, char **argv)

@@ -7746,13 +7746,13 @@ int main_cpupoolnumasplit(int argc, char **argv)
 
     libxl_dominfo_init(&info);
 
-    rc = 1;
+    rc = EXIT_FAILURE;
 
     libxl_bitmap_init(&cpumap);
     poolinfo = libxl_list_cpupool(ctx, &n_pools);
     if (!poolinfo) {
         fprintf(stderr, "error getting cpupool info\n");
-        return 1;
+        return EXIT_FAILURE;
     }
     poolid = poolinfo[0].poolid;
     sched = poolinfo[0].sched;

@@ -7760,13 +7760,13 @@ int main_cpupoolnumasplit(int argc, char **argv)
 
     if (n_pools > 1) {
         fprintf(stderr, "splitting not possible, already cpupools in use\n");
-        return 1;
+        return EXIT_FAILURE;
     }
 
     topology = libxl_get_cpu_topology(ctx, &n_cpus);
     if (topology == NULL) {
         fprintf(stderr, "libxl_get_topologyinfo failed\n");
-        return 1;
+        return EXIT_FAILURE;
     }
 
     if (libxl_cpu_bitmap_alloc(ctx, &cpumap, 0)) {

@@ -7855,7 +7855,7 @@ int main_cpupoolnumasplit(int argc, char **argv)
         }
     }
 
-    rc = 0;
+    rc = EXIT_SUCCESS;
 
 out:
     libxl_cputopology_list_free(topology, n_cpus);
Signature · 1 line
1.9.1
Reviewed-by on v2 4/5
#9 lore raw

Re: [PATCH v2 4/5] xl: improve return and exit codes of cpupool related functions

Quoted 5 lines · Harmandeep Kaur

turning cpupools related functions xl exit codes towards using the EXIT_[SUCCESS|FAILURE] macros, instead of instead of arbitrary numbers
or libxl return codes.

Ditch "xl" :-)

With that:

Quoted 2 lines
Signed-off-by Harmandeep Kaur <[email protected]>
Reviewed-by Dario Faggioli <[email protected]>

Thanks and Regards,
Dario

Signature · 4 lines
<<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)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

author
#10 lore raw

PATCH v2 5/5 xl: improve return and exit codes of parse related functions

In reply to Harmandeep Kaur

turning parsing related functions xl exit codes towards using the EXIT_[SUCCESS|FAILURE] macros, instead of instead of arbitrary numbers or libxl return codes.

it doesn't include parse_config_data() which is big enough to deserve its own patch

Signed-off-by Harmandeep Kaur <[email protected]>
---
 tools/libxl/xl_cmdimpl.c | 71 ++++++++++++++++++++++--------------------------
 1 file changed, 32 insertions(+), 39 deletions(-)
tools/libxl/xl_cmdimpl.c +32 -39
index 07b2bcd..f176689 100644
@@ -578,10 +578,10 @@ static void parse_disk_config_multistring(XLU_Config **config,
     }
 
     e = xlu_disk_parse(*config, nspecs, specs, disk);
-    if (e == EINVAL) exit(-1);
+    if (e == EINVAL) exit(EXIT_FAILURE);
     if (e) {
         fprintf(stderr,"xlu_disk_parse failed: %s\n",strerror(errno));
-        exit(-1);
+        exit(EXIT_FAILURE);
     }
 }
 

@@ -600,7 +600,7 @@ static void parse_vif_rate(XLU_Config **config, const char *rate,
     if (e == EINVAL || e == EOVERFLOW) exit(-1);
     if (e) {
         fprintf(stderr,"xlu_vif_parse_rate failed: %s\n",strerror(errno));
-        exit(-1);
+        exit(EXIT_FAILURE);
     }
 }
 

@@ -741,19 +741,19 @@ static int parse_range(const char *str, unsigned long *a, unsigned long *b)
 
     *a = *b = strtoul(str, &endptr, 10);
     if (endptr == str || *a == ULONG_MAX)
-        return ERROR_INVAL;
+        return 1;
 
     if (*endptr == '-') {
         nstr = endptr + 1;
 
         *b = strtoul(nstr, &endptr, 10);
         if (endptr == nstr || *b == ULONG_MAX || *b < *a)
-            return ERROR_INVAL;
+            return 1;
     }
 
     /* Valid value or range so far, but we also don't want junk after that */
     if (*endptr != '\0')
-        return ERROR_INVAL;
+        return 1;
 
     return 0;
 }

@@ -841,17 +841,15 @@ static int update_cpumap_range(const char *str, libxl_bitmap *cpumap)
 static int cpurange_parse(const char *cpu, libxl_bitmap *cpumap)
 {
     char *ptr, *saveptr = NULL, *buf = strdup(cpu);
-    int rc = 0;
 
     for (ptr = strtok_r(buf, ",", &saveptr); ptr;
          ptr = strtok_r(NULL, ",", &saveptr)) {
-        rc = update_cpumap_range(ptr, cpumap);
-        if (rc)
+        if (update_cpumap_range(ptr, cpumap))
             break;
     }
     free(buf);
 
-    return rc;
+    return 0;
 }
 
 static void parse_top_level_vnc_options(XLU_Config *config,

@@ -902,7 +900,7 @@ static char *parse_cmdline(XLU_Config *config)
 
     if ((buf || root || extra) && !cmdline) {
         fprintf(stderr, "Failed to allocate memory for cmdline\n");
-        exit(1);
+        exit(EXIT_FAILURE);
     }
 
     return cmdline;

@@ -946,11 +944,11 @@ static void parse_vcpu_affinity(libxl_domain_build_info *b_info,
             libxl_bitmap_init(&vcpu_affinity_array[j]);
             if (libxl_cpu_bitmap_alloc(ctx, &vcpu_affinity_array[j], 0)) {
                 fprintf(stderr, "Unable to allocate cpumap for vcpu %d\n", j);
-                exit(1);
+                exit(EXIT_FAILURE);
             }
 
             if (cpurange_parse(buf, &vcpu_affinity_array[j]))
-                exit(1);
+                exit(EXIT_FAILURE);
 
             j++;
         }

@@ -963,17 +961,17 @@ static void parse_vcpu_affinity(libxl_domain_build_info *b_info,
         libxl_bitmap_init(&vcpu_affinity_array[0]);
         if (libxl_cpu_bitmap_alloc(ctx, &vcpu_affinity_array[0], 0)) {
             fprintf(stderr, "Unable to allocate cpumap for vcpu 0\n");
-            exit(1);
+            exit(EXIT_FAILURE);
         }
 
         if (cpurange_parse(buf, &vcpu_affinity_array[0]))
-            exit(1);
+            exit(EXIT_FAILURE);
 
         for (i = 1; i < b_info->max_vcpus; i++) {
             libxl_bitmap_init(&vcpu_affinity_array[i]);
             if (libxl_cpu_bitmap_alloc(ctx, &vcpu_affinity_array[i], 0)) {
                 fprintf(stderr, "Unable to allocate cpumap for vcpu %d\n", i);
-                exit(1);
+                exit(EXIT_FAILURE);
             }
             libxl_bitmap_copy(ctx, &vcpu_affinity_array[i],
                               &vcpu_affinity_array[0]);

@@ -1064,7 +1062,7 @@ static unsigned long parse_ulong(const char *str)
     val = strtoul(str, &endptr, 10);
     if (endptr == str || val == ULONG_MAX) {
         fprintf(stderr, "xl: failed to convert \"%s\" to number\n", str);
-        exit(1);
+        exit(EXIT_FAILURE);
     }
     return val;
 }

@@ -1086,7 +1084,7 @@ static void parse_vnuma_config(const XLU_Config *config,
     if (libxl_get_physinfo(ctx, &physinfo) != 0) {
         libxl_physinfo_dispose(&physinfo);
         fprintf(stderr, "libxl_get_physinfo failed\n");
-        exit(1);
+        exit(EXIT_FAILURE);
     }
 
     nr_nodes = physinfo.nr_nodes;

@@ -1105,7 +1103,7 @@ static void parse_vnuma_config(const XLU_Config *config,
         libxl_bitmap_init(&vcpu_parsed[i]);
         if (libxl_cpu_bitmap_alloc(ctx, &vcpu_parsed[i], b_info->max_vcpus)) {
             fprintf(stderr, "libxl_node_bitmap_alloc failed.\n");
-            exit(1);
+            exit(EXIT_FAILURE);
         }
     }
 

@@ -1130,7 +1128,7 @@ static void parse_vnuma_config(const XLU_Config *config,
         xlu_cfg_value_get_list(config, vnode_spec, &vnode_config_list, 0);
         if (!vnode_config_list) {
             fprintf(stderr, "xl: cannot get vnode config option list\n");
-            exit(1);
+            exit(EXIT_FAILURE);
         }
 
         for (conf_count = 0;

@@ -1152,7 +1150,7 @@ static void parse_vnuma_config(const XLU_Config *config,
                                            &value_untrimmed)) {
                     fprintf(stderr, "xl: failed to split \"%s\" into pair\n",
                             buf);
-                    exit(1);
+                    exit(EXIT_FAILURE);
                 }
                 trim(isspace, option_untrimmed, &option);
                 trim(isspace, value_untrimmed, &value);

@@ -1162,7 +1160,7 @@ static void parse_vnuma_config(const XLU_Config *config,
                     if (val >= nr_nodes) {
                         fprintf(stderr,
                                 "xl: invalid pnode number: %lu\n", val);
-                        exit(1);
+                        exit(EXIT_FAILURE);
                     }
                     p->pnode = val;
                     libxl_defbool_set(&b_info->numa_placement, false);

@@ -1218,20 +1216,20 @@ static void parse_vnuma_config(const XLU_Config *config,
     if (b_info->max_vcpus != 0) {
         if (b_info->max_vcpus != max_vcpus) {
             fprintf(stderr, "xl: vnuma vcpus and maxvcpus= mismatch\n");
-            exit(1);
+            exit(EXIT_FAILURE);
         }
     } else {
         int host_cpus = libxl_get_online_cpus(ctx);
 
         if (host_cpus < 0) {
             fprintf(stderr, "Failed to get online cpus\n");
-            exit(1);
+            exit(EXIT_FAILURE);
         }
 
         if (host_cpus < max_vcpus) {
             fprintf(stderr, "xl: vnuma specifies more vcpus than pcpus, "\
                     "use maxvcpus= to override this check.\n");
-            exit(1);
+            exit(EXIT_FAILURE);
         }
 
         b_info->max_vcpus = max_vcpus;

@@ -1241,7 +1239,7 @@ static void parse_vnuma_config(const XLU_Config *config,
     if (b_info->max_memkb != LIBXL_MEMKB_DEFAULT &&
         b_info->max_memkb != max_memkb) {
         fprintf(stderr, "xl: maxmem and vnuma memory size mismatch\n");
-        exit(1);
+        exit(EXIT_FAILURE);
     } else
         b_info->max_memkb = max_memkb;
 

@@ -3126,7 +3124,7 @@ static int64_t parse_mem_size_kb(const char *mem)
     kbytes = strtoll(mem, &endptr, 10);
 
     if (strlen(endptr) > 1)
-        return -1;
+        return 1;
 
     switch (tolower((uint8_t)*endptr)) {
     case 't':

@@ -3145,7 +3143,7 @@ static int64_t parse_mem_size_kb(const char *mem)
         kbytes >>= 10;
         break;
     default:
-        return -1;
+        return 1;
     }
 
     return kbytes;

@@ -3259,17 +3257,14 @@ static int def_getopt(int argc, char * const argv[],
 static int set_memory_max(uint32_t domid, const char *mem)
 {
     int64_t memorykb;
-    int rc;
 
     memorykb = parse_mem_size_kb(mem);
-    if (memorykb == -1) {
+    if (memorykb == 1) {
         fprintf(stderr, "invalid memory size: %s\n", mem);
-        exit(3);
+        exit(EXIT_FAILURE);
     }
 
-    rc = libxl_domain_setmaxmem(ctx, domid, memorykb);
-
-    return rc;
+    return libxl_domain_setmaxmem(ctx, domid, memorykb);
 }
 
 int main_memmax(int argc, char **argv)

@@ -3277,7 +3272,6 @@ int main_memmax(int argc, char **argv)
     uint32_t domid;
     int opt = 0;
     char *mem;
-    int rc;
 
     SWITCH_FOREACH_OPT(opt, "", NULL, "mem-max", 2) {
         /* No options */

@@ -3286,8 +3280,7 @@ int main_memmax(int argc, char **argv)
     domid = find_domain(argv[optind]);
     mem = argv[optind + 1];
 
-    rc = set_memory_max(domid, mem);
-    if (rc) {
+    if (set_memory_max(domid, mem)){
         fprintf(stderr, "cannot set domid %d static max memory to : %s\n", domid, mem);
         return 1;
     }

@@ -3300,9 +3293,9 @@ static void set_memory_target(uint32_t domid, const char *mem)
     long long int memorykb;
 
     memorykb = parse_mem_size_kb(mem);
-    if (memorykb == -1)  {
+    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);
Signature · 1 line
1.9.1
#11 lore raw

Re: [PATCH v2 5/5] xl: improve return and exit codes of parse related functions

Quoted 5 lines · Harmandeep Kaur

turning parsing related functions xl exit codes towards using the EXIT_[SUCCESS|FAILURE] macros, instead of instead of arbitrary numbers
or libxl return codes.

I think the changelog, in this case, can be restructured and improved just like I said for patch 2.

Quoted 4 lines

it doesn't include parse_config_data() which is big enough to deserve its
own patch

"Don't touch parse_config_data() which..."

Quoted 2 lines
Signed-off-by Harmandeep Kaur <[email protected]>

Again, this is almost ok, but with some issues:

tools/libxl/xl_cmdimpl.c static void parse_vif_rate(XLU_Config **config, · line 602

Quoted 4 lines
tools/libxl/xl_cmdimpl.c +0 -0
@@ -600,7 +600,7 @@ static void parse_vif_rate(XLU_Config **config,
 const char *rate,
     if (e == EINVAL || e == EOVERFLOW) exit(-1);

What about this one? :-D :-D

Quoted 27 lines
tools/libxl/xl_cmdimpl.c +1 -1
     if (e) {
         fprintf(stderr,"xlu_vif_parse_rate failed:
 %s\n",strerror(errno));
-        exit(-1);
+        exit(EXIT_FAILURE);

     }

}

tools/libxl/xl_cmdimpl.c +1 -1
@@ -3126,7 +3124,7 @@ static int64_t parse_mem_size_kb(const char
 *mem)
     kbytes = strtoll(mem, &endptr, 10);
 
     if (strlen(endptr) > 1)
-        return -1;
+        return 1;
 
     switch (tolower((uint8_t)*endptr)) {

     case 't':
tools/libxl/xl_cmdimpl.c +1 -1
@@ -3145,7 +3143,7 @@ static int64_t parse_mem_size_kb(const char
 *mem)
         kbytes >>= 10;
         break;
     default:
-        return -1;
+        return 1;
     }
Quoted 2 lines
     return kbytes;

I see why you're doing this, and I saw you're took care of the call sites, which is good.

However, in this case, I think the return value should stay -1. Tools people may advise better than me, but it looks like this function is meant at returning the size of some amount of memory, in kilobytes. So, although I agree that it would be very unlikely that someone specifies 1 Kb, we really can't rule it out (not in this patch, at least!).

So, I really think it's better to keep using negative numbers here (on the ground that a negative amount of memory is a clearer indication of an error).

One good thing that you can do in the case of this function is, maybe, adding a comment just above it saying (very quickly, as it's pretty evident and straightforward, IMO) exactly that, i.e., that the functions returns -1 if the parsing fails.

tools/libxl/xl_cmdimpl.c static int def_getopt(int argc, char * const · line 3264

Quoted 11 lines
tools/libxl/xl_cmdimpl.c +1 -2
@@ -3259,17 +3257,14 @@ static int def_getopt(int argc, char * const
 argv[],
 static int set_memory_max(uint32_t domid, const char *mem)
 {
     int64_t memorykb;
-    int rc;
 
     memorykb = parse_mem_size_kb(mem);
-    if (memorykb == -1) {
+    if (memorykb == 1) {

This will therefore be left as it is, of course.

tools/libxl/xl_cmdimpl.c static int def_getopt(int argc, char * const argv[], · line 3266

Quoted 5 lines
tools/libxl/xl_cmdimpl.c +1 -1
         fprintf(stderr, "invalid memory size: %s\n", mem);
-        exit(3);
+        exit(EXIT_FAILURE);
     }

While this is, of course, ok.

Quoted 19 lines
tools/libxl/xl_cmdimpl.c +1 -3
 int main_memmax(int argc, char **argv)

@@ -3277,7 +3272,6 @@ int main_memmax(int argc, char **argv)
     uint32_t domid;
     int opt = 0;
     char *mem;
-    int rc;
 
     SWITCH_FOREACH_OPT(opt, "", NULL, "mem-max", 2) {
         /* No options */

@@ -3286,8 +3280,7 @@ int main_memmax(int argc, char **argv)
     domid = find_domain(argv[optind]);
     mem = argv[optind + 1];
 
-    rc = set_memory_max(domid, mem);
-    if (rc) {
+    if (set_memory_max(domid, mem)){
         fprintf(stderr, "cannot set domid %d static max memory to :
 %s\n", domid, mem);
         return 1;
Quoted 1 line
     }

I'm not sure about this specific change. It is ok, of course, but it seems a bit out of what you declared the scope of this patch was.

In fact, you are fixing and improving error reporting and exit codes, not getting rid of unnecessary variables. Then, yes, in most cases mean we can get rid of those variables, as a result of the declared goal, but in this case there is no return value/exit code involved. At the end, I think you should leave this hunk out of this patch.

Regards,
Dario

Signature · 4 lines
<<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)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

#12 lore raw

Re: [PATCH v2 5/5] xl: improve return and exit codes of parse related functions

In reply to Harmandeep Kaur
Quoted 20 lines · Harmandeep Kaur

turning parsing related functions xl exit codes towards using the EXIT_[SUCCESS|FAILURE] macros, instead of instead of arbitrary numbers
or libxl return codes.

it doesn't include parse_config_data() which is big enough to deserve its
own patch

Signed-off-by Harmandeep Kaur <[email protected]>

libxl_bitmap *cpumap)
static int cpurange_parse(const char *cpu, libxl_bitmap *cpumap)

 {
     char *ptr, *saveptr = NULL, *buf = strdup(cpu);
tools/libxl/xl_cmdimpl.c +0 -1
-    int rc = 0;
 
     for (ptr = strtok_r(buf, ",", &saveptr); ptr;
          ptr = strtok_r(NULL, ",", &saveptr)) {

tools/libxl/xl_cmdimpl.c static int update_cpumap_range(const char *str, libxl_bitmap *cpumap) · line 853

Quoted 10 lines
tools/libxl/xl_cmdimpl.c +2 -3
-        rc = update_cpumap_range(ptr, cpumap);
-        if (rc)
+        if (update_cpumap_range(ptr, cpumap))
             break;
     }
     free(buf);
 
-    return rc;
+    return 0;
 }

Oh, and also, here: I think rc is needed, in this case, to properly deal with the failure of update_cpumap_range(), and poperly propagate that failure to the caller.

If you want to get rid of it, you should do something like this, inside the loop:

 if (update_cpumap_range(ptr, cpumap)) {
     free(buf);
     return 1;

}

Regards,
Dario

Signature · 4 lines
<<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)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

#13 lore raw

Re: [PATCH v2 5/5] xl: improve return and exit codes of parse related functions

In reply to Harmandeep Kaur
Quoted 2 lines · Harmandeep Kaur

turning parsing related functions xl exit codes towards using the EXIT_[SUCCESS|FAILURE] macros, instead of instead of arbitrary numbers

They are constants, not macros -- I'm a being pedantic here. :-)

Quoted 5 lines

or libxl return codes.

it doesn't include parse_config_data() which is big enough to deserve its own patch

Please capitalise first word of the sentence.

Quoted 5 lines
Signed-off-by Harmandeep Kaur <[email protected]>
---
 tools/libxl/xl_cmdimpl.c | 71 ++++++++++++++++++++++--------------------------
 1 file changed, 32 insertions(+), 39 deletions(-)

[...]

Quoted 7 lines
tools/libxl/xl_cmdimpl.c +0 -1
@@ -841,17 +841,15 @@ static int update_cpumap_range(const char *str, libxl_bitmap *cpumap)
 static int cpurange_parse(const char *cpu, libxl_bitmap *cpumap)
 {
     char *ptr, *saveptr = NULL, *buf = strdup(cpu);
-    int rc = 0;
 
     for (ptr = strtok_r(buf, ",", &saveptr); ptr;

tools/libxl/xl_cmdimpl.c static int update_cpumap_range(const char *str, libxl_bitmap *cpumap) · line 852

Quoted 10 lines
tools/libxl/xl_cmdimpl.c +2 -3
          ptr = strtok_r(NULL, ",", &saveptr)) {
-        rc = update_cpumap_range(ptr, cpumap);
-        if (rc)
+        if (update_cpumap_range(ptr, cpumap))
             break;
     }
     free(buf);
 
-    return rc;
+    return 0;

This is not wrong. You return 0 even when there is error.

Quoted 144 lines
tools/libxl/xl_cmdimpl.c +17 -17
 }
 
 static void parse_top_level_vnc_options(XLU_Config *config,

@@ -902,7 +900,7 @@ static char *parse_cmdline(XLU_Config *config)
 
     if ((buf || root || extra) && !cmdline) {
         fprintf(stderr, "Failed to allocate memory for cmdline\n");
-        exit(1);
+        exit(EXIT_FAILURE);
     }
 
     return cmdline;

@@ -946,11 +944,11 @@ static void parse_vcpu_affinity(libxl_domain_build_info *b_info,
             libxl_bitmap_init(&vcpu_affinity_array[j]);
             if (libxl_cpu_bitmap_alloc(ctx, &vcpu_affinity_array[j], 0)) {
                 fprintf(stderr, "Unable to allocate cpumap for vcpu %d\n", j);
-                exit(1);
+                exit(EXIT_FAILURE);
             }
 
             if (cpurange_parse(buf, &vcpu_affinity_array[j]))
-                exit(1);
+                exit(EXIT_FAILURE);
 
             j++;
         }

@@ -963,17 +961,17 @@ static void parse_vcpu_affinity(libxl_domain_build_info *b_info,
         libxl_bitmap_init(&vcpu_affinity_array[0]);
         if (libxl_cpu_bitmap_alloc(ctx, &vcpu_affinity_array[0], 0)) {
             fprintf(stderr, "Unable to allocate cpumap for vcpu 0\n");
-            exit(1);
+            exit(EXIT_FAILURE);
         }
 
         if (cpurange_parse(buf, &vcpu_affinity_array[0]))
-            exit(1);
+            exit(EXIT_FAILURE);
 
         for (i = 1; i < b_info->max_vcpus; i++) {
             libxl_bitmap_init(&vcpu_affinity_array[i]);
             if (libxl_cpu_bitmap_alloc(ctx, &vcpu_affinity_array[i], 0)) {
                 fprintf(stderr, "Unable to allocate cpumap for vcpu %d\n", i);
-                exit(1);
+                exit(EXIT_FAILURE);
             }
             libxl_bitmap_copy(ctx, &vcpu_affinity_array[i],
                               &vcpu_affinity_array[0]);

@@ -1064,7 +1062,7 @@ static unsigned long parse_ulong(const char *str)
     val = strtoul(str, &endptr, 10);
     if (endptr == str || val == ULONG_MAX) {
         fprintf(stderr, "xl: failed to convert \"%s\" to number\n", str);
-        exit(1);
+        exit(EXIT_FAILURE);
     }
     return val;
 }

@@ -1086,7 +1084,7 @@ static void parse_vnuma_config(const XLU_Config *config,
     if (libxl_get_physinfo(ctx, &physinfo) != 0) {
         libxl_physinfo_dispose(&physinfo);
         fprintf(stderr, "libxl_get_physinfo failed\n");
-        exit(1);
+        exit(EXIT_FAILURE);
     }
 
     nr_nodes = physinfo.nr_nodes;

@@ -1105,7 +1103,7 @@ static void parse_vnuma_config(const XLU_Config *config,
         libxl_bitmap_init(&vcpu_parsed[i]);
         if (libxl_cpu_bitmap_alloc(ctx, &vcpu_parsed[i], b_info->max_vcpus)) {
             fprintf(stderr, "libxl_node_bitmap_alloc failed.\n");
-            exit(1);
+            exit(EXIT_FAILURE);
         }
     }
 

@@ -1130,7 +1128,7 @@ static void parse_vnuma_config(const XLU_Config *config,
         xlu_cfg_value_get_list(config, vnode_spec, &vnode_config_list, 0);
         if (!vnode_config_list) {
             fprintf(stderr, "xl: cannot get vnode config option list\n");
-            exit(1);
+            exit(EXIT_FAILURE);
         }
 
         for (conf_count = 0;

@@ -1152,7 +1150,7 @@ static void parse_vnuma_config(const XLU_Config *config,
                                            &value_untrimmed)) {
                     fprintf(stderr, "xl: failed to split \"%s\" into pair\n",
                             buf);
-                    exit(1);
+                    exit(EXIT_FAILURE);
                 }
                 trim(isspace, option_untrimmed, &option);
                 trim(isspace, value_untrimmed, &value);

@@ -1162,7 +1160,7 @@ static void parse_vnuma_config(const XLU_Config *config,
                     if (val >= nr_nodes) {
                         fprintf(stderr,
                                 "xl: invalid pnode number: %lu\n", val);
-                        exit(1);
+                        exit(EXIT_FAILURE);
                     }
                     p->pnode = val;
                     libxl_defbool_set(&b_info->numa_placement, false);

@@ -1218,20 +1216,20 @@ static void parse_vnuma_config(const XLU_Config *config,
     if (b_info->max_vcpus != 0) {
         if (b_info->max_vcpus != max_vcpus) {
             fprintf(stderr, "xl: vnuma vcpus and maxvcpus= mismatch\n");
-            exit(1);
+            exit(EXIT_FAILURE);
         }
     } else {
         int host_cpus = libxl_get_online_cpus(ctx);
 
         if (host_cpus < 0) {
             fprintf(stderr, "Failed to get online cpus\n");
-            exit(1);
+            exit(EXIT_FAILURE);
         }
 
         if (host_cpus < max_vcpus) {
             fprintf(stderr, "xl: vnuma specifies more vcpus than pcpus, "\
                     "use maxvcpus= to override this check.\n");
-            exit(1);
+            exit(EXIT_FAILURE);
         }
 
         b_info->max_vcpus = max_vcpus;

@@ -1241,7 +1239,7 @@ static void parse_vnuma_config(const XLU_Config *config,
     if (b_info->max_memkb != LIBXL_MEMKB_DEFAULT &&
         b_info->max_memkb != max_memkb) {
         fprintf(stderr, "xl: maxmem and vnuma memory size mismatch\n");
-        exit(1);
+        exit(EXIT_FAILURE);
     } else
         b_info->max_memkb = max_memkb;
 

@@ -3126,7 +3124,7 @@ static int64_t parse_mem_size_kb(const char *mem)
     kbytes = strtoll(mem, &endptr, 10);
 
     if (strlen(endptr) > 1)
-        return -1;
+        return 1;
 
     switch (tolower((uint8_t)*endptr)) {
     case 't':

tools/libxl/xl_cmdimpl.c static int64_t parse_mem_size_kb(const char *mem) · line 3146

Quoted 5 lines
tools/libxl/xl_cmdimpl.c +1 -1
@@ -3145,7 +3143,7 @@ static int64_t parse_mem_size_kb(const char *mem)
         kbytes >>= 10;
         break;
     default:
-        return -1;
+        return 1;

This is wrong. Consider that you genuinely need 1 MB ram (well, not likely, but still)?

Quoted 25 lines
tools/libxl/xl_cmdimpl.c +3 -6
     }
 
     return kbytes;

@@ -3259,17 +3257,14 @@ static int def_getopt(int argc, char * const argv[],
 static int set_memory_max(uint32_t domid, const char *mem)
 {
     int64_t memorykb;
-    int rc;
 
     memorykb = parse_mem_size_kb(mem);
-    if (memorykb == -1) {
+    if (memorykb == 1) {
         fprintf(stderr, "invalid memory size: %s\n", mem);
-        exit(3);
+        exit(EXIT_FAILURE);
     }
 
-    rc = libxl_domain_setmaxmem(ctx, domid, memorykb);
-
-    return rc;
+    return libxl_domain_setmaxmem(ctx, domid, memorykb);
 }
 
 int main_memmax(int argc, char **argv)

tools/libxl/xl_cmdimpl.c int main_memmax(int argc, char **argv) · line 3275

Quoted 5 lines
tools/libxl/xl_cmdimpl.c +0 -1
@@ -3277,7 +3272,6 @@ int main_memmax(int argc, char **argv)
     uint32_t domid;
     int opt = 0;
     char *mem;
-    int rc;

In this patch you have removed a bunch of rc's, which is a bit out of scope, really. You can just leave them alone to reduce overall size of this patch.

Wei.