[PATCH 0/9] xl: convert exit codes related to domain subcommands to EXIT_[SUCCESS|FAILURE]

[PATCH 0/9] xl: convert exit codes related to domain subcommands to EXIT_[SUCCESS|FAILURE]

[Harmandeep Kaur]

*main_foo() is treated somewhat as a regular main(), it is changed to
return EXIT_SUCCESS or EXIT_FAILURE.

*Functions that are not main_foo(), are changed to do 'return 0' or
'return 1', and then 0/1 is taken care in the main_foo() functions
that calls them.

*Functions in xl_cmdimpl.c related to domain subcommands are changed.

*This series perceds the following series:
http://lists.xenproject.org/archives/html/xen-devel/2015-10/msg02990.html

Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
---

[PATCH 1/9] xl: Improve return and exit codes of memory related functions.

[Harmandeep Kaur]

Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
---
 tools/libxl/xl_cmdimpl.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 116363d..8f5a2f4 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -172,7 +172,7 @@ static uint32_t find_domain(const char *p)
     rc = libxl_domain_qualifier_to_domid(ctx, p, &domid);
     if (rc) {
         fprintf(stderr, "%s is an invalid domain identifier (rc=%d)\n", p, rc);
-        exit(2);
+        exit(EXIT_FAILURE);
     }
     common_domname = libxl_domid_to_name(ctx, domid);
     return domid;
@@ -3275,7 +3275,7 @@ 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);
     }
 
     rc = libxl_domain_setmaxmem(ctx, domid, memorykb);
@@ -3300,10 +3300,10 @@ int main_memmax(int argc, char **argv)
     rc = set_memory_max(domid, mem);
     if (rc) {
         fprintf(stderr, "cannot set domid %d static max memory to : %s\n", domid, mem);
-        return 1;
+        return EXIT_FAILURE;
     }
 
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 static void set_memory_target(uint32_t domid, const char *mem)
@@ -3313,7 +3313,7 @@ static void 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);
     }
 
     libxl_set_memory_target(ctx, domid, memorykb, 0, /* enforce */ 1);
@@ -3333,7 +3333,7 @@ int main_memset(int argc, char **argv)
     mem = argv[optind + 1];
 
     set_memory_target(domid, mem);
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 static int cd_insert(uint32_t domid, const char *virtdev, char *phys)
-- 
2.5.0

Re: [PATCH 1/9] xl: Improve return and exit codes of memory related functions.

[Olaf Hering]

On Wed, Feb 24, Harmandeep Kaur wrote:

> -        exit(2);
> +        exit(EXIT_FAILURE);

The commit message should state why changing the value is fine.
I dont know if anyone actually cares about the exact value.

Olaf

Re: [PATCH 1/9] xl: Improve return and exit codes of memory related functions.

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1588 bytes --]

On Wed, 2016-02-24 at 14:55 +0100, Olaf Hering wrote:
> On Wed, Feb 24, Harmandeep Kaur wrote:
> 
> > -        exit(2);
> > +        exit(EXIT_FAILURE);
> 
> The commit message should state why changing the value is fine.
> I dont know if anyone actually cares about the exact value.
> 
This has been discussed before here on the mailing list, and there
already are patches committed that do this same thing in a bunch of
cases (and, of course, the aim is consistently doing it everywhere).

Previous discussion is here:
 http://lists.xen.org/archives/html/xen-devel/2015-10/msg02497.html
 http://lists.xen.org/archives/html/xen-devel/2015-10/msg02501.html
 http://lists.xen.org/archives/html/xen-devel/2015-10/msg02509.html

And, even before, here:
 http://lists.xenproject.org/archives/html/xen-devel/2015-03/msg01336.html
 http://lists.xenproject.org/archives/html/xen-devel/2015-03/msg01341.html

I'm quit sure there is no one doing anything useful with exit codes
right now, as they're so inconsistent that it would just be plain
impossible! :-)

Anyway, Harmandeep, can you summarize better (and maybe reference)
these previous discussions and your previous related work in the cover
letter of this series (and any other similar one that may follow)?

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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 1/9] xl: Improve return and exit codes of memory related functions.

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2308 bytes --]

On Wed, 2016-02-24 at 18:23 +0530, Harmandeep Kaur wrote:
> Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
> ---
>  tools/libxl/xl_cmdimpl.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 116363d..8f5a2f4 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -172,7 +172,7 @@ static uint32_t find_domain(const char *p)
>      rc = libxl_domain_qualifier_to_domid(ctx, p, &domid);
>      if (rc) {
>          fprintf(stderr, "%s is an invalid domain identifier
> (rc=%d)\n", p, rc);
> -        exit(2);
> +        exit(EXIT_FAILURE);
>
I'm not sure find_domain() is a "memory related functions", so I
wouldn't change it in this patch.

On the other hand, there is parse_mem_size_kb(), which returns -1 on
failure, or the amount of memory, on success.

That is ok, IMO, so, don't change it. However, I think you can take the
chance of this patch to add just a couple of line of comments, above
its signature, explaining that it does exactly that.

Also, there is freemem(), which also does look "memory related" to me.
And in its case, it indeed uses libxl's error codes while it should --
being an internal function-- just use the 0/1 convention. I think you
can 'fix' it in this patch.

> @@ -3275,7 +3275,7 @@ 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);
>      }
>  
>      rc = libxl_domain_setmaxmem(ctx, domid, memorykb);
>
The statement right below this one is:

 return rc;

set_memory_max() is an internal function so, again, it should retunr
0/1 to its caller, rather than a libxl error code.

The rest of this patch looks ok to me.

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)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

[PATCH 2/9] xl: Improve return and exit codes of restore and save related functions.

[Harmandeep Kaur]

Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
---
 tools/libxl/xl_cmdimpl.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 8f5a2f4..e5bb41f 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2708,11 +2708,11 @@ static uint32_t create_domain(struct domain_create *dom_info)
             restore_fd = open(restore_file, O_RDONLY);
             if (restore_fd == -1) {
                 fprintf(stderr, "Can't open restore file: %s\n", strerror(errno));
-                return ERROR_INVAL;
+                return -1;
             }
             restore_fd_to_close = restore_fd;
             rc = libxl_fd_set_cloexec(ctx, restore_fd, 1);
-            if (rc) return rc;
+            if (rc) return -1;
         }
 
         CHK_ERRNOVAL(libxl_read_exactly(
@@ -2721,11 +2721,11 @@ static uint32_t create_domain(struct domain_create *dom_info)
         if (memcmp(hdr.magic, savefileheader_magic, sizeof(hdr.magic))) {
             fprintf(stderr, "File has wrong magic number -"
                     " corrupt or for a different tool?\n");
-            return ERROR_INVAL;
+            return -1;
         }
         if (hdr.byteorder != SAVEFILE_BYTEORDER_VALUE) {
             fprintf(stderr, "File has wrong byte order\n");
-            return ERROR_INVAL;
+            return -1;
         }
         fprintf(stderr, "Loading new save file %s"
                 " (new xl fmt info"
@@ -2738,7 +2738,7 @@ static uint32_t create_domain(struct domain_create *dom_info)
             fprintf(stderr, "Savefile has mandatory flag(s) 0x%"PRIx32" "
                     "which are not supported; need newer xl\n",
                     badflags);
-            return ERROR_INVAL;
+            return -1;
         }
         if (hdr.optional_data_len) {
             optdata_begin = xmalloc(hdr.optional_data_len);
@@ -2752,7 +2752,7 @@ static uint32_t create_domain(struct domain_create *dom_info)
 #define WITH_OPTDATA(amt, body)                                 \
             if (OPTDATA_LEFT < (amt)) {                         \
                 fprintf(stderr, "Savefile truncated.\n");       \
-                return ERROR_INVAL;                             \
+                return -1;         	                        \
             } else {                                            \
                 body;                                           \
                 optdata_here += (amt);                          \
@@ -2785,12 +2785,12 @@ static uint32_t create_domain(struct domain_create *dom_info)
             ret = libxl_read_file_contents(ctx, config_file,
                                            &config_data, &config_len);
             if (ret) { fprintf(stderr, "Failed to read config file: %s: %s\n",
-                               config_file, strerror(errno)); return ERROR_FAIL; }
+                               config_file, strerror(errno)); return -1; }
         }
         if (!restoring && extra_config && strlen(extra_config)) {
             if (config_len > INT_MAX - (strlen(extra_config) + 2 + 1)) {
                 fprintf(stderr, "Failed to attach extra configuration\n");
-                return ERROR_FAIL;
+                return -1;
             }
             /* allocate space for the extra config plus two EOLs plus \0 */
             config_data = xrealloc(config_data, config_len
@@ -2804,7 +2804,7 @@ static uint32_t create_domain(struct domain_create *dom_info)
         if (!config_data) {
             fprintf(stderr, "Config file not specified and"
                     " none in save file\n");
-            return ERROR_INVAL;
+            return -1;
         }
         config_source = "<saved>";
         config_in_json = !!(hdr.mandatory_flags & XL_MANDATORY_FLAG_JSON);
@@ -2843,7 +2843,7 @@ static uint32_t create_domain(struct domain_create *dom_info)
             if (!json) {
                 fprintf(stderr,
                         "Failed to convert domain configuration to JSON\n");
-                exit(1);
+                exit(EXIT_FAILURE);
             }
             fputs(json, cfg_print_fh);
             free(json);
@@ -2867,7 +2867,7 @@ start:
         ret = freemem(domid, &d_config.b_info);
         if (ret < 0) {
             fprintf(stderr, "failed to free memory for the domain\n");
-            ret = ERROR_FAIL;
+            ret = -1;
             goto error_out;
         }
     }
@@ -3091,9 +3091,9 @@ out:
      * already happened in the parent.
      */
     if ( daemonize && !need_daemon )
-        exit(ret);
+        exit(EXIT_SUCCESS);
 
-    return ret;
+    return ret < 0 ? -1 : 0;
 }
 
 void help(const char *command)
@@ -4122,7 +4122,7 @@ static int save_domain(uint32_t domid, const char *filename, int checkpoint,
     fd = open(filename, O_WRONLY|O_CREAT|O_TRUNC, 0644);
     if (fd < 0) {
         fprintf(stderr, "Failed to open temp file %s for writing\n", filename);
-        exit(2);
+        exit(EXIT_FAILURE);
     }
 
     save_domain_core_writeconfig(fd, filename, config_data, config_len);
@@ -4142,7 +4142,7 @@ static int save_domain(uint32_t domid, const char *filename, int checkpoint,
     else
         libxl_domain_destroy(ctx, domid, 0);
 
-    exit(rc < 0 ? 1 : 0);
+    exit(rc < 0 ? EXIT_FAILURE : EXIT_SUCCESS);
 }
 
 static pid_t create_migration_child(const char *rune, int *send_fd,
@@ -4611,7 +4611,7 @@ int main_restore(int argc, char **argv)
         checkpoint_file = argv[optind + 1];
     } else {
         help("restore");
-        return 2;
+        return EXIT_FAILURE;
     }
 
     memset(&dom_info, 0, sizeof(dom_info));
@@ -4628,9 +4628,9 @@ int main_restore(int argc, char **argv)
 
     rc = create_domain(&dom_info);
     if (rc < 0)
-        return -rc;
+        return EXIT_FAILURE;
 
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_migrate_receive(int argc, char **argv)
@@ -4685,7 +4685,7 @@ int main_save(int argc, char **argv)
 
     if (argc-optind > 3) {
         help("save");
-        return 2;
+        return EXIT_FAILURE;
     }
 
     domid = find_domain(argv[optind]);
@@ -4694,7 +4694,7 @@ int main_save(int argc, char **argv)
         config_filename = argv[optind + 2];
 
     save_domain(domid, filename, checkpoint, leavepaused, config_filename);
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_migrate(int argc, char **argv)
-- 
2.5.0

Re: [PATCH 2/9] xl: Improve return and exit codes of restore and save related functions.

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1981 bytes --]

On Wed, 2016-02-24 at 18:23 +0530, Harmandeep Kaur wrote:
> Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
> ---
>  tools/libxl/xl_cmdimpl.c | 40 ++++++++++++++++++++----------------
> ----
>  1 file changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 8f5a2f4..e5bb41f 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -2708,11 +2708,11 @@ static uint32_t create_domain(struct
> domain_create *dom_info)
>              restore_fd = open(restore_file, O_RDONLY);
>              if (restore_fd == -1) {
>                  fprintf(stderr, "Can't open restore file: %s\n",
> strerror(errno));
> -                return ERROR_INVAL;
> +                return -1;
>              }
Ah, so here it is create_domain(). Mmm... no, I think it would be best
to have it changed in the other patch where I mentioned it, together
with the other domain creation related functions.

That being said, the way in which the function is changed looks ok to
me. Only one comment about this hunk:

> @@ -3091,9 +3091,9 @@ out:
>       * already happened in the parent.
>       */
>      if ( daemonize && !need_daemon )
> -        exit(ret);
> +        exit(EXIT_SUCCESS);
>  
> -    return ret;
> +    return ret < 0 ? -1 : 0;
>
The ret<0 part was thre because libxl error codes where used... now
that we're not using them any longer, can we just initialize ret to 0,
change it to -1 on error (like you're doing) and, here, just return it.

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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

[PATCH 3/9] xl: Improve return and exit codes of migrate related functions.

[Harmandeep Kaur]

Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
---
 tools/libxl/xl_cmdimpl.c | 56 ++++++++++++++++++++++++------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index e5bb41f..2092c80 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -50,7 +50,7 @@
         else if (chk_errnoval > 0) {                                    \
             fprintf(stderr,"xl: fatal error: %s:%d: %s: %s\n",          \
                     __FILE__,__LINE__, strerror(chk_errnoval), #call);  \
-            exit(-ERROR_FAIL);                                          \
+            exit(EXIT_FAILURE);                                         \
         }                                                               \
     })
 
@@ -4036,7 +4036,7 @@ static void save_domain_core_begin(uint32_t domid,
                                       &config_v, config_len_r);
         if (rc) {
             fprintf(stderr, "unable to read overridden config file\n");
-            exit(2);
+            exit(EXIT_FAILURE);
         }
         parse_config_data(override_config_file, config_v, *config_len_r,
                           &d_config);
@@ -4045,14 +4045,14 @@ static void save_domain_core_begin(uint32_t domid,
         rc = libxl_retrieve_domain_configuration(ctx, domid, &d_config);
         if (rc) {
             fprintf(stderr, "unable to retrieve domain configuration\n");
-            exit(2);
+            exit(EXIT_FAILURE);
         }
     }
 
     config_c = libxl_domain_config_to_json(ctx, &d_config);
     if (!config_c) {
         fprintf(stderr, "unable to convert config file to JSON\n");
-        exit(2);
+        exit(EXIT_FAILURE);
     }
     *config_data_r = (uint8_t *)config_c;
     *config_len_r = strlen(config_c) + 1; /* including trailing '\0' */
@@ -4152,7 +4152,7 @@ static pid_t create_migration_child(const char *rune, int *send_fd,
     pid_t child;
 
     if (!rune || !send_fd || !recv_fd)
-        return -1;
+        return EXIT_FAILURE;
 
     MUST( libxl_pipe(ctx, sendpipe) );
     MUST( libxl_pipe(ctx, recvpipe) );
@@ -4166,7 +4166,7 @@ static pid_t create_migration_child(const char *rune, int *send_fd,
         close(recvpipe[0]); close(recvpipe[1]);
         execlp("sh","sh","-c",rune,(char*)0);
         perror("failed to exec sh");
-        exit(-1);
+        exit(EXIT_FAILURE);
     }
 
     close(sendpipe[0]);
@@ -4189,16 +4189,16 @@ static int migrate_read_fixedmessage(int fd, const void *msg, int msgsz,
 
     stream = rune ? "migration receiver stream" : "migration stream";
     rc = libxl_read_exactly(ctx, fd, buf, msgsz, stream, what);
-    if (rc) return ERROR_FAIL;
+    if (rc) return EXIT_FAILURE;
 
     if (memcmp(buf, msg, msgsz)) {
         fprintf(stderr, "%s contained unexpected data instead of %s\n",
                 stream, what);
         if (rune)
             fprintf(stderr, "(command run was: %s )\n", rune);
-        return ERROR_FAIL;
+        return EXIT_FAILURE;
     }
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 static void migration_child_report(int recv_fd) {
@@ -4272,7 +4272,7 @@ static void migrate_do_preamble(int send_fd, int recv_fd, pid_t child,
 
     if (send_fd < 0 || recv_fd < 0) {
         fprintf(stderr, "migrate_do_preamble: invalid file descriptors\n");
-        exit(1);
+        exit(EXIT_FAILURE);
     }
 
     rc = migrate_read_fixedmessage(recv_fd, migrate_receiver_banner,
@@ -4281,7 +4281,7 @@ static void migrate_do_preamble(int send_fd, int recv_fd, pid_t child,
     if (rc) {
         close(send_fd);
         migration_child_report(recv_fd);
-        exit(-rc);
+        exit(EXIT_FAILURE);
     }
 
     save_domain_core_writeconfig(send_fd, "migration stream",
@@ -4306,7 +4306,7 @@ static void migrate_domain(uint32_t domid, const char *rune, int debug,
     if (!config_len) {
         fprintf(stderr, "No config file stored for running domain and "
                 "none supplied - cannot migrate.\n");
-        exit(1);
+        exit(EXIT_FAILURE);
     }
 
     child = create_migration_child(rune, &send_fd, &recv_fd);
@@ -4393,26 +4393,26 @@ static void migrate_domain(uint32_t domid, const char *rune, int debug,
         if (!rc) fprintf(stderr, "migration sender: Resumed OK.\n");
 
         fprintf(stderr, "Migration failed due to problems at target.\n");
-        exit(-ERROR_FAIL);
+        exit(EXIT_FAILURE);
     }
 
     fprintf(stderr, "migration sender: Target reports successful startup.\n");
     libxl_domain_destroy(ctx, domid, 0); /* bang! */
     fprintf(stderr, "Migration successful.\n");
-    exit(0);
+    exit(EXIT_SUCCESS);
 
  failed_suspend:
     close(send_fd);
     migration_child_report(recv_fd);
     fprintf(stderr, "Migration failed, failed to suspend at sender.\n");
-    exit(-ERROR_FAIL);
+    exit(EXIT_FAILURE);
 
  failed_resume:
     close(send_fd);
     migration_child_report(recv_fd);
     fprintf(stderr, "Migration failed, resuming at sender.\n");
     libxl_domain_resume(ctx, domid, 1, 0);
-    exit(-ERROR_FAIL);
+    exit(EXIT_FAILURE);
 
  failed_badly:
     fprintf(stderr,
@@ -4425,7 +4425,7 @@ static void migrate_domain(uint32_t domid, const char *rune, int debug,
 
     close(send_fd);
     migration_child_report(recv_fd);
-    exit(-ERROR_BADFAIL);
+    exit(EXIT_FAILURE);
 }
 
 static void migrate_receive(int debug, int daemonize, int monitor,
@@ -4460,7 +4460,7 @@ static void migrate_receive(int debug, int daemonize, int monitor,
     if (rc < 0) {
         fprintf(stderr, "migration target: Domain creation failed"
                 " (code %d).\n", rc);
-        exit(-rc);
+        exit(EXIT_FAILURE);
     }
 
     domid = rc;
@@ -4497,7 +4497,7 @@ static void migrate_receive(int debug, int daemonize, int monitor,
                     "Failed to unpause domain %s (id: %u):%d\n",
                     common_domname, domid, rc);
 
-        exit(rc ? -ERROR_FAIL: 0);
+        exit(rc ? EXIT_FAILURE : EXIT_SUCCESS);
     }
 
     fprintf(stderr, "migration target: Transfer complete,"
@@ -4507,7 +4507,7 @@ static void migrate_receive(int debug, int daemonize, int monitor,
                              migrate_receiver_ready,
                              sizeof(migrate_receiver_ready),
                              "migration ack stream", "ready message");
-    if (rc) exit(-rc);
+    if (rc) exit(EXIT_FAILURE);
 
     rc = migrate_read_fixedmessage(recv_fd, migrate_permission_to_go,
                                    sizeof(migrate_permission_to_go),
@@ -4532,14 +4532,14 @@ static void migrate_receive(int debug, int daemonize, int monitor,
                               migrate_report, sizeof(migrate_report),
                               "migration ack stream",
                               "success/failure report");
-    if (rc2) exit(-ERROR_BADFAIL);
+    if (rc2) exit(EXIT_FAILURE);
 
     rc_buf = -rc;
     assert(!!rc_buf == !!rc);
     rc2 = libxl_write_exactly(ctx, send_fd, &rc_buf, 1,
                               "migration ack stream",
                               "success/failure code");
-    if (rc2) exit(-ERROR_BADFAIL);
+    if (rc2) exit(EXIT_FAILURE);
 
     if (rc) {
         fprintf(stderr, "migration target: Failure, destroying our copy.\n");
@@ -4548,7 +4548,7 @@ static void migrate_receive(int debug, int daemonize, int monitor,
         if (rc2) {
             fprintf(stderr, "migration target: Failed to destroy our copy"
                     " (code %d).\n", rc2);
-            exit(-ERROR_BADFAIL);
+            exit(EXIT_FAILURE);
         }
 
         fprintf(stderr, "migration target: Cleanup OK, granting sender"
@@ -4559,10 +4559,10 @@ static void migrate_receive(int debug, int daemonize, int monitor,
                                   sizeof(migrate_permission_to_go),
                                   "migration ack stream",
                                   "permission to sender to have domain back");
-        if (rc2) exit(-ERROR_BADFAIL);
+        if (rc2) exit(EXIT_FAILURE);
     }
 
-    exit(0);
+    exit(EXIT_SUCCESS);
 }
 
 int main_restore(int argc, char **argv)
@@ -4656,13 +4656,13 @@ int main_migrate_receive(int argc, char **argv)
 
     if (argc-optind != 0) {
         help("migrate-receive");
-        return 2;
+        return EXIT_FAILURE;
     }
     migrate_receive(debug, daemonize, monitor,
                     STDOUT_FILENO, STDIN_FILENO,
                     remus);
 
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_save(int argc, char **argv)
@@ -4761,7 +4761,7 @@ int main_migrate(int argc, char **argv)
     }
 
     migrate_domain(domid, rune, debug, config_filename);
-    return 0;
+    return EXIT_SUCCESS;
 }
 #endif
 
-- 
2.5.0

Re: [PATCH 3/9] xl: Improve return and exit codes of migrate related functions.

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2536 bytes --]

On Wed, 2016-02-24 at 18:23 +0530, Harmandeep Kaur wrote:
> @@ -50,7 +50,7 @@
>          else if (chk_errnoval > 0)
> {                                    \
>              fprintf(stderr,"xl: fatal error: %s:%d: %s:
> %s\n",          \
>                      __FILE__,__LINE__, strerror(chk_errnoval),
> #call);  \
> -            exit(-
> ERROR_FAIL);                                          \
> +            exit(EXIT_FAILURE);                                     
>     \
>          }                                                           
>     \
>      })
>  
Right below this, there are two more macros, CHK_SYSCALL and MUST,
which also need "fixing"

> @@ -4152,7 +4152,7 @@ static pid_t create_migration_child(const char
> *rune, int *send_fd,
>      pid_t child;
>  
>      if (!rune || !send_fd || !recv_fd)
> -        return -1;
> +        return EXIT_FAILURE;
> 
Err.. no, create_migration_child() is an internal function, so it is ok
for it to return -1/0, isn't it?
>  
>      MUST( libxl_pipe(ctx, sendpipe) );
>      MUST( libxl_pipe(ctx, recvpipe) );
> @@ -4166,7 +4166,7 @@ static pid_t create_migration_child(const char
> *rune, int *send_fd,
>          close(recvpipe[0]); close(recvpipe[1]);
>          execlp("sh","sh","-c",rune,(char*)0);
>          perror("failed to exec sh");
> -        exit(-1);
> +        exit(EXIT_FAILURE);
>      }
Of course, in this specific case, since it's an exit() and not a
'return', it is ok to convert this to EXIT_FAILURE, like you're doing.

> @@ -4189,16 +4189,16 @@ static int migrate_read_fixedmessage(int fd,
> const void *msg, int msgsz,
>  
>      stream = rune ? "migration receiver stream" : "migration
> stream";
>      rc = libxl_read_exactly(ctx, fd, buf, msgsz, stream, what);
> -    if (rc) return ERROR_FAIL;
> +    if (rc) return EXIT_FAILURE;
>  
Internal too, so ok changing it, but to -1/0.

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)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

[PATCH 4/9] xl: Improve return and exit codes of main_console(), main_vncviewer() and main_dump_core().

[Harmandeep Kaur]

Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
---
 tools/libxl/xl_cmdimpl.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 2092c80..e1b4286 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3421,7 +3421,7 @@ int main_console(int argc, char **argv)
             type = LIBXL_CONSOLE_TYPE_SERIAL;
         else {
             fprintf(stderr, "console type supported are: pv, serial\n");
-            return 2;
+            return EXIT_FAILURE;
         }
         break;
     case 'n':
@@ -3435,7 +3435,7 @@ int main_console(int argc, char **argv)
     else
         libxl_console_exec(ctx, domid, num, type);
     fprintf(stderr, "Unable to attach console\n");
-    return 1;
+    return EXIT_FAILURE;
 }
 
 int main_vncviewer(int argc, char **argv)
@@ -3457,8 +3457,8 @@ int main_vncviewer(int argc, char **argv)
     domid = find_domain(argv[optind]);
 
     if (vncviewer(domid, autopass))
-        return 1;
-    return 0;
+        return EXIT_FAILURE;
+    return EXIT_SUCCESS;
 }
 
 static void pcilist(uint32_t domid)
@@ -4013,7 +4013,7 @@ static void core_dump_domain(uint32_t domid, const char *filename)
     int rc;
 
     rc=libxl_domain_core_dump(ctx, domid, filename, NULL);
-    if (rc) { fprintf(stderr,"core dump failed (rc=%d)\n",rc);exit(-1); }
+    if (rc) { fprintf(stderr,"core dump failed (rc=%d)\n",rc);exit(EXIT_FAILURE); }
 }
 
 #ifndef LIBXL_HAVE_NO_SUSPEND_RESUME
@@ -4774,7 +4774,7 @@ int main_dump_core(int argc, char **argv)
     }
 
     core_dump_domain(find_domain(argv[optind]), argv[optind + 1]);
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_pause(int argc, char **argv)
-- 
2.5.0

Re: [PATCH 4/9] xl: Improve return and exit codes of main_console(), main_vncviewer() and main_dump_core().

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1017 bytes --]

On Wed, 2016-02-24 at 18:23 +0530, Harmandeep Kaur wrote:
> Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
>
"console, vnc and core dump related function"

I'm not sure I'd put main_dump_core() in this patch, rather than in
patch 8, but I think it's just fine wither way.

> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> 
> @@ -3457,8 +3457,8 @@ int main_vncviewer(int argc, char **argv)
>      domid = find_domain(argv[optind]);
>  
>      if (vncviewer(domid, autopass))
> -        return 1;
> -    return 0;
> +        return EXIT_FAILURE;
> +    return EXIT_SUCCESS;
>
Have a look at vncviewer() and autoconnect_vncviewer() too.

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)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 4/9] xl: Improve return and exit codes of main_console(), main_vncviewer() and main_dump_core().

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1766 bytes --]

[Re-adding xen-devel... please, don't drop it. :-)]

On Tue, 2016-03-08 at 13:08 +0530, Harmandeep Kaur wrote:
> On Thu, Feb 25, 2016 at 5:03 PM, Dario Faggioli
> <dario.faggioli@citrix.com> wrote:
> > 
> 
> > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > > 
> > > @@ -3457,8 +3457,8 @@ int main_vncviewer(int argc, char **argv)
> > >      domid = find_domain(argv[optind]);
> > > 
> > >      if (vncviewer(domid, autopass))
> > > -        return 1;
> > > -    return 0;
> > > +        return EXIT_FAILURE;
> > > +    return EXIT_SUCCESS;
> > > 
> > Have a look at vncviewer() and autoconnect_vncviewer() too.
> > 
> I am not sure about vncviewer(), do we need something like below:
> 
> static int vncviewer(uint32_t domid, int autopass)
> {
>     if (!libxl_vncviewer_exec(ctx, domid, autopass)) {
>         fprintf(stderr, "Unable to execute vncviewer\n");
>         return 1;
>     }
> }
> 
That won't compile, I think.

It's an internal function, and this patch is changing a bunch of
internal functions into returning -1 on failure ad 0 on success, which
is something vncviewer() is not doing.

It's not too big of a deal, honestly, and you can even let it alone
(we're not going to get 100% consistency anyway), but I thought you may
want to at least consider what to do.

OTOH, autoconnect_vncviewer() does have an _exit() that needs to be
dealt with.

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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

[PATCH 5/9] xl: Improve return and exit codes of main_pause(), main_unpause(), main_destroy() and main_shutdown_or_reboot() related functions.

[Harmandeep Kaur]

Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
---
 tools/libxl/xl_cmdimpl.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index e1b4286..b4920ff 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3696,10 +3696,10 @@ static void destroy_domain(uint32_t domid, int force)
         fprintf(stderr, "Not destroying domain 0; use -f to force.\n"
                         "This can only be done when using a disaggregated "
                         "hardware domain and toolstack.\n\n");
-        exit(-1);
+        exit(EXIT_FAILURE);
     }
     rc = libxl_domain_destroy(ctx, domid, 0);
-    if (rc) { fprintf(stderr,"destroy failed (rc=%d)\n",rc); exit(-1); }
+    if (rc) { fprintf(stderr,"destroy failed (rc=%d)\n",rc); exit(EXIT_FAILURE); }
 }
 
 static void wait_for_domain_deaths(libxl_evgen_domain_death **deathws, int nr)
@@ -3711,7 +3711,7 @@ static void wait_for_domain_deaths(libxl_evgen_domain_death **deathws, int nr)
         rc = libxl_event_wait(ctx, &event, LIBXL_EVENTMASK_ALL, 0,0);
         if (rc) {
             LOG("Failed to get event, quitting (rc=%d)", rc);
-            exit(-1);
+            exit(EXIT_FAILURE);
         }
 
         switch (event->type) {
@@ -3756,14 +3756,14 @@ static void shutdown_domain(uint32_t domid,
     }
 
     if (rc) {
-        fprintf(stderr,"shutdown failed (rc=%d)\n",rc);exit(-1);
+        fprintf(stderr,"shutdown failed (rc=%d)\n",rc);exit(EXIT_FAILURE);
     }
 
     if (deathw) {
         rc = libxl_evenable_domain_death(ctx, domid, for_user, deathw);
         if (rc) {
             fprintf(stderr,"wait for death failed (evgen, rc=%d)\n",rc);
-            exit(-1);
+            exit(EXIT_FAILURE);
         }
     }
 }
@@ -3787,14 +3787,14 @@ static void reboot_domain(uint32_t domid, libxl_evgen_domain_death **deathw,
         }
     }
     if (rc) {
-        fprintf(stderr,"reboot failed (rc=%d)\n",rc);exit(-1);
+        fprintf(stderr,"reboot failed (rc=%d)\n",rc);exit(EXIT_FAILURE);
     }
 
     if (deathw) {
         rc = libxl_evenable_domain_death(ctx, domid, for_user, deathw);
         if (rc) {
             fprintf(stderr,"wait for death failed (evgen, rc=%d)\n",rc);
-            exit(-1);
+            exit(EXIT_FAILURE);
         }
     }
 }
@@ -4787,7 +4787,7 @@ int main_pause(int argc, char **argv)
 
     pause_domain(find_domain(argv[optind]));
 
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_unpause(int argc, char **argv)
@@ -4800,7 +4800,7 @@ int main_unpause(int argc, char **argv)
 
     unpause_domain(find_domain(argv[optind]));
 
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_destroy(int argc, char **argv)
@@ -4815,7 +4815,7 @@ int main_destroy(int argc, char **argv)
     }
 
     destroy_domain(find_domain(argv[optind]), force);
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 static int main_shutdown_or_reboot(int do_reboot, int argc, char **argv)
@@ -4847,7 +4847,7 @@ static int main_shutdown_or_reboot(int do_reboot, int argc, char **argv)
 
     if (!argv[optind] && !all) {
         fprintf(stderr, "You must specify -a or a domain id.\n\n");
-        return opt;
+        return EXIT_FAILURE;
     }
 
     if (all) {
@@ -4855,7 +4855,7 @@ static int main_shutdown_or_reboot(int do_reboot, int argc, char **argv)
         libxl_evgen_domain_death **deathws = NULL;
         if (!(dominfo = libxl_list_domain(ctx, &nb_domain))) {
             fprintf(stderr, "libxl_list_domain failed.\n");
-            return -1;
+            return EXIT_FAILURE;
         }
 
         if (wait_for_it)
@@ -4886,7 +4886,7 @@ static int main_shutdown_or_reboot(int do_reboot, int argc, char **argv)
     }
 
 
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_shutdown(int argc, char **argv)
-- 
2.5.0

