[Libguestfs] [PATCH 1/2] proto: Fix FileIn ops that abort during the chunk upload stage.
Richard W.M. Jones
rjones at redhat.com
Fri Mar 18 18:54:37 UTC 2011
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 80 OCaml packages (the OPEN alternative to F#)
http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora
-------------- next part --------------
>From 33b638109ed66ea360b53b80b1f407b3a5f5ec39 Mon Sep 17 00:00:00 2001
From: Richard W.M. Jones <rjones at redhat.com>
Date: Fri, 18 Mar 2011 17:17:30 +0000
Subject: [PATCH 1/2] proto: Fix FileIn ops that abort during the chunk upload stage.
As a previous, incorrect attempt to fix RHBZ#576879 we tried to
prevent the daemon from sending an error reply if the daemon had
cancelled the transfer. This is wrong: the daemon should send an
error reply in these cases.
A simple test case is this:
guestfish -N fs -m /dev/sda1 upload big-file /
(This fails because the target "/" is a directory, not a file.)
Prior to this commit, libguestfs would hang instead of printing an
error. With this commit, libguestfs prints an error.
What is happening is:
(1) Library is uploading
a file (2) In the middle of the long
upload, daemon detects an error.
Daemon cancels.
(3) Library detects cancel,
sends cancel chunk, then waits
for the error reply from the
daemon. (4) Daemon is supposed to send
an error reply message.
Because step (4) wasn't happening, uploads that failed like this would
hang in the library (waiting for the error message, while the daemon
was waiting for the next request).
This also adds a regression test.
This temporarily breaks the "both ends cancel" case (RHBZ#576879c5).
Therefore the test for that is disabled, and this is fixed in the next
patch in the series.
This partially reverts commit dc706a639eec16084c0618baf7bfde00c6565f63.
---
daemon/command.c | 2 +-
daemon/daemon.h | 21 +++++++-----------
daemon/df.c | 4 +-
daemon/inotify.c | 2 +-
daemon/mount.c | 8 +++---
daemon/tar.c | 23 ++++++++-----------
daemon/upload.c | 9 +++----
generator/generator_daemon.ml | 10 ++++----
regressions/Makefile.am | 7 +++--
regressions/rhbz576879.sh | 33 ++++++++++++++++++++++++++++
regressions/rhbz576879c0.sh | 33 ----------------------------
regressions/rhbz576879c5.sh | 32 ---------------------------
regressions/test-both-ends-cancel.sh | 32 +++++++++++++++++++++++++++
regressions/test-upload-to-dir.sh | 39 ++++++++++++++++++++++++++++++++++
14 files changed, 143 insertions(+), 112 deletions(-)
create mode 100755 regressions/rhbz576879.sh
delete mode 100755 regressions/rhbz576879c0.sh
delete mode 100755 regressions/rhbz576879c5.sh
create mode 100755 regressions/test-both-ends-cancel.sh
create mode 100755 regressions/test-upload-to-dir.sh
diff --git a/daemon/command.c b/daemon/command.c
index 8ad5db5..5a194a4 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 (0, 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 4a686c0..79f41ae 100644
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -152,10 +152,6 @@ 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).
- *
- * 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 int cancel_receive (void);
@@ -181,8 +177,8 @@ extern void notify_progress (uint64_t position, uint64_t total);
#define NEED_ROOT(cancel_stmt,fail_stmt) \
do { \
if (!is_root_mounted ()) { \
- if ((cancel_stmt) != -2) \
- reply_with_error ("%s: you must call 'mount' first to mount the root filesystem", __func__); \
+ cancel_stmt; \
+ reply_with_error ("%s: you must call 'mount' first to mount the root filesystem", __func__); \
fail_stmt; \
} \
} \
@@ -194,8 +190,8 @@ extern void notify_progress (uint64_t position, uint64_t total);
#define ABS_PATH(path,cancel_stmt,fail_stmt) \
do { \
if ((path)[0] != '/') { \
- if ((cancel_stmt) != -2) \
- reply_with_error ("%s: path must start with a / character", __func__); \
+ cancel_stmt; \
+ reply_with_error ("%s: path must start with a / character", __func__); \
fail_stmt; \
} \
} while (0)
@@ -210,18 +206,17 @@ extern void notify_progress (uint64_t position, uint64_t total);
#define RESOLVE_DEVICE(path,cancel_stmt,fail_stmt) \
do { \
if (STRNEQLEN ((path), "/dev/", 5)) { \
- if ((cancel_stmt) != -2) \
- reply_with_error ("%s: %s: expecting a device name", __func__, (path)); \
+ cancel_stmt; \
+ reply_with_error ("%s: %s: expecting a device name", __func__, (path)); \
fail_stmt; \
} \
if (is_root_device (path)) \
reply_with_error ("%s: %s: device not found", __func__, path); \
if (device_name_translation ((path)) == -1) { \
int err = errno; \
- int r = cancel_stmt; \
+ cancel_stmt; \
errno = err; \
- if (r != -2) \
- reply_with_perror ("%s: %s", __func__, path); \
+ reply_with_perror ("%s: %s", __func__, path); \
fail_stmt; \
} \
} while (0)
diff --git a/daemon/df.c b/daemon/df.c
index a1ec706..7127c00 100644
--- a/daemon/df.c
+++ b/daemon/df.c
@@ -33,7 +33,7 @@ do_df (void)
int r;
char *out, *err;
- NEED_ROOT (0, 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 (0, return NULL);
+ NEED_ROOT (, return NULL);
r = command (&out, &err, "df", "-h", NULL);
if (r == -1) {
diff --git a/daemon/inotify.c b/daemon/inotify.c
index 807fb2f..8e8b690 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 (0, 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 0c990c3..a915b44 100644
--- a/daemon/mount.c
+++ b/daemon/mount.c
@@ -83,7 +83,7 @@ do_mount_vfs (const char *options, const char *vfstype,
char *error;
struct stat statbuf;
- ABS_PATH (mountpoint, 0, return -1);
+ ABS_PATH (mountpoint, , return -1);
mp = sysroot_path (mountpoint);
if (!mp) {
@@ -158,7 +158,7 @@ do_umount (const char *pathordevice)
}
if (is_dev)
- RESOLVE_DEVICE (buf, 0, { free (buf); return -1; });
+ RESOLVE_DEVICE (buf, , { free (buf); return -1; });
r = command (NULL, &err, "umount", buf, NULL);
free (buf);
@@ -409,7 +409,7 @@ do_mkmountpoint (const char *path)
int r;
/* NEED_ROOT (return -1); - we don't want this test for this call. */
- ABS_PATH (path, 0, return -1);
+ ABS_PATH (path, , return -1);
CHROOT_IN;
r = mkdir (path, 0777);
@@ -429,7 +429,7 @@ do_rmmountpoint (const char *path)
int r;
/* NEED_ROOT (return -1); - we don't want this test for this call. */
- ABS_PATH (path, 0, return -1);
+ ABS_PATH (path, , return -1);
CHROOT_IN;
r = rmdir (path);
diff --git a/daemon/tar.c b/daemon/tar.c
index 2ddde88..3c756af 100644
--- a/daemon/tar.c
+++ b/daemon/tar.c
@@ -85,7 +85,7 @@ do_tXz_in (const char *dir, const char *filter)
err = errno;
r = cancel_receive ();
errno = err;
- if (r != -2) reply_with_perror ("asprintf");
+ reply_with_perror ("asprintf");
return -1;
}
@@ -97,7 +97,7 @@ do_tXz_in (const char *dir, const char *filter)
err = errno;
r = cancel_receive ();
errno = err;
- if (r != -2) reply_with_perror ("%s", cmd);
+ reply_with_perror ("%s", cmd);
free (cmd);
return -1;
}
@@ -110,11 +110,10 @@ do_tXz_in (const char *dir, const char *filter)
r = receive_file (write_cb, &fd);
if (r == -1) { /* write error */
- if (cancel_receive () != -2) {
- char *errstr = read_error_file ();
- reply_with_error ("write error on directory: %s: %s", dir, errstr);
- free (errstr);
- }
+ cancel_receive ();
+ char *errstr = read_error_file ();
+ reply_with_error ("write error on directory: %s: %s", dir, errstr);
+ free (errstr);
pclose (fp);
return -1;
}
@@ -127,12 +126,10 @@ do_tXz_in (const char *dir, const char *filter)
if (pclose (fp) != 0) {
if (r == -1) /* if r == 0, file transfer ended already */
r = cancel_receive ();
- if (r != -2) {
- char *errstr = read_error_file ();
- reply_with_error ("tar subcommand failed on directory: %s: %s",
- dir, errstr);
- free (errstr);
- }
+ char *errstr = read_error_file ();
+ reply_with_error ("tar subcommand failed on directory: %s: %s",
+ dir, errstr);
+ free (errstr);
return -1;
}
diff --git a/daemon/upload.c b/daemon/upload.c
index e28bf96..f8d312f 100644
--- a/daemon/upload.c
+++ b/daemon/upload.c
@@ -70,7 +70,7 @@ upload (const char *filename, int flags, int64_t offset)
err = errno;
r = cancel_receive ();
errno = err;
- if (r != -2) reply_with_perror ("%s", filename);
+ reply_with_perror ("%s", filename);
return -1;
}
@@ -79,7 +79,7 @@ upload (const char *filename, int flags, int64_t offset)
err = errno;
r = cancel_receive ();
errno = err;
- if (r != -2) reply_with_perror ("lseek: %s", filename);
+ reply_with_perror ("lseek: %s", filename);
return -1;
}
}
@@ -89,7 +89,7 @@ upload (const char *filename, int flags, int64_t offset)
err = errno;
r = cancel_receive ();
errno = err;
- if (r != -2) reply_with_error ("write error: %s", filename);
+ reply_with_error ("write error: %s", filename);
close (data.fd);
return -1;
}
@@ -104,8 +104,7 @@ upload (const char *filename, int flags, int64_t offset)
if (r == -1) /* if r == 0, file transfer ended already */
r = cancel_receive ();
errno = err;
- if (r != -2)
- reply_with_perror ("close: %s", filename);
+ reply_with_perror ("close: %s", filename);
return -1;
}
diff --git a/generator/generator_daemon.ml b/generator/generator_daemon.ml
index 21a5888..8c3a548 100644
--- a/generator/generator_daemon.ml
+++ b/generator/generator_daemon.ml
@@ -165,15 +165,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 "0");
+ n (if is_filein then "cancel_receive ()" else "");
| Device n ->
pr_args n;
pr " RESOLVE_DEVICE (%s, %s, goto done);\n"
- n (if is_filein then "cancel_receive ()" else "0");
+ n (if is_filein then "cancel_receive ()" else "");
| 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 "0");
+ n (if is_filein then "cancel_receive ()" else "");
| String n | Key n -> pr_args n
| OptString n -> pr " %s = args.%s ? *args.%s : NULL;\n" n n n
| StringList n ->
@@ -187,7 +187,7 @@ and generate_daemon_actions () =
pr " size_t i;\n";
pr " for (i = 0; %s[i] != NULL; ++i)\n" n;
pr " RESOLVE_DEVICE (%s[i], %s, goto done);\n" n
- (if is_filein then "cancel_receive ()" else "0");
+ (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
@@ -206,7 +206,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 "0");
+ (if is_filein then "cancel_receive ()" else "");
);
(* Don't want to call the impl with any FileIn or FileOut
diff --git a/regressions/Makefile.am b/regressions/Makefile.am
index a64d272..6527e06 100644
--- a/regressions/Makefile.am
+++ b/regressions/Makefile.am
@@ -29,8 +29,7 @@ TESTS = \
rhbz503169c10.sh \
rhbz503169c13.sh \
rhbz557655.sh \
- rhbz576879c0.sh \
- rhbz576879c5.sh \
+ rhbz576879.sh \
rhbz578407.sh \
rhbz580246.sh \
test-add-domain.sh \
@@ -53,12 +52,14 @@ TESTS = \
test-read_file.sh \
test-remote.sh \
test-reopen.sh \
- test-stringlist.sh
+ test-stringlist.sh \
+ test-upload-to-dir.sh
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/regressions/rhbz576879.sh b/regressions/rhbz576879.sh
new file mode 100755
index 0000000..d3db55b
--- /dev/null
+++ b/regressions/rhbz576879.sh
@@ -0,0 +1,33 @@
+#!/bin/bash -
+# libguestfs
+# Copyright (C) 2010-2011 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#c0
+# upload loses synchronization if the disk is not mounted
+
+set -e
+
+rm -f test1.img
+
+../fish/guestfish -N disk <<EOF
+-upload $srcdir/rhbz576879.sh /test.sh
+# Shouldn't lose synchronization, so next command should work:
+ping-daemon
+EOF
+
+rm -f test1.img
\ No newline at end of file
diff --git a/regressions/rhbz576879c0.sh b/regressions/rhbz576879c0.sh
deleted file mode 100755
index f060496..0000000
--- a/regressions/rhbz576879c0.sh
+++ /dev/null
@@ -1,33 +0,0 @@
-#!/bin/bash -
-# libguestfs
-# Copyright (C) 2010-2011 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#c0
-# upload loses synchronization if the disk is not mounted
-
-set -e
-
-rm -f test1.img
-
-../fish/guestfish -N disk <<EOF
--upload $srcdir/rhbz576879c0.sh /test.sh
-# Shouldn't lose synchronization, so next command should work:
-ping-daemon
-EOF
-
-rm -f test1.img
\ No newline at end of file
diff --git a/regressions/rhbz576879c5.sh b/regressions/rhbz576879c5.sh
deleted file mode 100755
index 817e098..0000000
--- a/regressions/rhbz576879c5.sh
+++ /dev/null
@@ -1,32 +0,0 @@
-#!/bin/bash -
-# libguestfs
-# Copyright (C) 2010-2011 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#c5
-# where both ends send cancel messages simultaneously.
-
-set -e
-
-rm -f test1.img
-
-../fish/guestfish -N disk <<EOF
--tar-in /tmp/nosuchfile /blah
-ping-daemon
-EOF
-
-rm -f test1.img
diff --git a/regressions/test-both-ends-cancel.sh b/regressions/test-both-ends-cancel.sh
new file mode 100755
index 0000000..817e098
--- /dev/null
+++ b/regressions/test-both-ends-cancel.sh
@@ -0,0 +1,32 @@
+#!/bin/bash -
+# libguestfs
+# Copyright (C) 2010-2011 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#c5
+# where both ends send cancel messages simultaneously.
+
+set -e
+
+rm -f test1.img
+
+../fish/guestfish -N disk <<EOF
+-tar-in /tmp/nosuchfile /blah
+ping-daemon
+EOF
+
+rm -f test1.img
diff --git a/regressions/test-upload-to-dir.sh b/regressions/test-upload-to-dir.sh
new file mode 100755
index 0000000..03cce5b
--- /dev/null
+++ b/regressions/test-upload-to-dir.sh
@@ -0,0 +1,39 @@
+#!/bin/bash -
+# libguestfs
+# Copyright (C) 2011 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.
+
+# If you used the guestfish command 'upload' and accidentally set the
+# target to a directory instead of the full filename, then previously
+# libguestfs would hang. It should return an error instead.
+
+set -e
+
+rm -f test1.img test.out
+
+if ../fish/guestfish -N fs -m /dev/sda1 upload ../images/test.iso / 2>test.out
+then
+ echo "$0: expecting guestfish to return an error"
+ exit 1
+fi
+
+if ! grep -q "upload: /: Is a directory" test.out; then
+ echo "$0: unexpected error message from guestfish"
+ cat test.out
+ exit 1
+fi
+
+rm -f test1.img test.out
--
1.7.4.1
More information about the Libguestfs
mailing list