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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

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@xxxxxxxxx>
>
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: signature.asc
Description: This is a digitally signed message part

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