Libguestfs - May 2010 - [PATCH] Fix FileIn cmds losing synch if both ends send cancel messages (RHBZ#576879).

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
New in Fedora 11: Fedora Windows cross-compiler. Compile Windows
programs, test, and build Windows installers. Over 70 libraries supprt'd
http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw
-------------- next part --------------
>From 832a57670ea5e25361b16429e10445068e23ab53 Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones at redhat.com>
Date: Wed, 12 May 2010 19:55:06 +0100
Subject: [PATCH] Fix FileIn cmds losing synch if both ends send cancel messages
(RHBZ#576879).

During a FileIn command (eg. upload, tar-in) if both sides
experience errors, then both sides could send cancel messages,
the result being lost synchronization.

The reason for the lost synch was because the daemon was ignoring
this case and sending an error message back which the library side
(which had cancelled) was not expecting.

Fix this by checking in the daemon for the case where the library
also cancels during daemon cancellation, and not sending an error
messages.

This also includes an enhanced regression test which checks for this
case.

This extends the original fix in
commit 5922d7084d6b43f0a1a15b664c7082dfeaf584d0.

More details can be found here:
https://bugzilla.redhat.com/show_bug.cgi?id=576879#c5
---
 daemon/command.c          |    2 +-
 daemon/daemon.h           |   26 +++++++++++++++-----------
 daemon/df.c               |    4 ++--
 daemon/inotify.c          |    2 +-
 daemon/mount.c            |    8 ++++----
 daemon/proto.c            |    6 +++---
 daemon/tar.c              |   34 ++++++++++++++++++----------------
 daemon/upload.c           |   13 +++++++------
 regressions/rhbz576879.sh |   12 +++++++++++-
 src/generator.ml          |   18 +++++++++---------
 10 files changed, 71 insertions(+), 54 deletions(-)

diff --git a/daemon/command.c b/daemon/command.c
index 5e87067..48bed2d 100644
--- a/daemon/command.c
+++ b/daemon/command.c
@@ -36,7 +36,7 @@ do_command (char *const *argv)
   int dev_ok, dev_pts_ok, proc_ok, selinux_ok, sys_ok;
 
   /* We need a root filesystem mounted to do this. */
-  NEED_ROOT (, return NULL);
+  NEED_ROOT (0, return NULL);
 
   /* Conveniently, argv is already a NULL-terminated argv-style array
    * of parameters, so we can pass it straight in to our internal
diff --git a/daemon/daemon.h b/daemon/daemon.h
index 977dfdc..de598cd 100644
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -139,10 +139,13 @@ typedef int (*receive_cb) (void *opaque, const void *buf,
size_t len);
 extern int receive_file (receive_cb cb, void *opaque);
 
 /* daemon functions that receive files (FileIn) can call this
- * to cancel incoming transfers (eg. if there is a local error),
- * but they MUST then call reply_with_*.
+ * to cancel incoming transfers (eg. if there is a local error).
+ *
+ * If and only if this function does NOT return -2, they MUST then
+ * call reply_with_*
+ * (see https://bugzilla.redhat.com/show_bug.cgi?id=576879#c5).
  */
-extern void cancel_receive (void);
+extern int cancel_receive (void);
 
 /* daemon functions that return files (FileOut) should call
  * reply, then send_file_* for each FileOut parameter.
@@ -160,8 +163,8 @@ extern void reply (xdrproc_t xdrp, char *ret);
 #define NEED_ROOT(cancel_stmt,fail_stmt)                                \
   do {									\
     if (!root_mounted) {						\
-      cancel_stmt;                                                      \
-      reply_with_error ("%s: you must call 'mount' first to mount
the root filesystem", __func__); \
+      if ((cancel_stmt) != -2)                                          \
+        reply_with_error ("%s: you must call 'mount' first to
mount the root filesystem", __func__); \
       fail_stmt;							\
     }									\
   }									\
@@ -173,8 +176,8 @@ extern void reply (xdrproc_t xdrp, char *ret);
 #define ABS_PATH(path,cancel_stmt,fail_stmt)                            \
   do {									\
     if ((path)[0] != '/') {						\
-      cancel_stmt;							\
-      reply_with_error ("%s: path must start with a / character",
__func__); \
+      if ((cancel_stmt) != -2)                                          \
+        reply_with_error ("%s: path must start with a / character",
__func__); \
       fail_stmt;							\
     }									\
   } while (0)
@@ -189,15 +192,16 @@ extern void reply (xdrproc_t xdrp, char *ret);
 #define RESOLVE_DEVICE(path,cancel_stmt,fail_stmt)                      \
   do {									\
     if (STRNEQLEN ((path), "/dev/", 5)) {				\
-      cancel_stmt;							\
-      reply_with_error ("%s: %s: expecting a device name", __func__,
(path)); \
+      if ((cancel_stmt) != -2)                                          \
+        reply_with_error ("%s: %s: expecting a device name",
__func__, (path)); \
       fail_stmt;							\
     }									\
     if (device_name_translation ((path)) == -1) {                       \
       int err = errno;                                                  \
-      cancel_stmt;                                                      \
+      int r = cancel_stmt;                                              \
       errno = err;                                                      \
-      reply_with_perror ("%s: %s", __func__, path);                  
\
+      if (r != -2)                                                      \
+        reply_with_perror ("%s: %s", __func__, path);                
\
       fail_stmt;							\
     }                                                                   \
   } while (0)
diff --git a/daemon/df.c b/daemon/df.c
index 7aa6f4f..cce41a0 100644
--- a/daemon/df.c
+++ b/daemon/df.c
@@ -33,7 +33,7 @@ do_df (void)
   int r;
   char *out, *err;
 
-  NEED_ROOT (, return NULL);
+  NEED_ROOT (0, return NULL);
 
   r = command (&out, &err, "df", NULL);
   if (r == -1) {
@@ -54,7 +54,7 @@ do_df_h (void)
   int r;
   char *out, *err;
 
-  NEED_ROOT (, return NULL);
+  NEED_ROOT (0, return NULL);
 
   r = command (&out, &err, "df", "-h", NULL);
   if (r == -1) {
diff --git a/daemon/inotify.c b/daemon/inotify.c
index 104fa60..d5a5a73 100644
--- a/daemon/inotify.c
+++ b/daemon/inotify.c
@@ -70,7 +70,7 @@ do_inotify_init (int max_events)
 #ifdef HAVE_SYS_INOTIFY_H
   FILE *fp;
 
-  NEED_ROOT (, return -1);
+  NEED_ROOT (0, return -1);
 
   if (max_events < 0) {
     reply_with_error ("max_events < 0");
diff --git a/daemon/mount.c b/daemon/mount.c
index 8927c6c..5485116 100644
--- a/daemon/mount.c
+++ b/daemon/mount.c
@@ -48,7 +48,7 @@ do_mount_vfs (const char *options, const char *vfstype,
   char *mp;
   char *error;
 
-  ABS_PATH (mountpoint, , return -1);
+  ABS_PATH (mountpoint, 0, return -1);
 
   is_root = STREQ (mountpoint, "/");
 
@@ -121,7 +121,7 @@ do_umount (const char *pathordevice)
   }
 
   if (is_dev)
-    RESOLVE_DEVICE (buf, , { free (buf); return -1; });
+    RESOLVE_DEVICE (buf, 0, { free (buf); return -1; });
 
   r = command (NULL, &err, "umount", buf, NULL);
   free (buf);
@@ -356,7 +356,7 @@ do_mkmountpoint (const char *path)
   int r;
 
   /* NEED_ROOT (return -1); - we don't want this test for this call. */
-  ABS_PATH (path, , return -1);
+  ABS_PATH (path, 0, return -1);
 
   CHROOT_IN;
   r = mkdir (path, 0777);
@@ -381,7 +381,7 @@ do_rmmountpoint (const char *path)
   int r;
 
   /* NEED_ROOT (return -1); - we don't want this test for this call. */
-  ABS_PATH (path, , return -1);
+  ABS_PATH (path, 0, return -1);
 
   CHROOT_IN;
   r = rmdir (path);
diff --git a/daemon/proto.c b/daemon/proto.c
index ee1c400..6fa243f 100644
--- a/daemon/proto.c
+++ b/daemon/proto.c
@@ -395,7 +395,7 @@ receive_file (receive_cb cb, void *opaque)
 }
 
 /* Send a cancellation flag back to the library. */
-void
+int
 cancel_receive (void)
 {
   XDR xdr;
@@ -408,11 +408,11 @@ cancel_receive (void)
 
   if (xwrite (sock, fbuf, sizeof fbuf) == -1) {
     perror ("write to socket");
-    return;
+    return -1;
   }
 
   /* Keep receiving chunks and discarding, until library sees cancel. */
-  (void) receive_file (NULL, NULL);
+  return receive_file (NULL, NULL);
 }
 
 static int check_for_library_cancellation (void);
diff --git a/daemon/tar.c b/daemon/tar.c
index 26a0d30..3f5f60a 100644
--- a/daemon/tar.c
+++ b/daemon/tar.c
@@ -45,9 +45,9 @@ do_tar_in (const char *dir)
   /* "tar -C /sysroot%s -xf -" but we have to quote the dir. */
   if (asprintf_nowarn (&cmd, "tar -C %R -xf -", dir) == -1) {
     err = errno;
-    cancel_receive ();
+    r = cancel_receive ();
     errno = err;
-    reply_with_perror ("asprintf");
+    if (r != -2) reply_with_perror ("asprintf");
     return -1;
   }
 
@@ -57,9 +57,9 @@ do_tar_in (const char *dir)
   fp = popen (cmd, "w");
   if (fp == NULL) {
     err = errno;
-    cancel_receive ();
+    r = cancel_receive ();
     errno = err;
-    reply_with_perror ("%s", cmd);
+    if (r != -2) reply_with_perror ("%s", cmd);
     free (cmd);
     return -1;
   }
@@ -72,8 +72,8 @@ do_tar_in (const char *dir)
 
   r = receive_file (write_cb, &fd);
   if (r == -1) {		/* write error */
-    cancel_receive ();
-    reply_with_error ("write error on directory: %s", dir);
+    if (cancel_receive () != -2)
+      reply_with_error ("write error on directory: %s", dir);
     pclose (fp);
     return -1;
   }
@@ -85,8 +85,9 @@ do_tar_in (const char *dir)
 
   if (pclose (fp) != 0) {
     if (r == -1)                /* if r == 0, file transfer ended already */
-      cancel_receive ();
-    reply_with_error ("tar subcommand failed on directory: %s", dir);
+      r = cancel_receive ();
+    if (r != -2)
+      reply_with_error ("tar subcommand failed on directory: %s",
dir);
     return -1;
   }
 
@@ -162,9 +163,9 @@ do_tXz_in (const char *dir, char filter)
   /* "tar -C /sysroot%s -zxf -" but we have to quote the dir. */
   if (asprintf_nowarn (&cmd, "tar -C %R -%cxf -", dir, filter) ==
-1) {
     err = errno;
-    cancel_receive ();
+    r = cancel_receive ();
     errno = err;
-    reply_with_perror ("asprintf");
+    if (r != -2) reply_with_perror ("asprintf");
     return -1;
   }
 
@@ -174,9 +175,9 @@ do_tXz_in (const char *dir, char filter)
   fp = popen (cmd, "w");
   if (fp == NULL) {
     err = errno;
-    cancel_receive ();
+    r = cancel_receive ();
     errno = err;
-    reply_with_perror ("%s", cmd);
+    if (r != -2) reply_with_perror ("%s", cmd);
     free (cmd);
     return -1;
   }
@@ -186,8 +187,8 @@ do_tXz_in (const char *dir, char filter)
 
   r = receive_file (write_cb, &fd);
   if (r == -1) {		/* write error */
-    cancel_receive ();
-    reply_with_error ("write error on directory: %s", dir);
+    r = cancel_receive ();
+    if (r != -2) reply_with_error ("write error on directory: %s",
dir);
     pclose (fp);
     return -1;
   }
@@ -199,8 +200,9 @@ do_tXz_in (const char *dir, char filter)
 
   if (pclose (fp) != 0) {
     if (r == -1)                /* if r == 0, file transfer ended already */
-      cancel_receive ();
-    reply_with_error ("tar subcommand failed on directory: %s", dir);
+      r = cancel_receive ();
+    if (r != -2)
+      reply_with_error ("tar subcommand failed on directory: %s",
dir);
     return -1;
   }
 
diff --git a/daemon/upload.c b/daemon/upload.c
index d93a5ad..9a6c873 100644
--- a/daemon/upload.c
+++ b/daemon/upload.c
@@ -47,18 +47,18 @@ do_upload (const char *filename)
   if (!is_dev) CHROOT_OUT;
   if (fd == -1) {
     err = errno;
-    cancel_receive ();
+    r = cancel_receive ();
     errno = err;
-    reply_with_perror ("%s", filename);
+    if (r != -2) reply_with_perror ("%s", filename);
     return -1;
   }
 
   r = receive_file (write_cb, &fd);
   if (r == -1) {		/* write error */
     err = errno;
-    cancel_receive ();
+    r = cancel_receive ();
     errno = err;
-    reply_with_error ("write error: %s", filename);
+    if (r != -2) reply_with_error ("write error: %s", filename);
     close (fd);
     return -1;
   }
@@ -71,9 +71,10 @@ do_upload (const char *filename)
   if (close (fd) == -1) {
     err = errno;
     if (r == -1)                /* if r == 0, file transfer ended already */
-      cancel_receive ();
+      r = cancel_receive ();
     errno = err;
-    reply_with_perror ("close: %s", filename);
+    if (r != -2)
+      reply_with_perror ("close: %s", filename);
     return -1;
   }
 
diff --git a/regressions/rhbz576879.sh b/regressions/rhbz576879.sh
index 7453ac7..d5711fd 100755
--- a/regressions/rhbz576879.sh
+++ b/regressions/rhbz576879.sh
@@ -28,4 +28,14 @@ rm -f test1.img
 -upload $srcdir/rhbz576879.sh /test.sh
 # Shouldn't lose synchronization, so next command should work:
 ping-daemon
-EOF
\ No newline at end of file
+EOF
+
+# Second patch tests the problem found in comment 5 where both ends
+# send cancel messages simultaneously.
+
+../fish/guestfish -N disk <<EOF
+-tar-in /tmp/nosuchfile /blah
+ping-daemon
+EOF
+
+rm -f test1.img
\ No newline at end of file
diff --git a/src/generator.ml b/src/generator.ml
index a995b4c..73ab8fa 100755
--- a/src/generator.ml
+++ b/src/generator.ml
@@ -6198,8 +6198,8 @@ and generate_daemon_actions ()             pr
"\n";
            pr "  if (!xdr_guestfs_%s_args (xdr_in, &args)) {\n"
name;
            if is_filein then
-             pr "    cancel_receive ();\n";
-           pr "    reply_with_error (\"daemon failed to decode
procedure arguments\");\n";
+             pr "    if (cancel_receive () != -2)\n";
+           pr "      reply_with_error (\"daemon failed to decode
procedure arguments\");\n";
            pr "    goto done;\n";
            pr "  }\n";
            let pr_args n @@ -6210,8 +6210,8 @@ and generate_daemon_actions ()  
pr "                sizeof (char *) * (args.%s.%s_len+1));\n" n n;
              pr "  if (%s == NULL) {\n" n;
              if is_filein then
-               pr "    cancel_receive ();\n";
-             pr "    reply_with_perror (\"realloc\");\n";
+               pr "    if (cancel_receive () != -2)\n";
+             pr "      reply_with_perror (\"realloc\");\n";
              pr "    goto done;\n";
              pr "  }\n";
              pr "  %s[args.%s.%s_len] = NULL;\n" n n n;
@@ -6222,15 +6222,15 @@ and generate_daemon_actions ()               | Pathname
n ->
                  pr_args n;
                  pr "  ABS_PATH (%s, %s, goto done);\n"
-                   n (if is_filein then "cancel_receive ()" else
"");
+                   n (if is_filein then "cancel_receive ()" else
"0");
              | Device n ->
                  pr_args n;
                  pr "  RESOLVE_DEVICE (%s, %s, goto done);\n"
-                   n (if is_filein then "cancel_receive ()" else
"");
+                   n (if is_filein then "cancel_receive ()" else
"0");
              | Dev_or_Path n ->
                  pr_args n;
                  pr "  REQUIRE_ROOT_OR_RESOLVE_DEVICE (%s, %s, goto
done);\n"
-                   n (if is_filein then "cancel_receive ()" else
"");
+                   n (if is_filein then "cancel_receive ()" else
"0");
              | String n -> pr_args n
              | OptString n -> pr "  %s = args.%s ? *args.%s :
NULL;\n" n n n
              | StringList n ->
@@ -6241,7 +6241,7 @@ and generate_daemon_actions ()                   pr "
* and perform device name translation. */\n";
                  pr "  { int pvi; for (pvi = 0; physvols[pvi] != NULL;
++pvi)\n";
                  pr "    RESOLVE_DEVICE (physvols[pvi], %s, goto
done);\n"
-                   (if is_filein then "cancel_receive ()" else
"");
+                   (if is_filein then "cancel_receive ()" else
"0");
                  pr "  }\n";
              | Bool n -> pr "  %s = args.%s;\n" n n
              | Int n -> pr "  %s = args.%s;\n" n n
@@ -6257,7 +6257,7 @@ and generate_daemon_actions ()          (* Emit NEED_ROOT
just once, even when there are two or
            more Pathname args *)
         pr "  NEED_ROOT (%s, goto done);\n"
-          (if is_filein then "cancel_receive ()" else "");
+          (if is_filein then "cancel_receive ()" else "0");
       );
 
       (* Don't want to call the impl with any FileIn or FileOut
-- 
1.6.6.1