Re: [PATCH 5/9] xl: Improve return and exit codes of main_pause(), main_unpause(), main_destroy() and main_shutdown_or_reboot() related functions.

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 760 bytes --]

On Wed, 2016-02-24 at 18:23 +0530, Harmandeep Kaur wrote:
> Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
>
Apart from the subject that, as said already, should be more generic,
and from (at least) one long line, this patch looks fine to me.

I'd provide my Reviewed-by, but I asked to move some hunks from patch
4/9 to here, so that would not apply to next version anyway.

In any case, as said, as far as these modification go, they're fine.

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)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

[PATCH 6/9] xl: Improve return and exit codes of main_list() and main_vm_list() related functions.

[Harmandeep Kaur]

Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
---
 tools/libxl/xl_cmdimpl.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index b4920ff..9d95b50 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -433,7 +433,7 @@ static void flush_stream(FILE *fh)
 
     if (ferror(fh) || fflush(fh)) {
         perror(fh_name);
-        exit(-1);
+        exit(EXIT_FAILURE);
     }
 }
 
@@ -3927,12 +3927,12 @@ static void list_domains(bool verbose, bool context, bool claim, bool numa,
     if (numa) {
         if (libxl_node_bitmap_alloc(ctx, &nodemap, 0)) {
             fprintf(stderr, "libxl_node_bitmap_alloc_failed.\n");
-            exit(1);
+            exit(EXIT_FAILURE);
         }
         if (libxl_get_physinfo(ctx, &physinfo) != 0) {
             fprintf(stderr, "libxl_physinfo failed.\n");
             libxl_bitmap_dispose(&nodemap);
-            exit(1);
+            exit(EXIT_FAILURE);
         }
 
         printf(" NODE Affinity");
@@ -3996,7 +3996,7 @@ static void list_vm(void)
 
     if (!info) {
         fprintf(stderr, "libxl_list_vm failed.\n");
-        exit(1);
+        exit(EXIT_FAILURE);
     }
     printf("UUID                                  ID    name\n");
     for (i = 0; i < nb_vm; i++) {
@@ -4944,7 +4944,7 @@ int main_list(int argc, char **argv)
         info = libxl_list_domain(ctx, &nb_domain);
         if (!info) {
             fprintf(stderr, "libxl_list_domain failed.\n");
-            return 1;
+            return EXIT_FAILURE;
         }
         info_free = info;
     } else if (optind == argc-1) {
@@ -4953,17 +4953,17 @@ int main_list(int argc, char **argv)
         if (rc == ERROR_DOMAIN_NOTFOUND) {
             fprintf(stderr, "Error: Domain \'%s\' does not exist.\n",
                 argv[optind]);
-            return -rc;
+            return EXIT_FAILURE;
         }
         if (rc) {
             fprintf(stderr, "libxl_domain_info failed (code %d).\n", rc);
-            return -rc;
+            return EXIT_FAILURE;
         }
         info = &info_buf;
         nb_domain = 1;
     } else {
         help("list");
-        return 2;
+        return EXIT_FAILURE;
     }
 
     if (details)
@@ -4977,7 +4977,7 @@ int main_list(int argc, char **argv)
 
     libxl_dominfo_dispose(&info_buf);
 
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_vm_list(int argc, char **argv)
@@ -4989,7 +4989,7 @@ int main_vm_list(int argc, char **argv)
     }
 
     list_vm();
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 static void string_realloc_append(char **accumulate, const char *more)
-- 
2.5.0

Re: [PATCH 6/9] xl: Improve return and exit codes of main_list() and main_vm_list() related functions.

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 569 bytes --]

