Fix upload losing synchronization if root not mounted (RHBZ#576879).

Modify the generator so that it can correctly handle early
cancellation for Pathname|Device|.. parameters.  This fixes
the upload command, but consequently we need to fix the
parameters for tar_in and t?z_in commands.  This should also
mean that 'win:' can now be used as the second argument of
tar_in and t?z_in commands in guestfish, whereas previously
this wouldn't have worked.

Adds a regression test for the original problem.

diff --git a/daemon/command.c b/daemon/command.c
index ca06478..5e87067 100644 (file)
--- 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 (, 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 ebbeaa2..81a9f36 100644 (file)
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -68,7 +68,7 @@ extern char **split_lines (char *str);
 
 extern void trim (char *str);
 
-extern int device_name_translation (char *device, const char *func);
+extern int device_name_translation (char *device);
 
 extern int prog_exists (const char *prog);
 
@@ -155,9 +155,10 @@ extern void reply (xdrproc_t xdrp, char *ret);
 /* Helper for functions that need a root filesystem mounted.
  * NB. Cannot be used for FileIn functions.
  */
-#define NEED_ROOT(fail_stmt)                                           \
+#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__); \
       fail_stmt;                                                       \
     }                                                                  \
@@ -167,9 +168,10 @@ extern void reply (xdrproc_t xdrp, char *ret);
 /* Helper for functions that need an argument ("path") that is absolute.
  * NB. Cannot be used for FileIn functions.
  */
-#define ABS_PATH(path,fail_stmt)                                       \
+#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__); \
       fail_stmt;                                                       \
     }                                                                  \
@@ -182,14 +184,20 @@ extern void reply (xdrproc_t xdrp, char *ret);
  *
  * NB. Cannot be used for FileIn functions.
  */
-#define RESOLVE_DEVICE(path,fail_stmt)                                 \
+#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)); \
       fail_stmt;                                                       \
     }                                                                  \
-    if (device_name_translation ((path), __func__) == -1)              \
+    if (device_name_translation ((path)) == -1) {                       \
+      int err = errno;                                                  \
+      cancel_stmt;                                                      \
+      errno = err;                                                      \
+      reply_with_perror ("%s: %s", __func__, path);                     \
       fail_stmt;                                                       \
+    }                                                                   \
   } while (0)
 
 /* Helper for functions which need either an absolute path in the
@@ -202,13 +210,13 @@ extern void reply (xdrproc_t xdrp, char *ret);
  * because we intend in future to make device parameters a distinct
  * type from filenames.
  */
-#define REQUIRE_ROOT_OR_RESOLVE_DEVICE(path,fail_stmt)                 \
+#define REQUIRE_ROOT_OR_RESOLVE_DEVICE(path,cancel_stmt,fail_stmt)      \
   do {                                                                 \
     if (STREQLEN ((path), "/dev/", 5))                                  \
-      RESOLVE_DEVICE ((path), fail_stmt);                              \
+      RESOLVE_DEVICE ((path), cancel_stmt, fail_stmt);                  \
     else {                                                             \
-      NEED_ROOT (fail_stmt);                                           \
-      ABS_PATH ((path),fail_stmt);                                     \
+      NEED_ROOT (cancel_stmt, fail_stmt);                               \
+      ABS_PATH ((path), cancel_stmt, fail_stmt);                        \
     }                                                                  \
   } while (0)
 

