xl: convert scheduling related return codes to EXIT_[SUCCESS|FAILURE]

PATCH Superseded Harmandeep Kaur 3 participants 11 messages Oct 23, 2015

Share & tools

Harmandeep Kaur

author

2015-10-23 07:49 UTC #0 lore raw

PATCH 1/3 xl: convert scheduling related return codes to EXIT_[SUCCESS|FAILURE]

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 | 67 ++++++++++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 34 deletions(-)

tools/libxl/xl_cmdimpl.c +33 ~~-34~~

index 365798b..c215c14 100644
@@ -5851,13 +5851,13 @@ static int sched_credit_domain_output(int domid)
 
     if (domid < 0) {
         printf("%-33s %4s %6s %4s\n", "Name", "ID", "Weight", "Cap");
-        return 0;
+        return EXIT_SUCCESS;
     }
 
     libxl_domain_sched_params_init(&scinfo);
     rc = sched_domain_get(LIBXL_SCHEDULER_CREDIT, domid, &scinfo);
     if (rc)
-        return rc;
+        return EXIT_FAILURE;
     domname = libxl_domid_to_name(ctx, domid);
     printf("%-33s %4d %6d %4d\n",
         domname,

@@ -5866,7 +5866,7 @@ static int sched_credit_domain_output(int domid)
         scinfo.cap);
     free(domname);
     libxl_domain_sched_params_dispose(&scinfo);
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 static int sched_credit_pool_output(uint32_t poolid)

