diff mbox

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

Message ID 1456318407-3635-3-git-send-email-write.harmandeep@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

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

Comments

Dario Faggioli March 2, 2016, 6:02 p.m. UTC | #1
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
diff mbox

Patch

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)