On Wed, 2016-02-24 at 18:23 +0530, Harmandeep Kaur wrote:
> Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
>
This patch again looks fine, but I'll wait for next version to provide
Review-by-s, to double check the new function breakup in the various
patches.

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)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

[PATCH 7/9] xl: Improve return and exit codes of main_create(), main_config_update(), main_sharing(), main_rename() and related functions.

[Harmandeep Kaur]

Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
---
 tools/libxl/xl_cmdimpl.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 9d95b50..f7f7d7f 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5001,7 +5001,7 @@ static void string_realloc_append(char **accumulate, const char *more)
     size_t morelen = strlen(more) + 1/*nul*/;
     if (oldlen > SSIZE_MAX || morelen > SSIZE_MAX - oldlen) {
         fprintf(stderr,"Additional config data far too large\n");
-        exit(-ERROR_FAIL);
+        exit(EXIT_FAILURE);
     }
 
     *accumulate = xrealloc(*accumulate, oldlen + morelen);
@@ -5076,7 +5076,7 @@ int main_create(int argc, char **argv)
         } else {
             help("create");
             free(dom_info.extra_config);
-            return 2;
+            return EXIT_FAILURE;
         }
     }
 
@@ -5095,11 +5095,11 @@ int main_create(int argc, char **argv)
     rc = create_domain(&dom_info);
     if (rc < 0) {
         free(dom_info.extra_config);
-        return -rc;
+        return EXIT_FAILURE;
     }
 
     free(dom_info.extra_config);
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_config_update(int argc, char **argv)
@@ -5120,7 +5120,7 @@ int main_config_update(int argc, char **argv)
     if (argc < 2) {
         fprintf(stderr, "xl config-update requires a domain argument\n");
         help("config-update");
-        exit(1);
+        exit(EXIT_FAILURE);
     }
 
     fprintf(stderr, "WARNING: xl now has better capability to manage domain configuration, "
@@ -5152,7 +5152,7 @@ int main_config_update(int argc, char **argv)
         } else {
             help("create");
             free(extra_config);
-            return 2;
+            return EXIT_FAILURE;
         }
     }
     if (filename) {
@@ -5161,25 +5161,25 @@ int main_config_update(int argc, char **argv)
                                       &config_data, &config_len);
         if (rc) { fprintf(stderr, "Failed to read config file: %s: %s\n",
                            filename, strerror(errno));
-                  free(extra_config); return ERROR_FAIL; }
+                  free(extra_config); return EXIT_FAILURE; }
         if (extra_config && strlen(extra_config)) {
             if (config_len > INT_MAX - (strlen(extra_config) + 2 + 1)) {
                 fprintf(stderr, "Failed to attach extra configuration\n");
-                exit(1);
+                exit(EXIT_FAILURE);
             }
             /* allocate space for the extra config plus two EOLs plus \0 */
             config_data = realloc(config_data, config_len
                 + strlen(extra_config) + 2 + 1);
             if (!config_data) {
                 fprintf(stderr, "Failed to realloc config_data\n");
-                exit(1);
+                exit(EXIT_FAILURE);
             }
             config_len += sprintf(config_data + config_len, "\n%s\n",
                 extra_config);
         }
     } else {
         fprintf(stderr, "Config file not specified\n");
-        exit(1);
+        exit(EXIT_FAILURE);
     }
 
     libxl_domain_config_init(&d_config);