@@ -5887,7 +5887,7 @@ static int sched_credit_pool_output(uint32_t poolid)
                scparam.ratelimit_us);
     }
     free(poolname);
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 static int sched_credit2_domain_output(

@@ -5899,13 +5899,13 @@ static int sched_credit2_domain_output(
 
     if (domid < 0) {
         printf("%-33s %4s %6s\n", "Name", "ID", "Weight");
-        return 0;
+        return EXIT_SUCCESS;
     }
 
     libxl_domain_sched_params_init(&scinfo);
     rc = sched_domain_get(LIBXL_SCHEDULER_CREDIT2, domid, &scinfo);
     if (rc)
-        return rc;
+        return EXIT_FAILURE;
     domname = libxl_domid_to_name(ctx, domid);
     printf("%-33s %4d %6d\n",
         domname,

@@ -5913,7 +5913,7 @@ static int sched_credit2_domain_output(
         scinfo.weight);
     free(domname);
     libxl_domain_sched_params_dispose(&scinfo);
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 static int sched_rtds_domain_output(

@@ -5925,13 +5925,14 @@ static int sched_rtds_domain_output(
 
     if (domid < 0) {
         printf("%-33s %4s %9s %9s\n", "Name", "ID", "Period", "Budget");
-        return 0;
+        return EXIT_SUCCESS;
     }
 
     libxl_domain_sched_params_init(&scinfo);
     rc = sched_domain_get(LIBXL_SCHEDULER_RTDS, domid, &scinfo);
     if (rc)
-        goto out;
+        libxl_domain_sched_params_dispose(&scinfo);
+        return EXIT_FAILURE;
 
     domname = libxl_domid_to_name(ctx, domid);
     printf("%-33s %4d %9d %9d\n",

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

@@ -5954,7 +5953,7 @@ static int sched_rtds_pool_output(uint32_t poolid)
     printf("Cpupool %s: sched=RTDS\n", poolname);
 
     free(poolname);
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 static int sched_default_pool_output(uint32_t poolid)

@@ -5965,7 +5964,7 @@ static int sched_default_pool_output(uint32_t poolid)
     printf("Cpupool %s:\n",
            poolname);
     free(poolname);
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 static int sched_domain_output(libxl_scheduler sched, int (*output)(int),

@@ -5981,14 +5980,14 @@ static int sched_domain_output(libxl_scheduler sched, int (*output)(int),
         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 EXIT_FAILURE;
         }
     }
 
     info = libxl_list_domain(ctx, &nb_domain);
     if (!info) {
         fprintf(stderr, "libxl_list_domain failed.\n");
-        return 1;
+        return EXIT_FAILURE;
     }
     poolinfo = libxl_list_cpupool(ctx, &n_pools);
     if (!poolinfo) {

@@ -6016,7 +6015,7 @@ static int sched_domain_output(libxl_scheduler sched, int (*output)(int),
 
     libxl_cpupoolinfo_list_free(poolinfo, n_pools);
     libxl_dominfo_list_free(info, nb_domain);
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 /*

@@ -6080,16 +6079,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,7 +6100,7 @@ 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;
             }
         }

@@ -6110,7 +6109,7 @@ int main_sched_credit(int argc, char **argv)
         } else { /* Set scheduling parameters*/
             rc = sched_credit_params_get(poolid, &scparam);
             if (rc)
-                return -rc;
+                return EXIT_FAILURE;
 
             if (opt_t)
                 scparam.tslice_ms = tslice;

@@ -6120,7 +6119,7 @@ int main_sched_credit(int argc, char **argv)
 
             rc = sched_credit_params_set(poolid, &scparam);
             if (rc)
-                return -rc;
+                return EXIT_FAILURE;
         }
     } else if (!dom) { /* list all domain's credit scheduler info */
         return -sched_domain_output(LIBXL_SCHEDULER_CREDIT,

@@ -6144,11 +6143,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,11 +6179,11 @@ 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 */

@@ -6207,11 +6206,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,15 +6255,15 @@ 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 */

@@ -6287,11 +6286,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

Harmandeep Kaur

author

2015-10-23 07:49 UTC #1 lore raw

PATCH 2/3 xl: convert vcpu related return codes to EXIT_[SUCCESS|FAILURE]

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 | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

tools/libxl/xl_cmdimpl.c +22 ~~-22~~

index c215c14..2cb4fe8 100644
@@ -946,11 +946,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 +963,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]);

@@ -1086,7 +1086,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 +1105,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 +1130,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 +1152,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 +1162,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 +1218,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 +1241,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;

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

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

@@ -5434,7 +5434,7 @@ int main_vcpupin(int argc, char **argv)
  out:
     libxl_bitmap_dispose(&cpumap_soft);
     libxl_bitmap_dispose(&cpumap_hard);
-    return rc;
+    return rc ? EXIT_FAILURE : EXIT_SUCCESS;
 }
 
 static int vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)

@@ -5448,7 +5448,7 @@ static int vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
     max_vcpus = strtoul(nr_vcpus, &endptr, 10);
     if (nr_vcpus == endptr) {
         fprintf(stderr, "Error: Invalid argument.\n");
-        return 1;
+        return EXIT_FAILURE;
     }
 
     /*

@@ -5461,7 +5461,7 @@ static int vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
 
         rc = libxl_domain_info(ctx, &dominfo, domid);
         if (rc)
-            return 1;
+            return EXIT_FAILURE;
 
         if (max_vcpus > dominfo.vcpu_online && max_vcpus > host_cpu) {
             fprintf(stderr, "You are overcommmitting! You have %d physical" \

@@ -5471,12 +5471,12 @@ static int vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
         }
         libxl_dominfo_dispose(&dominfo);
         if (rc)
-            return 1;
+            return EXIT_FAILURE;
     }
     rc = libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus);
     if (rc) {
         fprintf(stderr, "libxl_cpu_bitmap_alloc failed, rc: %d\n", rc);
-        return 1;
+        return EXIT_FAILURE;
     }
     for (i = 0; i < max_vcpus; i++)
         libxl_bitmap_set(&cpumap, i);

@@ -5489,7 +5489,7 @@ static int vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
                 " rc: %d\n", domid, max_vcpus, rc);
 
     libxl_bitmap_dispose(&cpumap);
-    return rc ? 1 : 0;
+    return rc ? EXIT_FAILURE : EXIT_SUCCESS;
 }
 
 int main_vcpuset(int argc, char **argv)

Signature · 1 line

1.9.1

Dario Faggioli

2015-10-23 10:41 UTC #2 lore raw

Re: [PATCH 2/3] xl: convert vcpu related return codes to EXIT_[SUCCESS|FAILURE]

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.

So, this patch is, technically, mostly fine. The observations on the subject made when reviewing patch 1 applies here too, of course.

Something on the selection of functions the functions. The title says the patch will address "vcpu related functions". However, there are a bunch of parse_*() functions in the diff.

I appreciate that, for instance, parse_vcpu_affinity() can be considered a vcpu related function, but that applies a lot less to
parse_vnuma_config(), IMO. I'd therefore exclude the latter from this
patch.

Alternatively, get rid of both and do another patch specifically for dealing with parse_*() items. There are not many of them that are actual exit paths and/or contain calls to exit() (parse_config_data() is big enough, and a bit more complex, that it may well deserve its own patch, and you can leave it alone, just mention that in the changelog).

A few more comments below.

tools/libxl/xl_cmdimpl.c static int vcpuset(uint32_t domid, const char* · line 5466

Quoted 8 lines

tools/libxl/xl_cmdimpl.c +1 -1

@@ -5461,7 +5461,7 @@ static int vcpuset(uint32_t domid, const char*
 nr_vcpus, int check_host)
 
         rc = libxl_domain_info(ctx, &dominfo, domid);
         if (rc)
-            return 1;
+            return EXIT_FAILURE;

if (libxl_domain_info(ctx, &dominfo, domid))

     return EXIT_FAILURE;

Quoted 9 lines

(quoted diff) +0 -0

         if (max_vcpus > dominfo.vcpu_online && max_vcpus > host_cpu)

{
fprintf(stderr, "You are overcommmitting! You have %d
physical" \

tools/libxl/xl_cmdimpl.c +0 -0

@@ -5471,12 +5471,12 @@ static int vcpuset(uint32_t domid, const
 char* nr_vcpus, int check_host)
         }
         libxl_dominfo_dispose(&dominfo);
         if (rc)

tools/libxl/xl_cmdimpl.c static int vcpuset(uint32_t domid, const · line 5482

Quoted 10 lines

tools/libxl/xl_cmdimpl.c +2 -2

-            return 1;
+            return EXIT_FAILURE;
     }
     rc = libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus);
     if (rc) {
         fprintf(stderr, "libxl_cpu_bitmap_alloc failed, rc: %d\n",
 rc);
-        return 1;
+        return EXIT_FAILURE;

if (libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus))

     return EXIT_FAILURE;

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

Harmandeep Kaur

author

2015-10-23 07:49 UTC #3 lore raw

PATCH 3/3 xl: convert cpupool related return codes to EXIT_[SUCCESS|FAILURE]

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 | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

tools/libxl/xl_cmdimpl.c +22 ~~-22~~

index 2cb4fe8..c7009cf 100644
@@ -7497,7 +7497,7 @@ out:
     free(name);
     free(config_data);
     free(extra_config);
-    return rc;
+    return rc ? EXIT_FAILURE : EXIT_SUCCESS;
 }
 
 int main_cpupoollist(int argc, char **argv)

@@ -7524,14 +7524,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");

@@ -7562,7 +7562,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)

@@ -7580,13 +7580,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)

@@ -7605,17 +7605,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)

@@ -7633,7 +7633,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++];

@@ -7653,7 +7653,7 @@ int main_cpupoolcpuadd(int argc, char **argv)
 
 out:
     libxl_bitmap_dispose(&cpumap);
-    return rc;
+    return rc ? EXIT_FAILURE : EXIT_SUCCESS;
 }
 
 int main_cpupoolcpuremove(int argc, char **argv)

@@ -7667,7 +7667,7 @@ int main_cpupoolcpuremove(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;
     }
 
     SWITCH_FOREACH_OPT(opt, "", NULL, "cpupool-cpu-remove", 2) {

@@ -7691,7 +7691,7 @@ int main_cpupoolcpuremove(int argc, char **argv)
 
 out:
     libxl_bitmap_dispose(&cpumap);
-    return rc;
+    return rc ? EXIT_FAILURE : EXIT_SUCCESS;
 }
 
 int main_cpupoolmigrate(int argc, char **argv)

@@ -7712,19 +7712,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)

@@ -7758,7 +7758,7 @@ int main_cpupoolnumasplit(int argc, char **argv)
     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;

@@ -7766,13 +7766,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)) {

@@ -7869,7 +7869,7 @@ out:
     libxl_dominfo_dispose(&info);
     free(name);
 
-    return rc;
+    return rc ? EXIT_FAILURE : EXIT_SUCCESS;
 }
 
 int main_getenforce(int argc, char **argv)

Signature · 1 line

1.9.1

Dario Faggioli

2015-10-23 12:11 UTC #4 lore raw

Re: [PATCH 3/3] xl: convert cpupool related return codes to EXIT_[SUCCESS|FAILURE]

And here we are at the last patch of this series.

Allow me to say that this is quite good a first contribution! Thanks for this, and I'm looking forward to seeing version 2!! :-D

About this patch, a few comments below.

Quoted 10 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.
Signed-off-by Harmandeep Kaur <[email protected]>

tools/libxl/xl_cmdimpl.c out: · line 7502

Quoted 7 lines

tools/libxl/xl_cmdimpl.c +1 -1

@@ -7497,7 +7497,7 @@ out:
     free(name);
     free(config_data);
     free(extra_config);
-    return rc;
+    return rc ? EXIT_FAILURE : EXIT_SUCCESS;
 }

I think you can just initialize rc with EXIT_FAILURE, assign EXIT_SUCCESS to it near the end, if everything went ok, and then keep the 'return rc';

Quoted 9 lines

tools/libxl/xl_cmdimpl.c +0 -0

 int main_cpupooldestroy(int argc, char **argv)

@@ -7580,13 +7580,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);

tools/libxl/xl_cmdimpl.c +0 -1

-        return 1;

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

Quoted 10 lines

tools/libxl/xl_cmdimpl.c +3 -2

+        return EXIT_FAILURE;
     }
 
     if (libxl_cpupool_destroy(ctx, poolid))
-        return 1;
+        return EXIT_FAILURE;
 
-    return 0;
+    return EXIT_SUCCESS;
 }

For this one: I've sent a patch for another reason yesterday, and while there I did the exit code adjustment myself. So, update your tree and, if my patch has been committed already, just skip this function.

https://www.mail-archive.com/[email protected]/msg42850.html

Which brings up a question: what git tree are you using for development? You should stay either on master or staging branches (and I recommend staging) of the official repository:

http://wiki.xenproject.org/wiki/Xen_Project_Repositories

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

Quoted 7 lines

tools/libxl/xl_cmdimpl.c +1 -1

@@ -7653,7 +7653,7 @@ int main_cpupoolcpuadd(int argc, char **argv)
 
 out:
     libxl_bitmap_dispose(&cpumap);
-    return rc;
+    return rc ? EXIT_FAILURE : EXIT_SUCCESS;

Same as already said for main_cpupoolcreate, just us rc.

tools/libxl/xl_cmdimpl.c int main_cpupoolcpuremove(int argc, char · line 7696

Quoted 8 lines

tools/libxl/xl_cmdimpl.c +1 -1

@@ -7691,7 +7691,7 @@ int main_cpupoolcpuremove(int argc, char
 **argv)
 
 out:
     libxl_bitmap_dispose(&cpumap);
-    return rc;
+    return rc ? EXIT_FAILURE : EXIT_SUCCESS;

And here.

Quoted 30 lines

tools/libxl/xl_cmdimpl.c +1 -1

 int main_cpupoolnumasplit(int argc, char **argv)

@@ -7758,7 +7758,7 @@ int main_cpupoolnumasplit(int argc, char
 **argv)
     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;

tools/libxl/xl_cmdimpl.c +2 -2

@@ -7766,13 +7766,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)) {

tools/libxl/xl_cmdimpl.c out: · line 7874

Quoted 7 lines

tools/libxl/xl_cmdimpl.c +1 -1

@@ -7869,7 +7869,7 @@ out:
     libxl_dominfo_dispose(&info);
     free(name);
 
-    return rc;
+    return rc ? EXIT_FAILURE : EXIT_SUCCESS;
 }

And here too.

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

Harmandeep Kaur

author

2015-10-23 12:33 UTC #5 lore raw

Re: [PATCH 3/3] xl: convert cpupool related return codes to EXIT_[SUCCESS|FAILURE]

Quoted 23 lines · Dario Faggioli

Quoted 9 lines
int main_cpupooldestroy(int argc, char **argv)
(quoted diff) +0 -0
@@ -7580,13 +7580,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);
tools/libxl/xl_cmdimpl.c +0 -1
-        return 1;
tools/libxl/xl_cmdimpl.c int main_cpupooldestroy(int argc, char **argv) · line 7590
Quoted 10 lines
tools/libxl/xl_cmdimpl.c +3 -2
+        return EXIT_FAILURE;
     }
 
     if (libxl_cpupool_destroy(ctx, poolid))
-        return 1;
+        return EXIT_FAILURE;
 
-    return 0;
+    return EXIT_SUCCESS;
 }
For this one: I've sent a patch for another reason yesterday, and while there I did the exit code adjustment myself. So, update your tree and, if my patch has been committed already, just skip this function.

Quoted 7 lines

https://www.mail-archive.com/[email protected]/msg42850.html
Which brings up a question: what git tree are you using for development? You should stay either on master or staging branches (and I recommend staging) of the official repository:
http://wiki.xenproject.org/wiki/Xen_Project_Repositories

I was on master branch, now switching to staging.

Thank you so much for reviewing my patch and helping on this tight timeline. And regarding your other questions (patch 1 and 2) I will be answering as I digest all the information you just passed :)

Again, thank you Dario. I really appreciate your help.

Warmest Regards :)
Harman

Quoted 58 lines

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

Quoted 7 lines

tools/libxl/xl_cmdimpl.c +1 -1

@@ -7653,7 +7653,7 @@ int main_cpupoolcpuadd(int argc, char **argv)
 
 out:
     libxl_bitmap_dispose(&cpumap);
-    return rc;
+    return rc ? EXIT_FAILURE : EXIT_SUCCESS;

Same as already said for main_cpupoolcreate, just us rc.

tools/libxl/xl_cmdimpl.c int main_cpupoolcpuremove(int argc, char · line 7696

Quoted 8 lines

tools/libxl/xl_cmdimpl.c +1 -1

@@ -7691,7 +7691,7 @@ int main_cpupoolcpuremove(int argc, char
 **argv)
 
 out:
     libxl_bitmap_dispose(&cpumap);
-    return rc;
+    return rc ? EXIT_FAILURE : EXIT_SUCCESS;

And here.

Quoted 30 lines

tools/libxl/xl_cmdimpl.c +1 -1

 int main_cpupoolnumasplit(int argc, char **argv)

@@ -7758,7 +7758,7 @@ int main_cpupoolnumasplit(int argc, char
 **argv)
     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;

tools/libxl/xl_cmdimpl.c +2 -2

@@ -7766,13 +7766,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)) {

tools/libxl/xl_cmdimpl.c out: · line 7874

Quoted 7 lines

tools/libxl/xl_cmdimpl.c +1 -1

@@ -7869,7 +7869,7 @@ out:
     libxl_dominfo_dispose(&info);
     free(name);
 
-    return rc;
+    return rc ? EXIT_FAILURE : EXIT_SUCCESS;
 }

And here too.

Quoted 9 lines

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)

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

Dario Faggioli

2015-10-23 08:30 UTC #6 lore raw

In reply to Harmandeep Kaur

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.

So, it's probably worth mentioning that this is Harman's "bite size contribution" that is required for applying to the Outreachy program.

Harman, when you send a patch series, as you did here (thanks and good job doing it so quickly :-) ), you want to include a 'cover letter'. That is an introductory email, often labelled (usually automatically by git tools) as patch 0 of the series itself. In it, you give a brief explanation of what the series is meant at, and any kind of information you think the people reviewing the series should have, but that wouldn't fit in the various patches' changelogs, code comments, etc.

For instance, in this case, you could have mentioned (among other things) that this is your small contribution for the sake of applying to Outreachy right in there. :-)

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

Ian Campbell

2015-10-23 09:56 UTC #7 lore raw

Quoted 9 lines · Dario Faggioli

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.
So, it's probably worth mentioning that this is Harman's "bite size contribution" that is required for applying to the Outreachy program.

Quoted 7 lines

Harman, when you send a patch series, as you did here (thanks and good job doing it so quickly :-) ), you want to include a 'cover letter'. That is an introductory email, often labelled (usually automatically by git tools) as patch 0 of the series itself. In it, you give a brief explanation of what the series is meant at, and any kind of information you think the people reviewing the series should have, but that wouldn't fit in the various patches' changelogs, code comments, etc.

In particular in this case I would like to know whether all xl functions now use EXIT_SUCCESS/FAILURE or if there is still inconsistencies/work to be done.

I don't see any existing use of of EXIT_* in xl*.

Quoted 7 lines

For instance, in this case, you could have mentioned (among other things) that this is your small contribution for the sake of applying to Outreachy right in there. :-)
Thanks and Regards,
Dario

Dario Faggioli

2015-10-23 10:03 UTC #8 lore raw

Quoted 15 lines · Ian Campbell

Quoted 4 lines · Dario Faggioli
Harman, when you send a patch series, as you did here (thanks and good
job doing it so quickly :-) ), you want to include a 'cover letter'. That
Quoted 8 lines
is an introductory email, often labelled (usually automatically by git
tools) as patch 0 of the series itself. In it, you give a brief explanation of what the series is meant at, and any kind of information
you think the people reviewing the series should have, but that wouldn't
fit in the various patches' changelogs, code comments, etc.

Quoted 6 lines

In particular in this case I would like to know whether all xl functions
now use EXIT_SUCCESS/FAILURE or if there is still inconsistencies/work to
be done.

Exactly, Harman, these are the sort of things you'd include in a cover letter for a series like this.

Quoted 2 lines

I don't see any existing use of of EXIT_* in xl*.

I sent a patch turning one function into doing so just yesterday. :-)

We (me, you and Wei) talked about it a while back (see the changelog of that patch, it has the links):

https://www.mail-archive.com/[email protected]/msg42850.html

And yes, I have it in my todo list to convert all of them. I'm fine with committing to do that for 4.7.

This effort from Harman is just me using my todo list items as proposals for preliminary contribution for the sake of applying to Outreacy. :-)

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

Ian Campbell

2015-10-23 10:16 UTC #9 lore raw

Quoted 25 lines · Dario Faggioli

Quoted 15 lines · Ian Campbell
Quoted 4 lines · Dario Faggioli
Harman, when you send a patch series, as you did here (thanks and good
job doing it so quickly :-) ), you want to include a 'cover letter'. That
Quoted 8 lines
is an introductory email, often labelled (usually automatically by git
tools) as patch 0 of the series itself. In it, you give a brief explanation of what the series is meant at, and any kind of information
you think the people reviewing the series should have, but that wouldn't
fit in the various patches' changelogs, code comments, etc.
Quoted 6 lines
In particular in this case I would like to know whether all xl functions
now use EXIT_SUCCESS/FAILURE or if there is still inconsistencies/work to
be done.
Exactly, Harman, these are the sort of things you'd include in a cover letter for a series like this.

Quoted 8 lines

Quoted 2 lines
I don't see any existing use of of EXIT_* in xl*.
I sent a patch turning one function into doing so just yesterday. :-)
We (me, you and Wei) talked about it a while back (see the changelog of that patch, it has the links):
https://www.mail-archive.com/[email protected]/msg42850.html

Those links to previous discussions like are certainly the sort of rationale which ought to appear in either a commit log (or a patch #0 if one exists) as part of the rationale for a change.

Ian.

Dario Faggioli

2015-10-23 10:16 UTC #10 lore raw

In reply to Harmandeep Kaur

Now, to the technical review...

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.

Subject of the mail: something like "xl: convert exit codes of scheduling operations to EXIT_[SUCCESS|FAILURE]"

Note the difference between "return codes", which makes one think to all the 'return'-s off all the functions, and "exit codes", which, IMO, expresses better that what we care is those 'return'-s and 'exit()'-s that causes the program to actually exit.

Or, perhaps, given the kind of changes that I'm suggesting you to do in this review of mine, even something like "xl: improve return and exit codes of scheduling related functions".

(Yes, I know that the one you used, I kind of suggested it myself. Still... :-D )

Quoted 11 lines

Signed-off-by Harmandeep Kaur <[email protected]>
---
 tools/libxl/xl_cmdimpl.c | 67 ++++++++++++++++++++++++--------------
----------
 1 file changed, 33 insertions(+), 34 deletions(-)
tools/libxl/xl_cmdimpl.c +0 -0
index 365798b..c215c14 100644

tools/libxl/xl_cmdimpl.c static int sched_credit_domain_output(int · line 5858

Quoted 9 lines

tools/libxl/xl_cmdimpl.c +1 -1

@@ -5851,13 +5851,13 @@ static int sched_credit_domain_output(int
 domid)
 
     if (domid < 0) {
         printf("%-33s %4s %6s %4s\n", "Name", "ID", "Weight",
 "Cap");
-        return 0;
+        return EXIT_SUCCESS;
     }

In principle, this is ok. However, see below for a more general opinion on this and similar functions.

tools/libxl/xl_cmdimpl.c static int sched_credit_domain_output(int domid)

Quoted 6 lines

tools/libxl/xl_cmdimpl.c +1 -1

     libxl_domain_sched_params_init(&scinfo);
     rc = sched_domain_get(LIBXL_SCHEDULER_CREDIT, domid, &scinfo);
     if (rc)
-        return rc;
+        return EXIT_FAILURE;

This is right again. However, the actual value returned by
sched_domain_get() is only used for being returned to the caller,
without your patch. With your patch, it's basically useless.

I think you can just get rid of it, and do something like:

if (sched_domain_get(LIBXL_SCHEDULER_CREDIT, domid, &scinfo))

     return EXIT_SUCCESS;

(Or maybe just 'return -1', and you keep the 'return 0' in case domid<0, but again, see below for more details on this.)

Actually, as you probably noticed yourself, when dealing with sched_rtds_domain_output(), we're missing a call to the dispose function, both here and in the sched_credit2_domain_output().

So, I think you either get rid of rc and do it like this (and that in sched_credit2_domain_output() as well):

 if (sched_domain_get(LIBXL_SCHEDULER_CREDIT, domid, &scinfo)) {
     libxl_domain_sched_params_dispose();
     return -1;

}

Or you do similarly to what sched_rtds_domain_output() does already, and, again, do it for all the credit, credit2 and rtds variants:

    rc = 0
    ...
    if (sched_domain_get(LIBXL_SCHEDULER_CREDIT, domid, &scinfo)) {
        rc = -1;
        goto out;
    }
    ...
 out:
    libxl_domain_sched_param_dispose();
    return rc;

(again, see below on why -1 and not EXIT_FAILURE.)

Which variant to go for, I'll let you decide. If this were libxl (i.e., the library, not the xl program) code, the latter would be the way. In xl, there are occurrences of both paradigms, and, at least in this specific case, I personally like the former better.
But really, pick one, and then we'll see what tools maintainers think. :-)

tools/libxl/xl_cmdimpl.c static int sched_credit_domain_output(int · line 5872

Quoted 9 lines

tools/libxl/xl_cmdimpl.c +1 -1

@@ -5866,7 +5866,7 @@ static int sched_credit_domain_output(int
 domid)
         scinfo.cap);
     free(domname);
     libxl_domain_sched_params_dispose(&scinfo);
-    return 0;
+    return EXIT_SUCCESS;
 }

Ok. So, about these sched_*_domain_output() functions.

I know that, right now, their return values may actually be used as exit code, so you've done right changing all them to EXIT_SUCCESS/FAILURE.

However, their return value is used as exit code only once (e.g., in this case, in main_sched_credit()), while all the other times they're called, they are just internal functions.

So, the approach I like best would be as follows:

- make all them be proper internal functions. That means make them return either 0 (on success) or -1 (on failure). Never EXIT_SUCCESS/FAILURE and never rc or ERROR_<something>

- change the only case where they're used as "exit functions", e.g., in case of credit, inside main_sched_credit():

     if (!opt_w && !opt_c) { /* output credit scheduler info */
         sched_credit_domain_output(-1);
         if (sched_credit_domain_output(domid))
             return EXIT_FAILURE;
     }

and, of course, the same in main_sched_credit2() and main_sched_rtds().

What do you think?

tools/libxl/xl_cmdimpl.c static int sched_credit_pool_output(uint32_t · line 5893

Quoted 10 lines

static int sched_credit_pool_output(uint32_t poolid)

tools/libxl/xl_cmdimpl.c +1 -1

@@ -5887,7 +5887,7 @@ static int sched_credit_pool_output(uint32_t
 poolid)
                scparam.ratelimit_us);
     }
     free(poolname);
-    return 0;
+    return EXIT_SUCCESS;
 }

While you're changing this function, there is another example here of rc being defined and assigned for no useful reason, in this case, even before your patch.

Do you mind fixing this as well? That would mean, as above, getting rid of the rc variable, and putting the call to sched_credit_params_get() inside the if(), as condition, directly.

Ok, now that we are done with the sched_credit_*_output() functions, note that sched_credit2_domain_output(), sched_rtds_domain_output(), sched_credit2_pool_output() and sched_rtds_pool_output() have almost identical structure to these that we just discussed. So, I'd say that everything we say, applies to all.

Can you do the suggested restructuring for the whole lot?

Quoted 20 lines

tools/libxl/xl_cmdimpl.c +0 -0

@@ -5981,14 +5980,14 @@ static int
 sched_domain_output(libxl_scheduler sched, int (*output)(int),
         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 +2 -2

-            return -ERROR_FAIL;
+            return EXIT_FAILURE;
         }
     }
 
     info = libxl_list_domain(ctx, &nb_domain);
     if (!info) {
         fprintf(stderr, "libxl_list_domain failed.\n");
-        return 1;
+        return EXIT_FAILURE;
     }
     poolinfo = libxl_list_cpupool(ctx, &n_pools);
     if (!poolinfo) {

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

Quoted 8 lines

tools/libxl/xl_cmdimpl.c +1 -1

@@ -6016,7 +6015,7 @@ static int sched_domain_output(libxl_scheduler
 sched, int (*output)(int),
 
     libxl_cpupoolinfo_list_free(poolinfo, n_pools);
     libxl_dominfo_list_free(info, nb_domain);
-    return 0;
+    return EXIT_SUCCESS;
 }

This (sched_domain_output()) is ok, and it's so much better to see it much more consistent. :-)

However, I don't think I see you changing the call site of this. Right now it is:

        return -sched_domain_output(LIBXL_SCHEDULER_CREDIT,
                                    sched_credit_domain_output,
                                    sched_credit_pool_output,
                                    cpupool);

But since you are returning EXIT_FAILURE or EXIT_SUCCESS already, we don't want the '-' sign in front (I know, it's probably harmless, but it would be weird to keep it).

