proto: Fix both-ends-cancel case.

In the case where both ends cancel at the same time (eg. both ends
realize there are errors before or during the transfer), previously we
skipped sending back an error from the daemon, on the spurious basis
that the library would not need it (the library is cancelling because
of its own error).

However this is wrong: we should always send back an error message
from the daemon in order to preserve synchronization of the protocol.

A simple test case is:

  $ guestfish -N fs -m /dev/sda1 upload nosuchfile /
  libguestfs: error: open: nosuchfile: No such file or directory
  libguestfs: error: unexpected procedure number (66/282)

(Notice two things: there are errors at both ends, and the
loss of synchronization).

After applying this commit, the loss of synchronization does not occur
and we just see the library error:

  $ guestfish -N fs -m /dev/sda1 upload nosuchfile /
  libguestfs: error: open: nosuchfile: No such file or directory

The choice of displaying the library or the daemon error is fairly
arbitrary in this case -- it would be valid to display either or even
to combine them into one error.  Displaying the library error only
makes the code considerably simpler.

This commit also (re-)enables a test for this case.

diff --git a/daemon/base64.c b/daemon/base64.c
index 7e07a6a..f55e1f5 100644 (file)
--- a/daemon/base64.c
+++ b/daemon/base64.c
@@ -77,8 +77,11 @@ do_base64_in (const char *file)
     return -1;
   }
   if (r == -2) {               /* cancellation from library */
+    /* This error is ignored by the library since it initiated the
+     * cancel.  Nevertheless we must send an error reply here.
+     */
+    reply_with_error ("file upload cancelled");
     pclose (fp);
-    /* Do NOT send any error. */
     return -1;
   }
 

diff --git a/daemon/debug.c b/daemon/debug.c
index 091e741..cd3e8a5 100644 (file)
--- a/daemon/debug.c
+++ b/daemon/debug.c
@@ -540,8 +540,11 @@ do_debug_upload (const char *filename, int mode)
     return -1;
   }
   if (r == -2) {               /* cancellation from library */
+    /* This error is ignored by the library since it initiated the
+     * cancel.  Nevertheless we must send an error reply here.
+     */
+    reply_with_error ("file upload cancelled");
     close (fd);
-    /* Do NOT send any error. */
     return -1;
   }
 

diff --git a/daemon/tar.c b/daemon/tar.c
index 3c756af..39fce8c 100644 (file)
--- a/daemon/tar.c
+++ b/daemon/tar.c
@@ -118,8 +118,11 @@ do_tXz_in (const char *dir, const char *filter)
     return -1;
   }
   if (r == -2) {               /* cancellation from library */
+    /* This error is ignored by the library since it initiated the
+     * cancel.  Nevertheless we must send an error reply here.
+     */
+    reply_with_error ("file upload cancelled");
     pclose (fp);
-    /* Do NOT send any error. */
     return -1;
   }
 

diff --git a/daemon/upload.c b/daemon/upload.c
index f8d312f..2c42969 100644 (file)
--- a/daemon/upload.c
+++ b/daemon/upload.c
@@ -94,8 +94,11 @@ upload (const char *filename, int flags, int64_t offset)
     return -1;
   }
   if (r == -2) {               /* cancellation from library */
+    /* This error is ignored by the library since it initiated the
+     * cancel.  Nevertheless we must send an error reply here.
+     */
+    reply_with_error ("file upload cancelled");
     close (data.fd);
-    /* Do NOT send any error. */
     return -1;
   }
 

diff --git a/generator/generator_c.ml b/generator/generator_c.ml
index aee2d77..648ff68 100644 (file)
--- a/generator/generator_c.ml
+++ b/generator/generator_c.ml
@@ -1177,6 +1177,8 @@ trace_send_line (guestfs_h *g)
             pr "  if (r == -1) {\n";
             pr "    guestfs___end_busy (g);\n";
             trace_return_error ~indent:4 shortname style errcode;
+            pr "    /* daemon will send an error reply which we discard */\n";
+            pr "    guestfs___recv_discard (g, \"%s\");\n" shortname;
             pr "    return %s;\n" (string_of_errcode errcode);
             pr "  }\n";
             pr "  if (r == -2) /* daemon cancelled */\n";

diff --git a/generator/generator_daemon.ml b/generator/generator_daemon.ml
index 8c3a548..f5c1fa3 100644 (file)
--- a/generator/generator_daemon.ml
+++ b/generator/generator_daemon.ml
@@ -128,7 +128,7 @@ and generate_daemon_actions () =
         let mask = Int64.lognot (Int64.pred (Int64.shift_left 1L len)) in
         pr "  if (optargs_bitmask & UINT64_C(0x%Lx)) {\n" mask;
         if is_filein then