@@ -5195,7 +5195,7 @@ int main_config_update(int argc, char **argv)
                                    config_data, config_len);
         if (rc) {
             fprintf(stderr, "failed to update configuration\n");
-            exit(1);
+            exit(EXIT_FAILURE);
         }
     }
 
@@ -5203,7 +5203,7 @@ int main_config_update(int argc, char **argv)
 
     free(config_data);
     free(extra_config);
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 static void button_press(uint32_t domid, const char *b)
@@ -5772,7 +5772,7 @@ int main_sharing(int argc, char **argv)
         info = libxl_list_domain(ctx, &nb_domain);
         if (!info) {
             fprintf(stderr, "libxl_list_domain failed.\n");
-            return 1;
+            return EXIT_FAILURE;
         }
         info_free = info;
     } else if (optind == argc-1) {
@@ -5781,17 +5781,17 @@ int main_sharing(int argc, char **argv)
         if (rc == ERROR_DOMAIN_NOTFOUND) {
             fprintf(stderr, "Error: Domain \'%s\' does not exist.\n",
                 argv[optind]);
-            return -rc;
+            return EXIT_FAILURE;
         }
         if (rc) {
             fprintf(stderr, "libxl_domain_info failed (code %d).\n", rc);
-            return -rc;
+            return EXIT_FAILURE;
         }
         info = &info_buf;
         nb_domain = 1;
     } else {
         help("sharing");
-        return 2;
+        return EXIT_FAILURE;
     }
 
     sharing(info, nb_domain);
