diff mbox

[05/10] xl_cmdimpl: improve return codes for memset commands

Message ID 1459514413-18682-6-git-send-email-paulinaszubarczyk@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paulina Szubarczyk April 1, 2016, 12:40 p.m. UTC
- Use EXIT_{SUCCESS,FAILURE} for main_mem*() function
 - Use 0/1 as return values of set_memory_{max,target}

Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
---
 tools/libxl/xl_cmdimpl.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

Comments

Roger Pau Monné April 1, 2016, 2:33 p.m. UTC | #1
On Fri, 1 Apr 2016, Paulina Szubarczyk wrote:

>  - Use EXIT_{SUCCESS,FAILURE} for main_mem*() function
>  - Use 0/1 as return values of set_memory_{max,target}
> 
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
> ---
>  tools/libxl/xl_cmdimpl.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 7ee6953..31f037f 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -3380,15 +3380,15 @@ static int set_memory_max(uint32_t domid, const char *mem)
>      memorykb = parse_mem_size_kb(mem);
>      if (memorykb == -1) {
>          fprintf(stderr, "invalid memory size: %s\n", mem);
> -        exit(3);
> +        exit(EXIT_FAILURE);

IMHO (I would like to hear others' opinion) but I think you can just 
return 1 here instead of exiting.

>      }
>  
>      if (libxl_domain_setmaxmem(ctx, domid, memorykb)) {
>          fprintf(stderr, "cannot set domid %d static max memory to : %s\n", domid, mem);
> -        return EXIT_FAILURE;
> +        return 1;
>      }
>  
> -    return EXIT_SUCCESS;
> +    return 0;
>  }
>  
>  int main_memmax(int argc, char **argv)
> @@ -3404,7 +3404,11 @@ int main_memmax(int argc, char **argv)
>      domid = find_domain(argv[optind]);
>      mem = argv[optind + 1];
>  
> -    return set_memory_max(domid, mem);
> +    if (set_memory_max(domid, mem)) {
> +        return EXIT_FAILURE;
> +    }
> +
> +    return EXIT_SUCCESS;
>  }
>  
>  static int set_memory_target(uint32_t domid, const char *mem)
> @@ -3414,15 +3418,15 @@ static int set_memory_target(uint32_t domid, const char *mem)
>      memorykb = parse_mem_size_kb(mem);
>      if (memorykb == -1)  {
>          fprintf(stderr, "invalid memory size: %s\n", mem);
> -        exit(3);
> +        exit(EXIT_FAILURE);

Same here.

>      }
>  
>      if (libxl_set_memory_target(ctx, domid, memorykb, 0, /* enforce */ 1)) {
>          fprintf(stderr, "cannot set domid %d dynamic max memory to : %s\n", domid, mem);
> -        return EXIT_FAILURE;
> +        return 1;
>      }
>  
> -    return EXIT_SUCCESS;
> +    return 0;
>  }
>  
>  int main_memset(int argc, char **argv)
> @@ -3438,7 +3442,11 @@ int main_memset(int argc, char **argv)
>      domid = find_domain(argv[optind]);
>      mem = argv[optind + 1];
>  
> -    return set_memory_target(domid, mem);
> +    if (set_memory_target(domid, mem)) {
> +        return EXIT_FAILURE;
> +    }
> +
> +    return EXIT_SUCCESS;
>  }
>  
>  static int cd_insert(uint32_t domid, const char *virtdev, char *phys)
> -- 
> 1.9.1
>
George Dunlap April 4, 2016, 11:38 a.m. UTC | #2
On 01/04/16 15:33, Roger Pau Monné wrote:
> On Fri, 1 Apr 2016, Paulina Szubarczyk wrote:
> 
>>  - Use EXIT_{SUCCESS,FAILURE} for main_mem*() function
>>  - Use 0/1 as return values of set_memory_{max,target}
>>
>> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
>> ---
>>  tools/libxl/xl_cmdimpl.c | 24 ++++++++++++++++--------
>>  1 file changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 7ee6953..31f037f 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -3380,15 +3380,15 @@ static int set_memory_max(uint32_t domid, const char *mem)
>>      memorykb = parse_mem_size_kb(mem);
>>      if (memorykb == -1) {
>>          fprintf(stderr, "invalid memory size: %s\n", mem);
>> -        exit(3);
>> +        exit(EXIT_FAILURE);
> 
> IMHO (I would like to hear others' opinion) but I think you can just 
> return 1 here instead of exiting.

I'm generally not in favor of non-top-level functions calling exit()
unless returning an error is really impossible (as libxl does with
memory allocation failures).

IOW, I tend to agree with Roger.

 -George
diff mbox

Patch

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 7ee6953..31f037f 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3380,15 +3380,15 @@  static int set_memory_max(uint32_t domid, const char *mem)
     memorykb = parse_mem_size_kb(mem);
     if (memorykb == -1) {
         fprintf(stderr, "invalid memory size: %s\n", mem);
-        exit(3);
+        exit(EXIT_FAILURE);
     }
 
     if (libxl_domain_setmaxmem(ctx, domid, memorykb)) {
         fprintf(stderr, "cannot set domid %d static max memory to : %s\n", domid, mem);
-        return EXIT_FAILURE;
+        return 1;
     }
 
-    return EXIT_SUCCESS;
+    return 0;
 }
 
 int main_memmax(int argc, char **argv)
@@ -3404,7 +3404,11 @@  int main_memmax(int argc, char **argv)
     domid = find_domain(argv[optind]);
     mem = argv[optind + 1];
 
-    return set_memory_max(domid, mem);
+    if (set_memory_max(domid, mem)) {
+        return EXIT_FAILURE;
+    }
+
+    return EXIT_SUCCESS;
 }
 
 static int set_memory_target(uint32_t domid, const char *mem)
@@ -3414,15 +3418,15 @@  static int set_memory_target(uint32_t domid, const char *mem)
     memorykb = parse_mem_size_kb(mem);
     if (memorykb == -1)  {
         fprintf(stderr, "invalid memory size: %s\n", mem);
-        exit(3);
+        exit(EXIT_FAILURE);
     }
 
     if (libxl_set_memory_target(ctx, domid, memorykb, 0, /* enforce */ 1)) {
         fprintf(stderr, "cannot set domid %d dynamic max memory to : %s\n", domid, mem);
-        return EXIT_FAILURE;
+        return 1;
     }
 
-    return EXIT_SUCCESS;
+    return 0;
 }
 
 int main_memset(int argc, char **argv)
@@ -3438,7 +3442,11 @@  int main_memset(int argc, char **argv)
     domid = find_domain(argv[optind]);
     mem = argv[optind + 1];
 
-    return set_memory_target(domid, mem);
+    if (set_memory_target(domid, mem)) {
+        return EXIT_FAILURE;
+    }
+
+    return EXIT_SUCCESS;
 }
 
 static int cd_insert(uint32_t domid, const char *virtdev, char *phys)