Libguestfs - Apr 2010 - [PATCH 0/2] Fix tar-in, tgz-in commands (RHBZ#580246)

Two-part patch to fix RHBZ#580246.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top

-- 
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 07f4b20ae959069fca41756b0dc103ec5fa99754 Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones at redhat.com>
Date: Thu, 8 Apr 2010 08:48:38 +0100
Subject: [PATCH 1/2] Fix tar-in command hangs when running out of disk space
(RHBZ#580246).

The problem was this sequence of events:

 (1) File transfer goes through OK.

 (2) pclose returns failure (because 'tar' subprocess failed)

 (3) We try to cancel the transfer by calling cancel_receive.

Step (3) fails because the transfer (as far as the library is
concerned) has succeeded, so causing a hang.

The more fundamental reason why we see steps (1) and (2) is that
'tar' does NOT fail immediately if there is a write error.  Instead
it continues reading and discarding the input until the end of the
input before giving "Error exit delayed from previous errors".
IMHO this is a bug with tar, since an ENOSPC write error should
be fatal for tar.
---
 daemon/tar.c    |    6 ++++--
 daemon/upload.c |    3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/daemon/tar.c b/daemon/tar.c
index ebcaded..bb0e483 100644
--- a/daemon/tar.c
+++ b/daemon/tar.c
@@ -88,7 +88,8 @@ do_tar_in (const char *dir)
 
   if (pclose (fp) != 0) {
     err = errno;
-    cancel_receive ();
+    if (r == -1)                /* if r == 0, file transfer ended already */
+      cancel_receive ();
     errno = err;
     reply_with_perror ("pclose: %s", dir);
     return -1;
@@ -209,7 +210,8 @@ do_tgz_in (const char *dir)
 
   if (pclose (fp) != 0) {
     err = errno;
-    cancel_receive ();
+    if (r == -1)                /* if r == 0, file transfer ended already */
+      cancel_receive ();
     errno = err;
     reply_with_perror ("pclose: %s", dir);
     return -1;
diff --git a/daemon/upload.c b/daemon/upload.c
index e15eade..65c6667 100644
--- a/daemon/upload.c
+++ b/daemon/upload.c
@@ -77,7 +77,8 @@ do_upload (const char *filename)
 
   if (close (fd) == -1) {
     err = errno;
-    cancel_receive ();
+    if (r == -1)                /* if r == 0, file transfer ended already */
+      cancel_receive ();
     errno = err;
     reply_with_perror ("close: %s", filename);
     return -1;
-- 
1.6.6.1

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v
-------------- next part --------------
>From 514fa1fe48c38213cebc71b787469ed36dcf367a Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones at redhat.com>
Date: Wed, 7 Apr 2010 21:04:01 +0100
Subject: [PATCH 2/2] Code cleanups related to RHBZ#580246.

This includes various code cleanups:

 (a) A regression test for RHBZ#580246.

 (b) Use write instead of fwrite to write out the tar file.  This is
     just because the error handling of write seems to be better
     specified and easier to use.

 (c) Use size_t instead of int for length.

 (d) Clearer debug messages when in verbose mode.
---
 daemon/daemon.h           |    2 +-
 daemon/proto.c            |   17 ++++++++++++---
 daemon/tar.c              |   23 +++++++++++++++------
 daemon/upload.c           |    4 +-
 regressions/Makefile.am   |    1 +
 regressions/rhbz580246.sh |   47 +++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 80 insertions(+), 14 deletions(-)
 create mode 100755 regressions/rhbz580246.sh

diff --git a/daemon/daemon.h b/daemon/daemon.h
index 19dd69c..ebbeaa2 100644
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -133,7 +133,7 @@ extern void reply_with_perror_errno (int err, const char
*fs, ...)
 /* daemon functions that receive files (FileIn) should call
  * receive_file for each FileIn parameter.
  */
-typedef int (*receive_cb) (void *opaque, const void *buf, int len);
+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
diff --git a/daemon/proto.c b/daemon/proto.c
index 0002d80..ee1c400 100644
--- a/daemon/proto.c
+++ b/daemon/proto.c
@@ -324,6 +324,9 @@ receive_file (receive_cb cb, void *opaque)
   uint32_t len;
 
   for (;;) {
+    if (verbose)
+      fprintf (stderr, "receive_file: reading length word\n");
+
     /* Read the length word. */
     if (xread (sock, lenbuf, 4) == -1)
       exit (EXIT_FAILURE);
@@ -361,15 +364,18 @@ receive_file (receive_cb cb, void *opaque)
     free (buf);
 
     if (verbose)
-      printf ("receive_file: got chunk: cancel = %d, len = %d, buf =
%p\n",
-              chunk.cancel, chunk.data.data_len, chunk.data.data_val);
+      fprintf (stderr, "receive_file: got chunk: cancel = %d, len = %d,
buf = %p\n",
+               chunk.cancel, chunk.data.data_len, chunk.data.data_val);
 
     if (chunk.cancel) {
-      fprintf (stderr, "receive_file: received cancellation from
library\n");
+      if (verbose)
+        fprintf (stderr, "receive_file: received cancellation from
library\n");
       xdr_free ((xdrproc_t) xdr_guestfs_chunk, (char *) &chunk);
       return -2;
     }
     if (chunk.data.data_len == 0) {
+      if (verbose)
+        fprintf (stderr, "receive_file: end of file, leaving
function\n");
       xdr_free ((xdrproc_t) xdr_guestfs_chunk, (char *) &chunk);
       return 0;			/* end of file */
     }
@@ -380,8 +386,11 @@ receive_file (receive_cb cb, void *opaque)
       r = 0;
 
     xdr_free ((xdrproc_t) xdr_guestfs_chunk, (char *) &chunk);
-    if (r == -1)		/* write error */
+    if (r == -1) {		/* write error */
+      if (verbose)
+        fprintf (stderr, "receive_file: write error\n");
       return -1;
+    }
   }
 }
 
diff --git a/daemon/tar.c b/daemon/tar.c
index bb0e483..dfae741 100644
--- a/daemon/tar.c
+++ b/daemon/tar.c
@@ -23,15 +23,17 @@
 #include <string.h>
 #include <fcntl.h>
 
+#include "ignore-value.h"
+
 #include "../src/guestfs_protocol.h"
 #include "daemon.h"
 #include "actions.h"
 
 static int
-fwrite_cb (void *fp_ptr, const void *buf, int len)
+write_cb (void *fd_ptr, const void *buf, size_t len)
 {
-  FILE *fp = *(FILE **)fp_ptr;
-  return fwrite (buf, len, 1, fp) == 1 ? 0 : -1;
+  int fd = *(int *)fd_ptr;
+  return xwrite (fd, buf, len);
 }
 
 /* Has one FileIn parameter. */
@@ -71,12 +73,17 @@ do_tar_in (const char *dir)
   }
   free (cmd);
 
-  r = receive_file (fwrite_cb, &fp);
+  /* The semantics of fwrite are too undefined, so write to the
+   * file descriptor directly instead.
+   */
+  int fd = fileno (fp);
+
+  r = receive_file (write_cb, &fd);
   if (r == -1) {		/* write error */
     err = errno;
     cancel_receive ();
     errno = err;
-    reply_with_perror ("write: %s", dir);
+    reply_with_error ("write error on directory: %s", dir);
     pclose (fp);
     return -1;
   }
@@ -193,12 +200,14 @@ do_tgz_in (const char *dir)
   }
   free (cmd);
 
-  r = receive_file (fwrite_cb, &fp);
+  int fd = fileno (fp);
+
+  r = receive_file (write_cb, &fd);
   if (r == -1) {		/* write error */
     err = errno;
     cancel_receive ();
     errno = err;
-    reply_with_perror ("write: %s", dir);
+    reply_with_error ("write error on directory: %s", dir);
     pclose (fp);
     return -1;
   }
diff --git a/daemon/upload.c b/daemon/upload.c
index 65c6667..3c20d6f 100644
--- a/daemon/upload.c
+++ b/daemon/upload.c
@@ -28,7 +28,7 @@
 #include "actions.h"
 
 static int
-write_cb (void *fd_ptr, const void *buf, int len)
+write_cb (void *fd_ptr, const void *buf, size_t len)
 {
   int fd = *(int *)fd_ptr;
   return xwrite (fd, buf, len);
@@ -65,7 +65,7 @@ do_upload (const char *filename)
     err = errno;
     cancel_receive ();
     errno = err;
-    reply_with_perror ("write: %s", filename);
+    reply_with_error ("write error: %s", filename);
     close (fd);
     return -1;
   }
diff --git a/regressions/Makefile.am b/regressions/Makefile.am
index 2710e82..e8e43cf 100644
--- a/regressions/Makefile.am
+++ b/regressions/Makefile.am
@@ -27,6 +27,7 @@ TESTS = \
 	rhbz503169c10.sh \
 	rhbz503169c13.sh \
 	rhbz557655.sh \
+	rhbz580246.sh \
 	test-cancellation-download-librarycancels.sh \
 	test-cancellation-upload-daemoncancels.sh \
 	test-find0.sh \
diff --git a/regressions/rhbz580246.sh b/regressions/rhbz580246.sh
new file mode 100755
index 0000000..707231e
--- /dev/null
+++ b/regressions/rhbz580246.sh
@@ -0,0 +1,47 @@
+#!/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.
+
+# Test tar_in call when we upload something which is larger than
+# available space.
+# https://bugzilla.redhat.com/show_bug.cgi?id=580246
+
+set -e
+export LANG=C
+
+rm -f test.img test.tar
+
+dd if=/dev/zero of=test.img bs=1M count=2
+tar cf test.tar test.img
+
+output=$(
+../fish/guestfish 2>&1 <<'EOF'
+add test.img
+run
+mkfs ext2 /dev/sda
+mount /dev/sda /
+-tar-in test.tar /
+EOF
+)
+
+rm -f test.img test.tar
+
+# Check for error message in the output.
+if [[ ! $output =~ libguestfs:.error:.tar_in ]]; then
+    echo "Missing error message from tar-in (expecting an error
message)"
+    exit 1
+fi
-- 
1.6.6.1