@@ -5801,7 +5801,7 @@ int main_sharing(int argc, char **argv)
     else
         libxl_dominfo_dispose(info);
 
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 static int sched_domain_get(libxl_scheduler sched, int domid,
@@ -6374,10 +6374,10 @@ int main_rename(int argc, char **argv)
     domid = find_domain(dom);
     if (libxl_domain_rename(ctx, domid, common_domname, new_name)) {
         fprintf(stderr, "Can't rename domain '%s'.\n", dom);
-        return 1;
+        return EXIT_FAILURE;
     }
 
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_trigger(int argc, char **argv)
-- 
2.5.0

Re: [PATCH 7/9] xl: Improve return and exit codes of main_create(), main_config_update(), main_sharing(), main_rename() and related functions.

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1006 bytes --]

On Wed, 2016-02-24 at 18:23 +0530, Harmandeep Kaur wrote:
> Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
>
I don't recall if I said this already, but main_sharing() does not
belong here.
 
> @@ -5095,11 +5095,11 @@ int main_create(int argc, char **argv)
>      rc = create_domain(&dom_info);
>      if (rc < 0) {
>          free(dom_info.extra_config);
> -        return -rc;
> +        return EXIT_FAILURE;
>      }
As far as I can see, create_domain() mostly returns libxl error codes.
I think you should convert that one as well (in 0/-1, as it's internal,
and doing it in this patch would be ok).