-          pr "    if (cancel_receive () != -2)\n";
+          pr "    cancel_receive ();\n";
         pr "    reply_with_error (\"unknown option in optional arguments bitmask (this can happen if a program is compiled against a newer version of libguestfs, then run against an older version of the daemon)\");\n";
         pr "    goto done;\n";
         pr "  }\n";
@@ -141,8 +141,8 @@ and generate_daemon_actions () =
         pr "\n";
         pr "  if (!xdr_guestfs_%s_args (xdr_in, &args)) {\n" name;
         if is_filein then
-          pr "    if (cancel_receive () != -2)\n";
-        pr "      reply_with_error (\"daemon failed to decode procedure arguments\");\n";
+          pr "    cancel_receive ();\n";
+        pr "    reply_with_error (\"daemon failed to decode procedure arguments\");\n";
         pr "    goto done;\n";
         pr "  }\n";
         let pr_args n =
@@ -153,8 +153,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 "    if (cancel_receive () != -2)\n";
-          pr "      reply_with_perror (\"realloc\");\n";
+            pr "    cancel_receive ();\n";
+          pr "    reply_with_perror (\"realloc\");\n";
           pr "    goto done;\n";
           pr "  }\n";
           pr "  %s[args.%s.%s_len] = NULL;\n" n n n;

diff --git a/regressions/Makefile.am b/regressions/Makefile.am
index 6527e06..b01d3a5 100644 (file)
--- a/regressions/Makefile.am
+++ b/regressions/Makefile.am
@@ -33,6 +33,7 @@ TESTS = \
        rhbz578407.sh \
        rhbz580246.sh \
        test-add-domain.sh \
+       test-both-ends-cancel.sh \
        test-cancellation-download-librarycancels.sh \
        test-cancellation-upload-daemoncancels.sh \
        test-copy.sh \
@@ -59,7 +60,6 @@ SKIPPED_TESTS = \
        test-bootbootboot.sh
 
 FAILING_TESTS = \
-       test-both-ends-cancel.sh \
        test-qemudie-launchfail.sh
 
 random_val := $(shell awk 'BEGIN{srand(); print 1+int(255*rand())}' < /dev/null)

diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index 7223a88..ccc5bb5 100644 (file)
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -289,6 +289,7 @@ extern int guestfs___set_busy (guestfs_h *g);
 extern int guestfs___end_busy (guestfs_h *g);
 extern int guestfs___send (guestfs_h *g, int proc_nr, uint64_t progress_hint, uint64_t optargs_bitmask, xdrproc_t xdrp, char *args);
 extern int guestfs___recv (guestfs_h *g, const char *fn, struct guestfs_message_header *hdr, struct guestfs_message_error *err, xdrproc_t xdrp, char *ret);
+extern int guestfs___recv_discard (guestfs_h *g, const char *fn);
 extern int guestfs___send_file (guestfs_h *g, const char *filename);
 extern int guestfs___recv_file (guestfs_h *g, const char *filename);
 extern int guestfs___send_to_daemon (guestfs_h *g, const void *v_buf, size_t n);

diff --git a/src/proto.c b/src/proto.c
index 2ca24c2..cda731e 100644 (file)
--- a/src/proto.c
+++ b/src/proto.c
@@ -848,9 +848,6 @@ guestfs___send_file (guestfs_h *g, const char *filename)
   if (fd == -1) {
     perrorf (g, "open: %s", filename);
     send_file_cancellation (g);
-    /* Daemon sees cancellation and won't reply, so caller can
-     * just return here.
-     */
     return -1;
   }
 
@@ -1034,6 +1031,34 @@ guestfs___recv (guestfs_h *g, const char *fn,
   return 0;
 }
 
+/* Same as guestfs___recv, but it discards the reply message. */
+int
+guestfs___recv_discard (guestfs_h *g, const char *fn)
+{
+  void *buf;
+  uint32_t size;
+  int r;
+
+ again:
+  r = guestfs___recv_from_daemon (g, &size, &buf);
+  if (r == -1)
+    return -1;
+
+  /* This can happen if a cancellation happens right at the end
+   * of us sending a FileIn parameter to the daemon.  Discard.  The
+   * daemon should send us an error message next.
+   */
+  if (size == GUESTFS_CANCEL_FLAG)
+    goto again;
+
+  if (size == GUESTFS_LAUNCH_FLAG) {
+    error (g, "%s: received unexpected launch flag from daemon when expecting reply", fn);
+    return -1;
+  }
+
+  return 0;
+}
+
 /* Receive a file. */
 
 /* Returns -1 = error, 0 = EOF, > 0 = more data */