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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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

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