Re: [Xen-devel] [PATCH] libxl: improve error handling when saving device model state

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2011-12-15 at 17:00 +0000, Ian Jackson wrote:
> Ian Campbell writes ("[Xen-devel] [PATCH] libxl: improve error handling when 
> saving device model state"):
> > libxl: improve error handling when saving device model state.
> ...
> > +out_close_fd2:
> >      close(fd2);
> > +out_unlink:
> >      unlink(filename);
> 
> This style of error handling is very prone to errors.
> 
> How about:
> 
>     int fd2 = -1;
> 
>     blah blah maybe goto out blah blah
> 
>     if (fd2 >= 0) close(fd2);
> 
> And always unlinking the filename is fine, surely ?

Yes. Patch is below.

In principal it ought to be possible to save upstream qemu state direct
to the fd and avoid the file altogether, Anthony what do you think?

Ian.

diff -r 5924514e0523 tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c   Fri Dec 16 09:48:06 2011 +0000
+++ b/tools/libxl/libxl_dom.c   Fri Dec 16 10:36:34 2011 +0000
@@ -605,7 +605,7 @@ out:
 int libxl__domain_save_device_model(libxl__gc *gc, uint32_t domid, int fd)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
-    int fd2, c;
+    int ret, fd2 = -1, c;
     char buf[1024];
     const char *filename = libxl__device_model_savefile(gc, domid);
     struct stat st;
@@ -630,8 +630,11 @@ int libxl__domain_save_device_model(libx
             return ERROR_FAIL;
         }
         /* Save DM state into fd2 */
-        if (libxl__qmp_migrate(gc, domid, fd2))
-            return ERROR_FAIL;
+        ret = libxl__qmp_migrate(gc, domid, fd2);
+        if (ret)
+            goto out;
+        close(fd2);
+        fd2 = -1;
         break;
     default:
         return ERROR_INVAL;
@@ -640,37 +643,45 @@ int libxl__domain_save_device_model(libx
     if (stat(filename, &st) < 0)
     {
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Unable to stat qemu save file\n");
-        return ERROR_FAIL;
+        ret = ERROR_FAIL;
+        goto out;
     }
 
     qemu_state_len = st.st_size;
     LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Qemu state is %d bytes\n", 
qemu_state_len);
 
-    c = libxl_write_exactly(ctx, fd, QEMU_SIGNATURE, strlen(QEMU_SIGNATURE),
-                            "saved-state file", "qemu signature");
-    if (c)
-        return c;
+    ret = libxl_write_exactly(ctx, fd, QEMU_SIGNATURE, strlen(QEMU_SIGNATURE),
+                              "saved-state file", "qemu signature");
+    if (ret)
+        goto out;
 
-    c = libxl_write_exactly(ctx, fd, &qemu_state_len, sizeof(qemu_state_len),
+    ret = libxl_write_exactly(ctx, fd, &qemu_state_len, sizeof(qemu_state_len),
                             "saved-state file", "saved-state length");
-    if (c)
-        return c;
+    if (ret)
+        goto out;
 
     fd2 = open(filename, O_RDONLY);
+    if (fd2 < 0) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Unable to open qemu save 
file\n");
+        goto out;
+    }
     while ((c = read(fd2, buf, sizeof(buf))) != 0) {
         if (c < 0) {
             if (errno == EINTR)
                 continue;
-            return errno;
+            ret = errno;
+            goto out;
         }
-        c = libxl_write_exactly(
+        ret = libxl_write_exactly(
             ctx, fd, buf, c, "saved-state file", "qemu state");
-        if (c)
-            return c;
+        if (ret)
+            goto out;
     }
-    close(fd2);
+    ret = 0;
+out:
+    if (fd2 >= 0) close(fd2);
     unlink(filename);
-    return 0;
+    return ret;
 }
 
 char *libxl__uuid2string(libxl__gc *gc, const libxl_uuid uuid)



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel