summary |
shortlog |
log |
commit | commitdiff |
tree
raw |
patch |
inline | side by side (from parent 1:
11a2ad8)
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
int dev_ok, dev_pts_ok, proc_ok, selinux_ok, sys_ok;
/* We need a root filesystem mounted to do this. */
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
/* Conveniently, argv is already a NULL-terminated argv-style array
* of parameters, so we can pass it straight in to our internal
extern int receive_file (receive_cb cb, void *opaque);
/* daemon functions that receive files (FileIn) can call this
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.
/* daemon functions that return files (FileOut) should call
* reply, then send_file_* for each FileOut parameter.
#define NEED_ROOT(cancel_stmt,fail_stmt) \
do { \
if (!root_mounted) { \
#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__); \
#define ABS_PATH(path,cancel_stmt,fail_stmt) \
do { \
if ((path)[0] != '/') { \
#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)
fail_stmt; \
} \
} while (0)
#define RESOLVE_DEVICE(path,cancel_stmt,fail_stmt) \
do { \
if (STRNEQLEN ((path), "/dev/", 5)) { \
#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; \
fail_stmt; \
} \
if (device_name_translation ((path)) == -1) { \
int err = errno; \
- reply_with_perror ("%s: %s", __func__, path); \
+ if (r != -2) \
+ reply_with_perror ("%s: %s", __func__, path); \
fail_stmt; \
} \
} while (0)
fail_stmt; \
} \
} while (0)
- NEED_ROOT (, return NULL);
+ NEED_ROOT (0, return NULL);
r = command (&out, &err, "df", NULL);
if (r == -1) {
r = command (&out, &err, "df", NULL);
if (r == -1) {
- NEED_ROOT (, return NULL);
+ NEED_ROOT (0, return NULL);
r = command (&out, &err, "df", "-h", NULL);
if (r == -1) {
r = command (&out, &err, "df", "-h", NULL);
if (r == -1) {
#ifdef HAVE_SYS_INOTIFY_H
FILE *fp;
#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");
if (max_events < 0) {
reply_with_error ("max_events < 0");
- ABS_PATH (mountpoint, , return -1);
+ ABS_PATH (mountpoint, 0, return -1);
is_root = STREQ (mountpoint, "/");
is_root = STREQ (mountpoint, "/");
- RESOLVE_DEVICE (buf, , { free (buf); return -1; });
+ RESOLVE_DEVICE (buf, 0, { free (buf); return -1; });
r = command (NULL, &err, "umount", buf, NULL);
free (buf);
r = command (NULL, &err, "umount", buf, NULL);
free (buf);
int r;
/* NEED_ROOT (return -1); - we don't want this test for this call. */
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);
CHROOT_IN;
r = mkdir (path, 0777);
int r;
/* NEED_ROOT (return -1); - we don't want this test for this call. */
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);
CHROOT_IN;
r = rmdir (path);
}
/* Send a cancellation flag back to the library. */
}
/* Send a cancellation flag back to the library. */
cancel_receive (void)
{
XDR xdr;
cancel_receive (void)
{
XDR xdr;
if (xwrite (sock, fbuf, sizeof fbuf) == -1) {
perror ("write to socket");
if (xwrite (sock, fbuf, sizeof fbuf) == -1) {
perror ("write to socket");
}
/* Keep receiving chunks and discarding, until library sees cancel. */
}
/* 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);
}
static int check_for_library_cancellation (void);
/* "tar -C /sysroot%s -xf -" but we have to quote the dir. */
if (asprintf_nowarn (&cmd, "tar -C %R -xf -", dir) == -1) {
err = errno;
/* "tar -C /sysroot%s -xf -" but we have to quote the dir. */
if (asprintf_nowarn (&cmd, "tar -C %R -xf -", dir) == -1) {
err = errno;
- reply_with_perror ("asprintf");
+ if (r != -2) reply_with_perror ("asprintf");
fp = popen (cmd, "w");
if (fp == NULL) {
err = errno;
fp = popen (cmd, "w");
if (fp == NULL) {
err = errno;
- reply_with_perror ("%s", cmd);
+ if (r != -2) reply_with_perror ("%s", cmd);
r = receive_file (write_cb, &fd);
if (r == -1) { /* write error */
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;
}
pclose (fp);
return -1;
}
if (pclose (fp) != 0) {
if (r == -1) /* if r == 0, file transfer ended already */
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);
/* "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;
/* "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;
- reply_with_perror ("asprintf");
+ if (r != -2) reply_with_perror ("asprintf");
fp = popen (cmd, "w");
if (fp == NULL) {
err = errno;
fp = popen (cmd, "w");
if (fp == NULL) {
err = errno;
- reply_with_perror ("%s", cmd);
+ if (r != -2) reply_with_perror ("%s", cmd);
r = receive_file (write_cb, &fd);
if (r == -1) { /* write error */
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;
}
pclose (fp);
return -1;
}
if (pclose (fp) != 0) {
if (r == -1) /* if r == 0, file transfer ended already */
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);
if (!is_dev) CHROOT_OUT;
if (fd == -1) {
err = errno;
if (!is_dev) CHROOT_OUT;
if (fd == -1) {
err = errno;
- 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;
return -1;
}
r = receive_file (write_cb, &fd);
if (r == -1) { /* write error */
err = errno;
- reply_with_error ("write error: %s", filename);
+ if (r != -2) reply_with_error ("write error: %s", filename);
if (close (fd) == -1) {
err = errno;
if (r == -1) /* if r == 0, file transfer ended already */
if (close (fd) == -1) {
err = errno;
if (r == -1) /* if r == 0, file transfer ended already */
- reply_with_perror ("close: %s", filename);
+ if (r != -2)
+ reply_with_perror ("close: %s", filename);
-upload $srcdir/rhbz576879.sh /test.sh
# Shouldn't lose synchronization, so next command should work:
ping-daemon
-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
pr "\n";
pr " if (!xdr_guestfs_%s_args (xdr_in, &args)) {\n" name;
if is_filein then
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 =
pr " goto done;\n";
pr " }\n";
let pr_args n =
pr " sizeof (char *) * (args.%s.%s_len+1));\n" n n;
pr " if (%s == NULL) {\n" n;
if is_filein then
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;
pr " goto done;\n";
pr " }\n";
pr " %s[args.%s.%s_len] = NULL;\n" n n n;
| Pathname n ->
pr_args n;
pr " ABS_PATH (%s, %s, goto done);\n"
| 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"
| 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"
| 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 ->
| String n -> pr_args n
| OptString n -> pr " %s = args.%s ? *args.%s : NULL;\n" n n n
| StringList n ->
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"
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
pr " }\n";
| Bool n -> pr " %s = args.%s;\n" n n
| Int n -> pr " %s = args.%s;\n" n n
(* Emit NEED_ROOT just once, even when there are two or
more Pathname args *)
pr " NEED_ROOT (%s, goto done);\n"
(* 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
);
(* Don't want to call the impl with any FileIn or FileOut