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

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

[Harmandeep Kaur]

*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/xen-devel@lists.xen.org/msg42850.html

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

Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
---

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

[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 <write.harmandeep@gmail.com>
---
 tools/libxl/xl.c | 12 ++++++------
 tools/libxl/xl.h |  6 ++++++
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index 5316ad9..dfae84a 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -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:
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index 0021112..0533398 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -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);
-- 
1.9.1

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1723 bytes --]

On Sat, 2015-10-24 at 11:01 +0530, Harmandeep Kaur wrote:
> 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..."

> 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 <write.harmandeep@gmail.com>
>
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 <dario.faggioli@citrix.com>

> --- a/tools/libxl/xl.h
> +++ b/tools/libxl/xl.h
> @@ -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
-- 
<<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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Wei Liu]

On Sat, Oct 24, 2015 at 11:01:32AM +0530, Harmandeep Kaur wrote:
> 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 <write.harmandeep@gmail.com>
> ---
>  tools/libxl/xl.c | 12 ++++++------
>  tools/libxl/xl.h |  6 ++++++
>  2 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
> index 5316ad9..dfae84a 100644
> --- a/tools/libxl/xl.c
> +++ b/tools/libxl/xl.c
> @@ -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:
> diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
> index 0021112..0533398 100644
> --- a/tools/libxl/xl.h
> +++ b/tools/libxl/xl.h
> @@ -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.

>  int main_vcpulist(int argc, char **argv);
>  int main_info(int argc, char **argv);
>  int main_sharing(int argc, char **argv);
> -- 
> 1.9.1

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

[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 <write.harmandeep@gmail.com>
---
 tools/libxl/xl_cmdimpl.c | 147 ++++++++++++++++++++++-------------------------
 1 file changed, 70 insertions(+), 77 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 646b281..718be54 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -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)
-- 
1.9.1

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 4080 bytes --]

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


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[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 <write.harmandeep@gmail.com>
---
 tools/libxl/xl_cmdimpl.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 718be54..f71a7f0 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -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)
-- 
1.9.1

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1148 bytes --]

On Sat, 2015-10-24 at 11:01 +0530, Harmandeep Kaur wrote:
> 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:

> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c

> @@ -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 <dario.faggioli@citrix.com>

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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[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 <write.harmandeep@gmail.com>
---
 tools/libxl/xl_cmdimpl.c | 52 ++++++++++++++++++++++++------------------------
 1 file changed, 26 insertions(+), 26 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index f71a7f0..07b2bcd 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -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);
-- 
1.9.1

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 684 bytes --]

On Sat, 2015-10-24 at 11:01 +0530, Harmandeep Kaur wrote:
> 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:

> Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[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 <write.harmandeep@gmail.com>
---
 tools/libxl/xl_cmdimpl.c | 71 ++++++++++++++++++++++--------------------------
 1 file changed, 32 insertions(+), 39 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 07b2bcd..f176689 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -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);
-- 
1.9.1

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 4242 bytes --]

On Sat, 2015-10-24 at 11:01 +0530, Harmandeep Kaur wrote:
> 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.

> it doesn't include parse_config_data() which is big enough to deserve
> its
> own patch
> 
"Don't touch parse_config_data() which..."

> Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
>
Again, this is almost ok, but with some issues:

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

>      if (e) {
>          fprintf(stderr,"xlu_vif_parse_rate failed:
> %s\n",strerror(errno));
> -        exit(-1);
> +        exit(EXIT_FAILURE);
>      }
>  }

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

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

>          fprintf(stderr, "invalid memory size: %s\n", mem);
> -        exit(3);
> +        exit(EXIT_FAILURE);
>      }
> 
While this is, of course, ok.
 
>  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;
>      }
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
-- 
<<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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1528 bytes --]

On Sat, 2015-10-24 at 11:01 +0530, Harmandeep Kaur wrote:
> 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 <write.harmandeep@gmail.com>

> 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;
>  }
>  
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
-- 
<<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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Wei Liu]

On Sat, Oct 24, 2015 at 11:01:36AM +0530, Harmandeep Kaur wrote:
> 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. :-)

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

> Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
> ---
>  tools/libxl/xl_cmdimpl.c | 71 ++++++++++++++++++++++--------------------------
>  1 file changed, 32 insertions(+), 39 deletions(-)
> 
[...]
> @@ -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;

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

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

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

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

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.