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

2015-10-26 Thread Dario Faggioli 
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 
>
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, , 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
-- 
<> (Raistlin Majere)
-
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R Ltd., Cambridge (UK)



signature.asc
Description: This is a digitally signed message part
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-10-26 Thread Dario Faggioli 
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 

> 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, ",", ); ptr;
>   ptr = strtok_r(NULL, ",", )) {
> -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
-- 
<> (Raistlin Majere)
-
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R Ltd., Cambridge (UK)



signature.asc
Description: This is a digitally signed message part
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-10-26 Thread 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 
> ---
>  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, ",", ); ptr;
>   ptr = strtok_r(NULL, ",", )) {
> -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(_affinity_array[j]);
>  if (libxl_cpu_bitmap_alloc(ctx, _affinity_array[j], 0)) {
>  fprintf(stderr, "Unable to allocate cpumap for vcpu %d\n", 
> j);
> -exit(1);
> +exit(EXIT_FAILURE);
>  }
>  
>  if (cpurange_parse(buf, _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(_affinity_array[0]);
>  if (libxl_cpu_bitmap_alloc(ctx, _affinity_array[0], 0)) {
>  fprintf(stderr, "Unable to allocate cpumap for vcpu 0\n");
> -exit(1);
> +exit(EXIT_FAILURE);
>  }
>  
>  if (cpurange_parse(buf, _affinity_array[0]))
> -exit(1);
> +exit(EXIT_FAILURE);
>  
>  for (i = 1; i < b_info->max_vcpus; i++) {
>  libxl_bitmap_init(_affinity_array[i]);
>  if (libxl_cpu_bitmap_alloc(ctx, _affinity_array[i], 0)) {
>  fprintf(stderr, "Unable to allocate cpumap for vcpu %d\n", 
> i);
> -exit(1);
> +exit(EXIT_FAILURE);
>  }
>  libxl_bitmap_copy(ctx, _affinity_array[i],
>_affinity_array[0]);
> @@ -1064,7 +1062,7 @@ static unsigned long parse_ulong(const char *str)
>  val = strtoul(str, , 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, ) != 0) {
>  libxl_physinfo_dispose();
>  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(_parsed[i]);
>  if (libxl_cpu_bitmap_alloc(ctx, _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, _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,
> _untrimmed)) {
>  fprintf(stderr, "xl: failed to split \"%s\" into pair\n",
>  buf);
> -exit(1);
> +exit(EXIT_FAILURE);
>  }
>  trim(isspace, option_untrimmed, );
>  trim(isspace, value_untrimmed, );
> @@ -1162,7 +1160,7 @@ static void