The rest of the patch looks ok to me.

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)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

[PATCH 8/9] xl: Improve return and exit codes main_button_press(), main_trigger(), main_remus() and related functions.

[Harmandeep Kaur]

Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
---
 tools/libxl/xl_cmdimpl.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index f7f7d7f..9e0a467 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -374,7 +374,7 @@ static void xvasprintf(char **strp, const char *fmt, va_list ap)
     int r = vasprintf(strp, fmt, ap);
     if (r == -1) {
         perror("asprintf failed");
-        exit(-ERROR_FAIL);
+        exit(EXIT_FAILURE);
     }
 }
 
@@ -5216,7 +5216,7 @@ static void button_press(uint32_t domid, const char *b)
         trigger = LIBXL_TRIGGER_SLEEP;
     } else {
         fprintf(stderr, "%s is an invalid button identifier\n", b);
-        exit(2);
+        exit(EXIT_FAILURE);
     }
 
     libxl_send_trigger(ctx, domid, trigger, 0);
@@ -6398,7 +6398,7 @@ int main_trigger(int argc, char **argv)
     trigger_name = argv[optind++];
     if (libxl_trigger_from_string(trigger_name, &trigger)) {
         fprintf(stderr, "Invalid trigger \"%s\"\n", trigger_name);
-        return -1;
+        return EXIT_FAILURE;
     }
 
     if (argv[optind]) {
@@ -6410,7 +6410,7 @@ int main_trigger(int argc, char **argv)
 
     libxl_send_trigger(ctx, domid, trigger, vcpuid);
 
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 
@@ -8064,7 +8064,7 @@ int main_remus(int argc, char **argv)
         send_fd = open("/dev/null", O_RDWR, 0644);
         if (send_fd < 0) {
             perror("failed to open /dev/null");
-            exit(-1);
+            exit(EXIT_FAILURE);
         }
     } else {
 
@@ -8081,7 +8081,7 @@ int main_remus(int argc, char **argv)
         if (!config_len) {
             fprintf(stderr, "No config file stored for running domain and "
                     "none supplied - cannot start remus.\n");
-            exit(1);
+            exit(EXIT_FAILURE);
         }
 
         child = create_migration_child(rune, &send_fd, &recv_fd);
@@ -8102,7 +8102,7 @@ int main_remus(int argc, char **argv)
     if (libxl_domain_info(ctx, 0, domid)) {
         fprintf(stderr, "Remus: Primary domain has been destroyed.\n");
         close(send_fd);
-        return 0;
+        return EXIT_SUCCESS;
     }
 
     /* If we are here, it means remus setup/domain suspend/backup has
@@ -8117,7 +8117,7 @@ int main_remus(int argc, char **argv)
     }
 
     close(send_fd);
-    return -ERROR_FAIL;
+    return EXIT_FAILURE;
 }
 #endif
 
-- 
2.5.0

Re: [PATCH 8/9] xl: Improve return and exit codes main_button_press(), main_trigger(), main_remus() and related functions.

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1428 bytes --]

On Wed, 2016-02-24 at 18:23 +0530, Harmandeep Kaur wrote:
> Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
>
About the subject lines: try to make them more abstract. It does not
scale to list all the changed functions! :-)

So, in this case, something like (just out of the top of my head!)
"debugging, special keys and triggering related functions".

And main_remus() does not really belong here, while main_sysrq(),
main_trigger(), and probably even main_debug_keys() and main_dmesg()
can well do.

> ---
>  tools/libxl/xl_cmdimpl.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index f7f7d7f..9e0a467 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -374,7 +374,7 @@ static void xvasprintf(char **strp, const char
> *fmt, va_list ap)
>      int r = vasprintf(strp, fmt, ap);
>      if (r == -1) {
>          perror("asprintf failed");
> -        exit(-ERROR_FAIL);
> +        exit(EXIT_FAILURE);
>
xvasprintf() doesn't fit here either.

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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

[PATCH 9/9] xl: Improve return codes of main_domid(), main_domname() and main_sysrq() and related functions.

[Harmandeep Kaur]

Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
---
 tools/libxl/xl_cmdimpl.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 9e0a467..234977c 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -6320,12 +6320,12 @@ int main_domid(int argc, char **argv)
 
     if (libxl_name_to_domid(ctx, domname, &domid)) {
         fprintf(stderr, "Can't get domid of domain name '%s', maybe this domain does not exist.\n", domname);
-        return 1;
+        return EXIT_FAILURE;
     }
 
     printf("%d\n", domid);
 
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_domname(int argc, char **argv)
@@ -6343,13 +6343,13 @@ int main_domname(int argc, char **argv)
     if (domid == 0 && !strcmp(endptr, argv[optind])) {
         /*no digits at all*/
         fprintf(stderr, "Invalid domain id.\n\n");
-        return 1;
+        return EXIT_FAILURE;
     }
 
     domname = libxl_domid_to_name(ctx, domid);
     if (!domname) {
         fprintf(stderr, "Can't get domain name of domain id '%d', maybe this domain does not exist.\n", domid);
-        return 1;
+        return EXIT_SUCCESS;
     }
 
     printf("%s\n", domname);