diff --git a/daemon/df.c b/daemon/df.c
index b5c4980..7aa6f4f 100644 (file)
--- 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 (, 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 (, return NULL);
 
   r = command (&out, &err, "df", "-h", NULL);
   if (r == -1) {

diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
index 69baf9e..06ad702 100644 (file)
--- a/daemon/guestfsd.c
+++ b/daemon/guestfsd.c
@@ -1022,42 +1022,36 @@ print_arginfo (const struct printf_info *info, size_t n, int *argtypes)
  * the device nodes themselves will exist in the appliance.
  */
 int
-device_name_translation (char *device, const char *func)
+device_name_translation (char *device)
 {
   int fd;
 
   fd = open (device, O_RDONLY);
   if (fd >= 0) {
+  close_ok:
     close (fd);
     return 0;
   }
 
-  if (errno != ENXIO && errno != ENOENT) {
-  error:
-    reply_with_perror ("%s: %s", func, device);
+  if (errno != ENXIO && errno != ENOENT)
     return -1;
-  }
 
   /* If the name begins with "/dev/sd" then try the alternatives. */
   if (STRNEQLEN (device, "/dev/sd", 7))
-    goto error;
+    return -1;
 
   device[5] = 'h';             /* /dev/hd (old IDE driver) */
   fd = open (device, O_RDONLY);
-  if (fd >= 0) {
-    close (fd);
-    return 0;
-  }
+  if (fd >= 0)
+    goto close_ok;
 
   device[5] = 'v';             /* /dev/vd (for virtio devices) */
   fd = open (device, O_RDONLY);
-  if (fd >= 0) {
-    close (fd);
-    return 0;
-  }
+  if (fd >= 0)
+    goto close_ok;
 
   device[5] = 's';             /* Restore original device name. */
-  goto error;
+  return -1;
 }
 
 /* Check program exists and is executable on $PATH.  Actually, we

diff --git a/daemon/inotify.c b/daemon/inotify.c
index ec3aa64..104fa60 100644 (file)
--- 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 (, return -1);
 
   if (max_events < 0) {
     reply_with_error ("max_events < 0");

diff --git a/daemon/mount.c b/daemon/mount.c
index 838bef8..8927c6c 100644 (file)
--- 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, , 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, , { 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, , 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, , return -1);
 
   CHROOT_IN;
   r = rmdir (path);

diff --git a/daemon/tar.c b/daemon/tar.c
index 5b73599..26a0d30 100644 (file)
--- a/daemon/tar.c
+++ b/daemon/tar.c
@@ -42,12 +42,6 @@ do_tar_in (const char *dir)
   FILE *fp;
   char *cmd;
 
-  if (!root_mounted || dir[0] != '/') {
-    cancel_receive ();
-    reply_with_error ("root must be mounted and path must be absolute");
-    return -1;
-  }
-
   /* "tar -C /sysroot%s -xf -" but we have to quote the dir. */
   if (asprintf_nowarn (&cmd, "tar -C %R -xf -", dir) == -1) {
     err = errno;
@@ -165,12 +159,6 @@ do_tXz_in (const char *dir, char filter)
   FILE *fp;
   char *cmd;
 
-  if (!root_mounted || dir[0] != '/') {
-    cancel_receive ();
-    reply_with_error ("root must be mounted and path must be absolute");
-    return -1;
-  }
-
   /* "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;

diff --git a/daemon/upload.c b/daemon/upload.c
index 3c20d6f..d93a5ad 100644 (file)
--- a/daemon/upload.c
+++ b/daemon/upload.c
@@ -41,13 +41,6 @@ do_upload (const char *filename)
   int err, fd, r, is_dev;
 
   is_dev = STRPREFIX (filename, "/dev/");
-  if (!is_dev) {
-    if (!root_mounted || filename[0] != '/') {
-      cancel_receive ();
-      reply_with_error ("root must be mounted and path must be absolute");
-      return -1;
-    }
-  }
 
   if (!is_dev) CHROOT_IN;
   fd = open (filename, O_WRONLY|O_CREAT|O_TRUNC|O_NOCTTY, 0666);

diff --git a/regressions/Makefile.am b/regressions/Makefile.am
index 8513f9a..7ec9dc8 100644 (file)
--- a/regressions/Makefile.am
+++ b/regressions/Makefile.am
@@ -27,6 +27,7 @@ TESTS = \
        rhbz503169c10.sh \
        rhbz503169c13.sh \
        rhbz557655.sh \
+       rhbz576879.sh \
        rhbz578407.sh \
        rhbz580246.sh \
        test-cancellation-download-librarycancels.sh \

diff --git a/regressions/rhbz576879.sh b/regressions/rhbz576879.sh
new file mode 100755 (executable)
index 0000000..639dd7c
--- /dev/null
+++ b/regressions/rhbz576879.sh
@@ -0,0 +1,33 @@
+#!/bin/bash -
+# libguestfs
+# Copyright (C) 2010 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+
+# Regression test for:
+# https://bugzilla.redhat.com/show_bug.cgi?id=576879
+# upload loses synchronization if the disk is not mounted
+
+set -e
+
+rm -f test1.img
+
+../fish/guestfish <<EOF
+alloc test1.img 10M
+run
+-upload $srcdir/rhbz576879.sh /test.sh
+# Shouldn't lose synchronization, so next command should work:
+ping-daemon
+EOF
\ No newline at end of file

diff --git a/src/generator.ml b/src/generator.ml
index b5b24cf..1e78a80 100755 (executable)
--- a/src/generator.ml
+++ b/src/generator.ml
@@ -2028,7 +2028,7 @@ Compute the SHA512 hash (using the C<sha512sum> program).
 
 The checksum is returned as a printable string.");
 
-  ("tar_in", (RErr, [FileIn "tarfile"; String "directory"]), 69, [],
+  ("tar_in", (RErr, [FileIn "tarfile"; Pathname "directory"]), 69, [],
    [InitBasicFS, Always, TestOutput (
       [["tar_in"; "../images/helloworld.tar"; "/"];
        ["cat"; "/hello"]], "hello\n")],
@@ -2050,7 +2050,7 @@ it to local file C<tarfile>.
 To download a compressed tarball, use C<guestfs_tgz_out>
 or C<guestfs_txz_out>.");
 
-  ("tgz_in", (RErr, [FileIn "tarball"; String "directory"]), 71, [],
+  ("tgz_in", (RErr, [FileIn "tarball"; Pathname "directory"]), 71, [],
    [InitBasicFS, Always, TestOutput (
       [["tgz_in"; "../images/helloworld.tar.gz"; "/"];
        ["cat"; "/hello"]], "hello\n")],
@@ -4382,7 +4382,7 @@ This command writes zeroes over the entire C<device>.  Compare
 with C<guestfs_zero> which just zeroes the first few blocks of
 a device.");
 
-  ("txz_in", (RErr, [FileIn "tarball"; String "directory"]), 229, [],
+  ("txz_in", (RErr, [FileIn "tarball"; Pathname "directory"]), 229, [],
    [InitBasicFS, Always, TestOutput (
       [["txz_in"; "../images/helloworld.tar.xz"; "/"];
        ["cat"; "/hello"]], "hello\n")],
@@ -6032,14 +6032,19 @@ and generate_daemon_actions () =
       );
       pr "\n";
 
+      let is_filein =
+        List.exists (function FileIn _ -> true | _ -> false) (snd style) in
+
       (match snd style with
        | [] -> ()
        | args ->
            pr "  memset (&args, 0, sizeof args);\n";
            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 "    return;\n";
+           pr "    goto done;\n";
            pr "  }\n";
            let pr_args n =
              pr "  char *%s = args.%s;\n" n n
@@ -6048,6 +6053,8 @@ and generate_daemon_actions () =
              pr "  %s = realloc (args.%s.%s_val,\n" n n n;
              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 "    goto done;\n";
              pr "  }\n";
@@ -6058,13 +6065,16 @@ and generate_daemon_actions () =
              function
              | Pathname n ->
                  pr_args n;
-                 pr "  ABS_PATH (%s, goto done);\n" n;
+                 pr "  ABS_PATH (%s, %s, goto done);\n"
+                   n (if is_filein then "cancel_receive ()" else "");
              | Device n ->
                  pr_args n;
-                 pr "  RESOLVE_DEVICE (%s, goto done);\n" n;
+                 pr "  RESOLVE_DEVICE (%s, %s, goto done);\n"
+                   n (if is_filein then "cancel_receive ()" else "");
              | Dev_or_Path n ->
                  pr_args n;
-                 pr "  REQUIRE_ROOT_OR_RESOLVE_DEVICE (%s, goto done);\n" n;
+                 pr "  REQUIRE_ROOT_OR_RESOLVE_DEVICE (%s, %s, goto done);\n"
+                   n (if is_filein then "cancel_receive ()" else "");
              | String n -> pr_args n
              | OptString n -> pr "  %s = args.%s ? *args.%s : NULL;\n" n n n
              | StringList n ->
@@ -6074,7 +6084,8 @@ and generate_daemon_actions () =
                  pr "  /* Ensure that each is a device,\n";
                  pr "   * and perform device name translation. */\n";
                  pr "  { int pvi; for (pvi = 0; physvols[pvi] != NULL; ++pvi)\n";
-                 pr "    RESOLVE_DEVICE (physvols[pvi], goto done);\n";
+                 pr "    RESOLVE_DEVICE (physvols[pvi], %s, goto done);\n"
+                   (if is_filein then "cancel_receive ()" else "");
                  pr "  }\n";
              | Bool n -> pr "  %s = args.%s;\n" n n
              | Int n -> pr "  %s = args.%s;\n" n n
@@ -6089,7 +6100,8 @@ and generate_daemon_actions () =
       if List.exists (function Pathname _ -> true | _ -> false) (snd style) then (
         (* Emit NEED_ROOT just once, even when there are two or
            more Pathname args *)
-        pr "  NEED_ROOT (goto done);\n";
+        pr "  NEED_ROOT (%s, goto done);\n"
+          (if is_filein then "cancel_receive ()" else "");
       );
 
       (* Don't want to call the impl with any FileIn or FileOut
@@ -6175,15 +6187,14 @@ and generate_daemon_actions () =
       );
 
       (* Free the args. *)
+      pr "done:\n";
       (match snd style with
-       | [] ->
-           pr "done: ;\n";
+       | [] -> ()
        | _ ->
-           pr "done:\n";
            pr "  xdr_free ((xdrproc_t) xdr_guestfs_%s_args, (char *) &args);\n"
              name
       );
-
+      pr "  return;\n";
       pr "}\n\n";
   ) daemon_functions;