Likewise for the other times this is called, for RTDS and CREDIT2.

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

Quoted 7 lines

tools/libxl/xl_cmdimpl.c +1 -1

@@ -6110,7 +6109,7 @@ int main_sched_credit(int argc, char **argv)
         } else { /* Set scheduling parameters*/
             rc = sched_credit_params_get(poolid, &scparam);
             if (rc)
-                return -rc;
+                return EXIT_FAILURE;

Here we, one more time, don't need to save the return value of sched_credit_params_get() in rc.

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

Quoted 9 lines

tools/libxl/xl_cmdimpl.c +1 -1

             if (opt_t)
                 scparam.tslice_ms = tslice;

@@ -6120,7 +6119,7 @@ int main_sched_credit(int argc, char **argv)
 
             rc = sched_credit_params_set(poolid, &scparam);
             if (rc)
-                return -rc;
+                return EXIT_FAILURE;

And neither we do here.

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

Quoted 10 lines

tools/libxl/xl_cmdimpl.c +0 -0

         }
     } else if (!dom) { /* list all domain's credit scheduler info */

return -sched_domain_output(LIBXL_SCHEDULER_CREDIT,

tools/libxl/xl_cmdimpl.c +1 -1

@@ -6144,11 +6143,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;

OTOH, here it is ok to use rc (i.e., this specific change is ok).

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