@@ -6431,12 +6431,12 @@ int main_sysrq(int argc, char **argv)
     if (sysrq[1] != '\0') {
         fprintf(stderr, "Invalid sysrq.\n\n");
         help("sysrq");
-        return 1;
+        return EXIT_FAILURE;
     }
 
     libxl_send_sysrq(ctx, domid, sysrq[0]);
 
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 int main_debug_keys(int argc, char **argv)
-- 
2.5.0

Re: [PATCH 9/9] xl: Improve return codes of main_domid(), main_domname() and main_sysrq() and related functions.

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1827 bytes --]

On Wed, 2016-02-24 at 18:23 +0530, Harmandeep Kaur wrote:
> Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
>
So, this basically is "domain id and name related functions", or
"domain utility functions".

I'd rename the patch with something like that.

Also, I think find_domain() is fits nicely in here.

> ---
>  tools/libxl/xl_cmdimpl.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 9e0a467..234977c 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c

>  int main_domname(int argc, char **argv)
> @@ -6343,13 +6343,13 @@ int main_domname(int argc, char **argv)
>      if (domid == 0 && !strcmp(endptr, argv[optind])) {
>          /*no digits at all*/
>          fprintf(stderr, "Invalid domain id.\n\n");
> -        return 1;
> +        return EXIT_FAILURE;
>      }
>  
>      domname = libxl_domid_to_name(ctx, domid);
>      if (!domname) {
>          fprintf(stderr, "Can't get domain name of domain id '%d',
> maybe this domain does not exist.\n", domid);
> -        return 1;
> +        return EXIT_SUCCESS;
>      }
>  
>      printf("%s\n", domname);
>
main_rename() can well be done in this patch as well.

> @@ -6431,12 +6431,12 @@ int main_sysrq(int argc, char **argv)
>
While this one, I'd do it in another patch. (I'd probably move it to
patch 8)

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)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 0/9] xl: convert exit codes related to domain subcommands to EXIT_[SUCCESS|FAILURE]

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1268 bytes --]

On Wed, 2016-02-24 at 18:23 +0530, Harmandeep Kaur wrote:
> *main_foo() is treated somewhat as a regular main(), it is changed to
> return EXIT_SUCCESS or EXIT_FAILURE.
> 
> *Functions that are not main_foo(), are changed to do 'return 0' or
> 'return 1', and then 0/1 is taken care in the main_foo() functions
> that calls them.
> 
> *Functions in xl_cmdimpl.c related to domain subcommands are changed.
> 
> *This series perceds the following series:
> http://lists.xenproject.org/archives/html/xen-devel/2015-
> 10/msg02990.html
> 
You mean "precedes"? And even if yes, isn't it that this series you're
sending now is a follow-up of the work at the link?

Anyway, as I said when replying to Olaf, please, try to summarize a bit
better the previous discussions we've had, and the work you have
already done on this.

> Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
>
Not that it matters much, but the cover letter needs not to be Signed-
off-by.

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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 0/9] xl: convert exit codes related to domain subcommands to EXIT_[SUCCESS|FAILURE]

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 874 bytes --]

On Wed, 2016-02-24 at 18:23 +0530, Harmandeep Kaur wrote:
> *main_foo() is treated somewhat as a regular main(), it is changed to
> return EXIT_SUCCESS or EXIT_FAILURE.
> 
Ok, I think I've looked at all the patches now. Good work. :-)

There were a few issues and mistakes, but mostly I think the various
functions could have been grouped better in the various patches.

I tried to point that out when I thought it was the case, and to
provide suggestions... let me know if there is something you did not
understand or need clarifications.

Looking forward to v2. :-)

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)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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