Libguestfs - Nov 2009 - [PATCH 0/9] FOR DISCUSSION ONLY: daemon error handling

The more I look at this patch, the less I like it.  I would summarise
why I think it's wrong here, but it's better if you look at the
message I posted on the gnulib mailing list here first:

http://lists.gnu.org/archive/html/bug-gnulib/2009-11/msg00434.html

Directly accessing errno on Windows is wrong: you won't see the true
reasons for an error by doing that.  However depending on what the
call is and whether it was replaced by gnulib, the alternatives are a
lot more complicated.

This patch series attempts to Do The Right Thing with gnulib code as
it stands at the moment (assuming my understanding of gnulib error
handling is correct -- see above link).

I feel however that it's probably better to change gnulib error
handling to be more rational.

There's also an additional complication I discovered: MinGW error
values are often wrong.  So that's a MinGW bug to take to the nice
upstream folks on the MinGW project.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://et.redhat.com/~rjones/libguestfs/
See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html

-- 
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 cc70a8c8b4c0cae61b0c1268bab56d6817fd23a9 Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones at redhat.com>
Date: Fri, 27 Nov 2009 13:43:05 +0000
Subject: [PATCH 1/9] daemon error handling: Formalize access to errno.

Windows itself doesn't have an errno variable, although Gnulib
provides a dummy variable (but Windows won't use it, only Gnulib).

In a lot of places in the daemon we access errno directly, eg.
saving and restoring it.  These places aren't safe on Windows.
Instead we provide a simple API for this.

Example, replace:

  int saved_errno = errno;
  do_some_cleanup ();
  errno = saved_errno;
  reply_with_perror ("failed");

with:

  errtype err = get_error ();
  do_some_cleanup ();
  reply_with_perror_with_err (err, "failed");

There's also a method to clear errors (clear_error()).  But we don't
provide a way to set errno to arbitrary values, because the code
doesn't need that and it's hard to do it in Windows.

In some cases you do really need access to errno (not "error"),
either because you know the code won't be used on Win32, or
because Gnulib replacement function sets errno and you want to
check it, so provide also a get_errno function.

Also strerror doesn't exist on Windows, and the Gnulib replacement
only works with network errors.  Replace it with a call to Format-
Message as documented on MSDN.
---
 daemon/daemon.h |   73 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 daemon/proto.c  |   29 +++++++++++++++++++---
 2 files changed, 94 insertions(+), 8 deletions(-)

diff --git a/daemon/daemon.h b/daemon/daemon.h
index e7e77e9..ea1e66f 100644
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -27,6 +27,10 @@
 #include <rpc/types.h>
 #include <rpc/xdr.h>
 
+#ifdef HAVE_WINDOWS_H
+#include <windows.h>
+#endif
+
 #include "../src/guestfs_protocol.h"
 
 /*-- in guestfsd.c --*/
@@ -116,11 +120,72 @@ extern int sync_disks (void);
 /*-- in proto.c --*/
 extern void main_loop (int sock) __attribute__((noreturn));
 
-/* ordinary daemon functions use these to indicate errors */
+/* Don't manipulate errno directly, use these instead.
+ *
+ * For example:
+ *   if (some_syscall (...) == -1) {
+ *     errtype err = get_error ();   // Save the error number.
+ *     do_some_cleanup ();           // This might destroy errno.
+ *     reply_with_perror_with_err (err, ...);  // Report original error.
+ *     return -1;
+ *   }
+ *
+ * The _sock_ variations are only used for socket calls (Win32 has a
+ * different way to return errors from socket calls).  In reality
+ * these variants are never used because the only socket we talk to is
+ * the vmchannel socket, and if that throws an error, we die.
+ */
+#ifndef WIN32
+typedef int errtype;
+static inline void clear_error(void) { errno = 0; }
+static inline int get_error(void) { return errno; }
+static inline int get_sock_error(void) { return errno; }
+#else
+typedef DWORD errtype;
+static inline void clear_error(void)
+{
+  SetLastError (0);
+  WSASetLastError (0);
+  errno = 0;
+}
+static inline DWORD get_error(void) { return GetLastError (); }
+static inline DWORD get_sock_error(void) { return WSAGetLastError (); }
+#endif
+
+/* This is for where you really want _errno_.
+ *
+ * The only case where you should check errno is when U || (GL && GE):
+ *
+ * U = the code only runs on a Unix system
+ * GL = a system call is being replaced by a Gnulib function
+ * GE = the Gnulib function sets errno to some value which you wish to check.
+ *
+ * These cases require careful testing to make sure they work properly on
+ * Windows.
+ */
+static inline int get_errno(void) { return errno; }
+
+/* Finally, discourage direct use of errno. */
+#ifdef errno
+# undef errno
+#endif
+#define errno dont_use_errno_directly_see_daemon_h
+
+/* Ordinary daemon functions use these to indicate errors.
+ * reply_with_error: Report an error.
+ * reply_with_perror: Report an error followed by errno message.
+ * reply_with_perror_sock: Same as reply_with_perror for socket calls.
+ * reply_with_perror_with_err: Same as reply_with_perror but use an
+ *   error code that you saved previously from get_error().
+ */
 extern void reply_with_error (const char *fs, ...)
   __attribute__((format (printf,1,2)));
-extern void reply_with_perror (const char *fs, ...)
-  __attribute__((format (printf,1,2)));
+#define reply_with_perror(...) \
+  reply_with_perror_with_err (get_error(),__VA_ARGS__)
+#define reply_with_perror_sock(...) \
+  reply_with_perror_with_err (get_sock_error(),__VA_ARGS__)
+extern void reply_with_perror_with_err (errtype err, const char *fs, ...)
+  __attribute__((format (printf,2,3)));
 
 /* daemon functions that receive files (FileIn) should call
  * receive_file for each FileIn parameter.
@@ -130,7 +195,7 @@ 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_error or reply_with_perror.
+ * but they MUST then call reply_with_*.
  */
 extern void cancel_receive (void);
 
diff --git a/daemon/proto.c b/daemon/proto.c
index 2231037..7c923c8 100644
--- a/daemon/proto.c
+++ b/daemon/proto.c
@@ -23,12 +23,15 @@
 #include <stdarg.h>
 #include <string.h>
 #include <unistd.h>
-#include <errno.h>
 #include <sys/param.h>		/* defines MIN */
 #include <sys/select.h>
 #include <rpc/types.h>
 #include <rpc/xdr.h>
 
+#ifdef HAVE_WINDOWS_H
+#include <windows.h>
+#endif
+
 #include "c-ctype.h"
 #include "ignore-value.h"
 
@@ -180,18 +183,36 @@ reply_with_error (const char *fs, ...)
 }
 
 void
-reply_with_perror (const char *fs, ...)
+reply_with_perror_with_err (errtype err, const char *fs, ...)
 {
   char buf1[GUESTFS_ERROR_LEN];
   char buf2[GUESTFS_ERROR_LEN];
   va_list args;
-  int err = errno;
+  char *errmsg;
 
   va_start (args, fs);
   vsnprintf (buf1, sizeof buf1, fs, args);
   va_end (args);
 
-  snprintf (buf2, sizeof buf2, "%s: %s", buf1, strerror (err));
+#ifdef WIN32
+  /* http://msdn.microsoft.com/en-us/library/ms680582%28VS.85%29.aspx */
+  FormatMessage (
+        FORMAT_MESSAGE_ALLOCATE_BUFFER | 
+        FORMAT_MESSAGE_FROM_SYSTEM |
+        FORMAT_MESSAGE_IGNORE_INSERTS,
+        NULL,
+        err,
+        MAKELANGID (LANG_NEUTRAL, SUBLANG_DEFAULT),
+        (LPTSTR) &errmsg,
+        0, NULL);
+#else
+  errmsg = strerror (err);
+#endif
+  snprintf (buf2, sizeof buf2, "%s: %s", buf1, errmsg);
+
+#ifdef WIN32
+  LocalFree (errmsg);
+#endif
 
   send_error (buf2);
 }
-- 
1.6.5.2

-- 
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 f982a590ba92d706425be1bdd40eefb6a61c5b29 Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones at redhat.com>
Date: Fri, 27 Nov 2009 13:53:31 +0000
Subject: [PATCH 2/9] daemon error handling: Remove direct access to errno.

---
 daemon/dir.c      |    4 ++--
 daemon/glob.c     |    6 ++++--
 daemon/guestfsd.c |    2 +-
 daemon/inotify.c  |    2 +-
 daemon/realpath.c |    7 +++----
 daemon/tar.c      |   46 ++++++++++++++++++++--------------------------
 daemon/upload.c   |   18 ++++++++----------
 7 files changed, 39 insertions(+), 46 deletions(-)

diff --git a/daemon/dir.c b/daemon/dir.c
index a5076b1..ecb9c79 100644
--- a/daemon/dir.c
+++ b/daemon/dir.c
@@ -127,7 +127,7 @@ recursive_mkdir (const char *path)
  again:
   r = mkdir (path, 0777);
   if (r == -1) {
-    if (errno == EEXIST) {	/* Something exists here, might not be a dir. */
+    if (get_errno() == EEXIST) { /* Something exists here, might not be a dir.
*/
       r = lstat (path, &buf);
       if (r == -1) return -1;
       if (!S_ISDIR (buf.st_mode)) {
@@ -137,7 +137,7 @@ recursive_mkdir (const char *path)
       return 0;			/* OK - directory exists here already. */
     }
 
-    if (!loop && errno == ENOENT) {
+    if (!loop && get_errno() == ENOENT) {
       loop = 1;			/* Stops it looping forever. */
 
       /* If we're at the root, and we failed, just give up. */
diff --git a/daemon/glob.c b/daemon/glob.c
index 4fe76f3..a330534 100644
--- a/daemon/glob.c
+++ b/daemon/glob.c
@@ -45,8 +45,10 @@ do_glob_expand (const char *pattern)
   }
 
   if (r != 0) {
-    if (errno != 0)
-      reply_with_perror ("glob: %s", pattern);
+    errtype err = get_error ();
+
+    if (err)
+      reply_with_perror_with_err (err, "glob: %s", pattern);
     else
       reply_with_error ("glob failed: %s", pattern);
     return NULL;
diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
index bb972f2..c3af8c5 100644
--- a/daemon/guestfsd.c
+++ b/daemon/guestfsd.c
@@ -1021,7 +1021,7 @@ device_name_translation (char *device, const char *func)
     return 0;
   }
 
-  if (errno != ENXIO && errno != ENOENT) {
+  if (get_errno() != ENXIO && get_errno() != ENOENT) {
   error:
     reply_with_perror ("%s: %s", func, device);
     return -1;
diff --git a/daemon/inotify.c b/daemon/inotify.c
index 24ce76e..749b8b7 100644
--- a/daemon/inotify.c
+++ b/daemon/inotify.c
@@ -224,7 +224,7 @@ do_inotify_read (void)
     r = read (inotify_fd, inotify_buf + inotify_posn,
               sizeof (inotify_buf) - inotify_posn);
     if (r == -1) {
-      if (errno == EWOULDBLOCK || errno == EAGAIN) /* End of list. */
+      if (get_errno() == EWOULDBLOCK || get_errno() == EAGAIN) /* End of list.
*/
         break;
       reply_with_perror ("read");
       goto error;
diff --git a/daemon/realpath.c b/daemon/realpath.c
index e6c81ef..f49e0c5 100644
--- a/daemon/realpath.c
+++ b/daemon/realpath.c
@@ -165,14 +165,13 @@ do_case_sensitive_path (const char *path)
 
     /* Is it a directory?  Try going into it. */
     fd2 = openat (fd_cwd, d->d_name, O_RDONLY | O_DIRECTORY);
-    int err = errno;
+    int err = get_error ();
     close (fd_cwd);
     fd_cwd = fd2;
-    errno = err;
     if (fd_cwd == -1) {
       /* ENOTDIR is OK provided we've reached the end of the path. */
-      if (errno != ENOTDIR) {
-        reply_with_perror ("openat: %s", d->d_name);
+      if (get_errno() != ENOTDIR) {
+        reply_with_perror_with_err (err, "openat: %s", d->d_name);
         goto error;
       }
 
diff --git a/daemon/tar.c b/daemon/tar.c
index c3bdcf7..56d291d 100644
--- a/daemon/tar.c
+++ b/daemon/tar.c
@@ -38,7 +38,8 @@ fwrite_cb (void *fp_ptr, const void *buf, int len)
 int
 do_tar_in (const char *dir)
 {
-  int err, r;
+  errtype err;
+  int r;
   FILE *fp;
   char *cmd;
 
@@ -50,10 +51,9 @@ do_tar_in (const char *dir)
 
   /* "tar -C /sysroot%s -xf -" but we have to quote the dir. */
   if (asprintf_nowarn (&cmd, "tar -C %R -xf -", dir) == -1) {
-    err = errno;
+    err = get_error ();
     cancel_receive ();
-    errno = err;
-    reply_with_perror ("asprintf");
+    reply_with_perror_with_err (err, "asprintf");
     return -1;
   }
 
@@ -62,10 +62,9 @@ do_tar_in (const char *dir)
 
   fp = popen (cmd, "w");
   if (fp == NULL) {
-    err = errno;
+    err = get_error ();
     cancel_receive ();
-    errno = err;
-    reply_with_perror ("%s", cmd);
+    reply_with_perror_with_err (err, "%s", cmd);
     free (cmd);
     return -1;
   }
@@ -73,10 +72,9 @@ do_tar_in (const char *dir)
 
   r = receive_file (fwrite_cb, &fp);
   if (r == -1) {		/* write error */
-    err = errno;
+    err = get_error ();
     cancel_receive ();
-    errno = err;
-    reply_with_perror ("write: %s", dir);
+    reply_with_perror_with_err (err, "write: %s", dir);
     pclose (fp);
     return -1;
   }
@@ -87,10 +85,9 @@ do_tar_in (const char *dir)
   }
 
   if (pclose (fp) != 0) {
-    err = errno;
+    err = get_error ();
     cancel_receive ();
-    errno = err;
-    reply_with_perror ("pclose: %s", dir);
+    reply_with_perror_with_err (err, "pclose: %s", dir);
     return -1;
   }
 
@@ -159,7 +156,8 @@ do_tar_out (const char *dir)
 int
 do_tgz_in (const char *dir)
 {
-  int err, r;
+  errtype err;
+  int r;
   FILE *fp;
   char *cmd;
 
@@ -171,10 +169,9 @@ do_tgz_in (const char *dir)
 
   /* "tar -C /sysroot%s -zxf -" but we have to quote the dir. */
   if (asprintf_nowarn (&cmd, "tar -C %R -zxf -", dir) == -1) {
-    err = errno;
+    err = get_error ();
     cancel_receive ();
-    errno = err;
-    reply_with_perror ("asprintf");
+    reply_with_perror_with_err (err, "asprintf");
     return -1;
   }
 
@@ -183,10 +180,9 @@ do_tgz_in (const char *dir)
 
   fp = popen (cmd, "w");
   if (fp == NULL) {
-    err = errno;
+    err = get_error ();
     cancel_receive ();
-    errno = err;
-    reply_with_perror ("%s", cmd);
+    reply_with_perror_with_err (err, "%s", cmd);
     free (cmd);
     return -1;
   }
@@ -194,10 +190,9 @@ do_tgz_in (const char *dir)
 
   r = receive_file (fwrite_cb, &fp);
   if (r == -1) {		/* write error */
-    err = errno;
+    err = get_error ();
     cancel_receive ();
-    errno = err;
-    reply_with_perror ("write: %s", dir);
+    reply_with_perror_with_err (err, "write: %s", dir);
     pclose (fp);
     return -1;
   }
@@ -208,10 +203,9 @@ do_tgz_in (const char *dir)
   }
 
   if (pclose (fp) != 0) {
-    err = errno;
+    err = get_error ();
     cancel_receive ();
-    errno = err;
-    reply_with_perror ("pclose: %s", dir);
+    reply_with_perror_with_err (err, "pclose: %s", dir);
     return -1;
   }
 
diff --git a/daemon/upload.c b/daemon/upload.c
index fdb8654..04aa21d 100644
--- a/daemon/upload.c
+++ b/daemon/upload.c
@@ -38,7 +38,8 @@ write_cb (void *fd_ptr, const void *buf, int len)
 int
 do_upload (const char *filename)
 {
-  int err, fd, r, is_dev;
+  errtype err;
+  int fd, r, is_dev;
 
   is_dev = STRPREFIX (filename, "/dev/");
   if (!is_dev) {
@@ -53,19 +54,17 @@ do_upload (const char *filename)
   fd = open (filename, O_WRONLY|O_CREAT|O_TRUNC|O_NOCTTY, 0666);
   if (!is_dev) CHROOT_OUT;
   if (fd == -1) {
-    err = errno;
+    err = get_error ();
     cancel_receive ();
-    errno = err;
-    reply_with_perror ("%s", filename);
+    reply_with_perror_with_err (err, "%s", filename);
     return -1;
   }
 
   r = receive_file (write_cb, &fd);
   if (r == -1) {		/* write error */
-    err = errno;
+    err = get_error ();
     cancel_receive ();
-    errno = err;
-    reply_with_perror ("write: %s", filename);
+    reply_with_perror_with_err (err, "write: %s", filename);
     close (fd);
     return -1;
   }
@@ -76,10 +75,9 @@ do_upload (const char *filename)
   }
 
   if (close (fd) == -1) {
-    err = errno;
+    err = get_error ();
     cancel_receive ();
-    errno = err;
-    reply_with_perror ("close: %s", filename);
+    reply_with_perror_with_err (err, "close: %s", filename);
     return -1;
   }
 
-- 
1.6.5.2

-- 
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 af2847f7da0810f09912e9b77caab9c834ef7fa2 Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones at redhat.com>
Date: Fri, 27 Nov 2009 13:50:28 +0000
Subject: [PATCH 3/9] daemon error handling: readdir code

Where readdir sets errno = 0 to ensure correct error handling, we
replace this with a call to clear_error ().
---
 daemon/devsparts.c |    8 ++++----
 daemon/realpath.c  |    4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/daemon/devsparts.c b/daemon/devsparts.c
index 60e7aa8..6b20503 100644
--- a/daemon/devsparts.c
+++ b/daemon/devsparts.c
@@ -49,7 +49,7 @@ foreach_block_device (block_dev_func_t func)
   }
 
   while(1) {
-    errno = 0;
+    clear_error ();
     struct dirent *d = readdir(dir);
     if(NULL == d) break;
 
@@ -80,7 +80,7 @@ foreach_block_device (block_dev_func_t func)
   }
 
   /* Check readdir didn't fail */
-  if(0 != errno) {
+  if (0 != get_error ()) {
     reply_with_perror ("readdir: /sys/block");
     free_stringslen(r, size);
     return NULL;
@@ -150,7 +150,7 @@ add_partitions(const char *device,
   /* Look in /sys/block/<device>/ for entries starting with
<device>
    * e.g. /sys/block/sda/sda1
    */
-  errno = 0;
+  clear_error ();
   struct dirent *d;
   while ((d = readdir (dir)) != NULL) {
     if (STREQLEN (d->d_name, device, strlen (device))) {
@@ -165,7 +165,7 @@ add_partitions(const char *device,
   }
 
   /* Check if readdir failed */
-  if(0 != errno) {
+  if (0 != get_error ()) {
       reply_with_perror ("readdir: %s", devdir);
       free_stringslen(*r, *size);
       return -1;
diff --git a/daemon/realpath.c b/daemon/realpath.c
index f49e0c5..1a6bc5f 100644
--- a/daemon/realpath.c
+++ b/daemon/realpath.c
@@ -129,13 +129,13 @@ do_case_sensitive_path (const char *path)
 
     struct dirent *d = NULL;
 
-    errno = 0;
+    clear_error ();
     while ((d = readdir (dir)) != NULL) {
       if (STRCASEEQ (d->d_name, name))
         break;
     }
 
-    if (d == NULL && errno != 0) {
+    if (d == NULL && get_error () != 0) {
       reply_with_perror ("readdir");
       goto error;
     }
-- 
1.6.5.2

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://et.redhat.com/~rjones/libguestfs/
See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html
-------------- next part --------------
>From c5d3eff7a052f0b4eac0290f0e928e8a3fea7927 Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones at redhat.com>
Date: Fri, 27 Nov 2009 13:51:46 +0000
Subject: [PATCH 4/9] daemon error handling: fix is_dir and is_file calls for
Windows.

This fixes the code so it should work on Windows.
---
 daemon/dir.c  |    4 +++-
 daemon/file.c |    4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/daemon/dir.c b/daemon/dir.c
index ecb9c79..47000da 100644
--- a/daemon/dir.c
+++ b/daemon/dir.c
@@ -190,11 +190,13 @@ do_is_dir (const char *path)
   CHROOT_OUT;
 
   if (r == -1) {
-    if (errno != ENOENT && errno != ENOTDIR) {
+#ifndef WIN32
+    if (get_errno() != ENOENT && get_errno() != ENOTDIR) {
       reply_with_perror ("stat: %s", path);
       return -1;
     }
     else
+#endif
       return 0;			/* Not a directory. */
   }
 
diff --git a/daemon/file.c b/daemon/file.c
index 0b50eeb..e6fe76c 100644
--- a/daemon/file.c
+++ b/daemon/file.c
@@ -267,11 +267,13 @@ do_is_file (const char *path)
   CHROOT_OUT;
 
   if (r == -1) {
-    if (errno != ENOENT && errno != ENOTDIR) {
+#ifndef WIN32
+    if (get_errno() != ENOENT && get_errno() != ENOTDIR) {
       reply_with_perror ("stat: %s", path);
       return -1;
     }
     else
+#endif
       return 0;			/* Not a file. */
   }
 
-- 
1.6.5.2

-- 
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
-------------- next part --------------
>From 6422809e06cf8e7044abe9afa2bfa9aea76b2ba1 Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones at redhat.com>
Date: Fri, 27 Nov 2009 13:54:11 +0000
Subject: [PATCH 5/9] daemon error handling: fix case_sensitive_path for Windows.

---
 daemon/realpath.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/daemon/realpath.c b/daemon/realpath.c
index 1a6bc5f..3f67c1b 100644
--- a/daemon/realpath.c
+++ b/daemon/realpath.c
@@ -69,6 +69,7 @@ do_realpath (const char *path)
 char *
 do_case_sensitive_path (const char *path)
 {
+#ifndef WIN32
   char ret[PATH_MAX+1] = "/";
   size_t next = 1;
   int fd_cwd;
@@ -195,4 +196,15 @@ do_case_sensitive_path (const char *path)
  error:
   close (fd_cwd);
   return NULL;
+#else /* WIN32 */
+  /* On Win32 paths are always handled case insensitively, so there is
+   * no need for this function to modify the path in any way.
+   */
+  char *ret = strdup (path);
+  if (ret == NULL) {
+    reply_with_perror ("strdup");
+    return NULL;
+  }
+  return ret;
+#endif /* WIN32 */
 }
-- 
1.6.5.2

-- 
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 e2b84e1f754fda37223dc9da152a9490cf6f9ea4 Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones at redhat.com>
Date: Fri, 27 Nov 2009 13:48:06 +0000
Subject: [PATCH 6/9] daemon error handling: clear errors before all functions

This isn't strictly necessary, but doesn't do any harm and
may provide more reliable error messages in the case where
our error handling code is wrong.
---
 daemon/proto.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/daemon/proto.c b/daemon/proto.c
index 7c923c8..23748a4 100644
--- a/daemon/proto.c
+++ b/daemon/proto.c
@@ -143,6 +143,7 @@ main_loop (int _sock)
     /* Now start to process this message. */
     proc_nr = hdr.proc;
     serial = hdr.serial;
+    clear_error (); /* Make sure errno is clear before calling the function. */
     dispatch_incoming_message (&xdr);
     /* Note that dispatch_incoming_message will also send a reply. */
 
-- 
1.6.5.2

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://et.redhat.com/~rjones/virt-df/
-------------- next part --------------
>From 491625c773d4dcb7581669fda51ddaafe47db067 Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones at redhat.com>
Date: Fri, 27 Nov 2009 14:14:21 +0000
Subject: [PATCH 7/9] daemon error handling: recursive_mkdir shouldn't need
to set errno.

---
 daemon/dir.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/daemon/dir.c b/daemon/dir.c
index 47000da..0d052c6 100644
--- a/daemon/dir.c
+++ b/daemon/dir.c
@@ -116,6 +116,11 @@ do_mkdir_mode (const char *path, int mode)
   return 0;
 }
 
+/* Returns:
+ * 0  if everything was OK,
+ * -1 for a general error (use perror/get_error to display it),
+ * -2 if an existing path element was not a directory.
+ */
 static int
 recursive_mkdir (const char *path)
 {
@@ -130,10 +135,7 @@ recursive_mkdir (const char *path)
     if (get_errno() == EEXIST) { /* Something exists here, might not be a dir.
*/
       r = lstat (path, &buf);
       if (r == -1) return -1;
-      if (!S_ISDIR (buf.st_mode)) {
-        errno = ENOTDIR;
-        return -1;
-      }
+      if (!S_ISDIR (buf.st_mode)) return -2;
       return 0;			/* OK - directory exists here already. */
     }
 
@@ -153,7 +155,7 @@ recursive_mkdir (const char *path)
       r = recursive_mkdir (ppath);
       free (ppath);
 
-      if (r == -1) return -1;
+      if (r != 0) return r;
 
       goto again;
     } else	  /* Failed for some other reason, so return error. */
@@ -175,6 +177,10 @@ do_mkdir_p (const char *path)
     reply_with_perror ("mkdir -p: %s", path);
     return -1;
   }
+  if (r == -2) {
+    reply_with_error ("mkdir -p: %s: a path element was not a
directory", path);
+    return -1;
+  }
 
   return 0;
 }
-- 
1.6.5.2

-- 
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 adda910f62b819d324de08abcab7587dfa2ae621 Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones at redhat.com>
Date: Fri, 27 Nov 2009 15:16:10 +0000
Subject: [PATCH 8/9] daemon error handling: Change CHROOT_* macros handling of
errno.

Previously the CHROOT_* macros were documented as preserving
errno.  I don't think this was such a good idea, and using the
get_error () function we can replace this code:

  errtype err;

  CHROOT_IN;
  r = some_syscall ();
  err = get_error ();
  CHROOT_OUT;

  if (r == -1) {
    reply_with_perror_with_err (err, "some_syscall");
    return -1;
  }
---
 daemon/daemon.h    |    6 +-----
 daemon/dir.c       |   25 ++++++++++++++++++-------
 daemon/fallocate.c |    4 +++-
 daemon/file.c      |   46 ++++++++++++++++++++++++++++++++++------------
 daemon/glob.c      |    4 ++--
 daemon/link.c      |    8 ++++++--
 daemon/ls.c        |    4 +++-
 daemon/mknod.c     |    4 +++-
 daemon/mount.c     |    8 ++++++--
 daemon/readdir.c   |    4 +++-
 daemon/realpath.c  |    5 ++++-
 daemon/stat.c      |   12 +++++++++---
 daemon/statvfs.c   |    4 +++-
 daemon/truncate.c  |    4 +++-
 daemon/upload.c    |    6 ++++--
 daemon/utimens.c   |    4 +++-
 daemon/xattr.c     |   28 ++++++++++++++++++++--------
 17 files changed, 125 insertions(+), 51 deletions(-)

diff --git a/daemon/daemon.h b/daemon/daemon.h
index ea1e66f..96d08dd 100644
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -274,22 +274,18 @@ extern void reply (xdrproc_t xdrp, char *ret);
  * (2) You must not change directory!  cwd must always be "/",
otherwise
  *     we can't escape our own chroot.
  * (3) All paths specified must be absolute.
- * (4) Neither macro affects errno.
+ * (4) Caution: these macros might modify errno.
  */
 #ifndef WIN32
 #define CHROOT_IN				\
   do {						\
-    int __old_errno = errno;			\
     if (chroot (sysroot) == -1)			\
       perror ("CHROOT_IN: sysroot");		\
-    errno = __old_errno;			\
   } while (0)
 #define CHROOT_OUT				\
   do {						\
-    int __old_errno = errno;			\
     if (chroot (".") == -1)			\
       perror ("CHROOT_OUT: .");			\
-    errno = __old_errno;			\
   } while (0)
 #else /* WIN32 */
 # define CHROOT_IN fprintf (stderr, "CHROOT_IN: Win32: cannot
chroot\n")
diff --git a/daemon/dir.c b/daemon/dir.c
index 0d052c6..072dcf5 100644
--- a/daemon/dir.c
+++ b/daemon/dir.c
@@ -33,13 +33,15 @@ int
 do_rmdir (const char *path)
 {
   int r;
+  errtype err;
 
   CHROOT_IN;
   r = rmdir (path);
+  err = get_error ();
   CHROOT_OUT;
 
   if (r == -1) {
-    reply_with_perror ("rmdir: %s", path);
+    reply_with_perror_with_err (err, "rmdir: %s", path);
     return -1;
   }
 
@@ -86,13 +88,15 @@ int
 do_mkdir (const char *path)
 {
   int r;
+  errtype err;
 
   CHROOT_IN;
   r = mkdir (path, 0777);
+  err = get_error ();
   CHROOT_OUT;
 
   if (r == -1) {
-    reply_with_perror ("mkdir: %s", path);
+    reply_with_perror_with_err (err, "mkdir: %s", path);
     return -1;
   }
 
@@ -103,13 +107,15 @@ int
 do_mkdir_mode (const char *path, int mode)
 {
   int r;
+  errtype err;
 
   CHROOT_IN;
   r = mkdir (path, mode);
+  err = get_error ();
   CHROOT_OUT;
 
   if (r == -1) {
-    reply_with_perror ("mkdir_mode: %s", path);
+    reply_with_perror_with_err (err, "mkdir_mode: %s", path);
     return -1;
   }
 
@@ -168,13 +174,15 @@ int
 do_mkdir_p (const char *path)
 {
   int r;
+  errtype err;
 
   CHROOT_IN;
   r = recursive_mkdir (path);
+  err = get_error ();
   CHROOT_OUT;
 
   if (r == -1) {
-    reply_with_perror ("mkdir -p: %s", path);
+    reply_with_perror_with_err (err, "mkdir -p: %s", path);
     return -1;
   }
   if (r == -2) {
@@ -190,15 +198,17 @@ do_is_dir (const char *path)
 {
   int r;
   struct stat buf;
+  errtype err;
 
   CHROOT_IN;
   r = lstat (path, &buf);
+  err = get_error ();
   CHROOT_OUT;
 
   if (r == -1) {
 #ifndef WIN32
-    if (get_errno() != ENOENT && get_errno() != ENOTDIR) {
-      reply_with_perror ("stat: %s", path);
+    if (err != ENOENT && err != ENOTDIR) {
+      reply_with_perror_with_err (err, "stat: %s", path);
       return -1;
     }
     else
@@ -220,10 +230,11 @@ do_mkdtemp (const char *template)
 
   CHROOT_IN;
   char *r = mkdtemp (writable);
+  errtype err = get_error ();
   CHROOT_OUT;
 
   if (r == NULL) {
-    reply_with_perror ("mkdtemp: %s", template);
+    reply_with_perror_with_err (err, "mkdtemp: %s", template);
     free (writable);
   }
 
diff --git a/daemon/fallocate.c b/daemon/fallocate.c
index 1800292..c938a65 100644
--- a/daemon/fallocate.c
+++ b/daemon/fallocate.c
@@ -31,12 +31,14 @@ int
 do_fallocate (const char *path, int len)
 {
   int fd;
+  errtype err;
 
   CHROOT_IN;
   fd = open (path, O_WRONLY | O_CREAT | O_TRUNC | O_NOCTTY, 0666);
+  err = get_error ();
   CHROOT_OUT;
   if (fd == -1) {
-    reply_with_perror ("failed to open %s", path);
+    reply_with_perror_with_err (err, "failed to open %s", path);
     return -1;
   }
 
diff --git a/daemon/file.c b/daemon/file.c
index e6fe76c..bbb6426 100644
--- a/daemon/file.c
+++ b/daemon/file.c
@@ -34,13 +34,15 @@ do_touch (const char *path)
 {
   int fd;
   int r;
+  errtype err;
 
   CHROOT_IN;
   fd = open (path, O_WRONLY | O_CREAT | O_NOCTTY, 0666);
+  err = get_error ();
   CHROOT_OUT;
 
   if (fd == -1) {
-    reply_with_perror ("open: %s", path);
+    reply_with_perror_with_err (err, "open: %s", path);
     return -1;
   }
 
@@ -65,13 +67,15 @@ do_cat (const char *path)
   int fd;
   int alloc, size, r, max;
   char *buf, *buf2;
+  errtype err;
 
   CHROOT_IN;
   fd = open (path, O_RDONLY);
+  err = get_error ();
   CHROOT_OUT;
 
   if (fd == -1) {
-    reply_with_perror ("open: %s", path);
+    reply_with_perror_with_err (err, "open: %s", path);
     return NULL;
   }
 
@@ -136,13 +140,15 @@ do_read_lines (const char *path)
   char *line = NULL;
   size_t len = 0;
   ssize_t n;
+  errtype err;
 
   CHROOT_IN;
   fp = fopen (path, "r");
+  err = get_error ();
   CHROOT_OUT;
 
   if (!fp) {
-    reply_with_perror ("fopen: %s", path);
+    reply_with_perror_with_err (err, "fopen: %s", path);
     return NULL;
   }
 
@@ -180,13 +186,15 @@ int
 do_rm (const char *path)
 {
   int r;
+  errtype err;
 
   CHROOT_IN;
   r = unlink (path);
+  err = get_error ();
   CHROOT_OUT;
 
   if (r == -1) {
-    reply_with_perror ("unlink: %s", path);
+    reply_with_perror_with_err (err, "unlink: %s", path);
     return -1;
   }
 
@@ -197,13 +205,15 @@ int
 do_chmod (int mode, const char *path)
 {
   int r;
+  errtype err;
 
   CHROOT_IN;
   r = chmod (path, mode);
+  err = get_error ();
   CHROOT_OUT;
 
   if (r == -1) {
-    reply_with_perror ("chmod: %s: 0%o", path, mode);
+    reply_with_perror_with_err (err, "chmod: %s: 0%o", path, mode);
     return -1;
   }
 
@@ -214,13 +224,15 @@ int
 do_chown (int owner, int group, const char *path)
 {
   int r;
+  errtype err;
 
   CHROOT_IN;
   r = chown (path, owner, group);
+  err = get_error ();
   CHROOT_OUT;
 
   if (r == -1) {
-    reply_with_perror ("chown: %s: %d.%d", path, owner, group);
+    reply_with_perror_with_err (err, "chown: %s: %d.%d", path, owner,
group);
     return -1;
   }
 
@@ -231,13 +243,15 @@ int
 do_lchown (int owner, int group, const char *path)
 {
   int r;
+  errtype err;
 
   CHROOT_IN;
   r = lchown (path, owner, group);
+  err = get_error ();
   CHROOT_OUT;
 
   if (r == -1) {
-    reply_with_perror ("lchown: %s: %d.%d", path, owner, group);
+    reply_with_perror_with_err (err, "lchown: %s: %d.%d", path,
owner, group);
     return -1;
   }
 
@@ -261,15 +275,17 @@ do_is_file (const char *path)
 {
   int r;
   struct stat buf;
+  errtype err;
 
   CHROOT_IN;
   r = lstat (path, &buf);
+  err = get_error ();
   CHROOT_OUT;
 
   if (r == -1) {
 #ifndef WIN32
-    if (get_errno() != ENOENT && get_errno() != ENOTDIR) {
-      reply_with_perror ("stat: %s", path);
+    if (err != ENOENT && err != ENOTDIR) {
+      reply_with_perror_with_err (err, "stat: %s", path);
       return -1;
     }
     else
@@ -284,16 +300,18 @@ int
 do_write_file (const char *path, const char *content, int size)
 {
   int fd;
+  errtype err;
 
   if (size == 0)
     size = strlen (content);
 
   CHROOT_IN;
   fd = open (path, O_WRONLY | O_TRUNC | O_CREAT | O_NOCTTY, 0666);
+  err = get_error ();
   CHROOT_OUT;
 
   if (fd == -1) {
-    reply_with_perror ("open: %s", path);
+    reply_with_perror_with_err (err, "open: %s", path);
     return -1;
   }
 
@@ -317,13 +335,15 @@ do_read_file (const char *path, size_t *size_r)
   int fd;
   struct stat statbuf;
   char *r;
+  errtype err;
 
   CHROOT_IN;
   fd = open (path, O_RDONLY);
+  err = get_error ();
   CHROOT_OUT;
 
   if (fd == -1) {
-    reply_with_perror ("open: %s", path);
+    reply_with_perror_with_err (err, "open: %s", path);
     return NULL;
   }
 
@@ -373,6 +393,7 @@ do_pread (const char *path, int count, int64_t offset,
size_t *size_r)
   int fd;
   ssize_t r;
   char *buf;
+  errtype err;
 
   /* The actual limit on messages is smaller than this.  This check
    * just limits the amount of memory we'll try and allocate in the
@@ -386,10 +407,11 @@ do_pread (const char *path, int count, int64_t offset,
size_t *size_r)
 
   CHROOT_IN;
   fd = open (path, O_RDONLY);
+  err = get_error ();
   CHROOT_OUT;
 
   if (fd == -1) {
-    reply_with_perror ("open: %s", path);
+    reply_with_perror_with_err (err, "open: %s", path);
     return NULL;
   }
 
diff --git a/daemon/glob.c b/daemon/glob.c
index a330534..b0b56b2 100644
--- a/daemon/glob.c
+++ b/daemon/glob.c
@@ -30,10 +30,12 @@ do_glob_expand (const char *pattern)
 {
   int r;
   glob_t buf;
+  errtype err;
 
   /* glob(3) in glibc never calls chdir, so this seems to be safe: */
   CHROOT_IN;
   r = glob (pattern, GLOB_MARK|GLOB_BRACE, NULL, &buf);
+  err = get_error ();
   CHROOT_OUT;
 
   if (r == GLOB_NOMATCH) {	/* Return an empty list instead of an error. */
@@ -45,8 +47,6 @@ do_glob_expand (const char *pattern)
   }
 
   if (r != 0) {
-    errtype err = get_error ();
-
     if (err)
       reply_with_perror_with_err (err, "glob: %s", pattern);
     else
diff --git a/daemon/link.c b/daemon/link.c
index 5ea0d39..97f6ac0 100644
--- a/daemon/link.c
+++ b/daemon/link.c
@@ -34,12 +34,14 @@ do_readlink (const char *path)
   ssize_t r;
   char *ret;
   char link[PATH_MAX];
+  errtype err;
 
   CHROOT_IN;
   r = readlink (path, link, sizeof link);
+  err = get_error ();
   CHROOT_OUT;
   if (r == -1) {
-    reply_with_perror ("readlink");
+    reply_with_perror_with_err (err, "readlink");
     return NULL;
   }
 
@@ -62,13 +64,15 @@ do_readlinklist (const char *path, char *const *names)
   const char *str;
   char **ret = NULL;
   int size = 0, alloc = 0;
+  errtype err;
 
   CHROOT_IN;
   fd_cwd = open (path, O_RDONLY | O_DIRECTORY);
+  err = get_error ();
   CHROOT_OUT;
 
   if (fd_cwd == -1) {
-    reply_with_perror ("readlinklist: %s", path);
+    reply_with_perror_with_err (err, "readlinklist: %s", path);
     return NULL;
   }
 
diff --git a/daemon/ls.c b/daemon/ls.c
index 0af2356..a15ce62 100644
--- a/daemon/ls.c
+++ b/daemon/ls.c
@@ -36,13 +36,15 @@ do_ls (const char *path)
   int size = 0, alloc = 0;
   DIR *dir;
   struct dirent *d;
+  errtype err;
 
   CHROOT_IN;
   dir = opendir (path);
+  err = get_error ();
   CHROOT_OUT;
 
   if (!dir) {
-    reply_with_perror ("opendir: %s", path);
+    reply_with_perror_with_err (err, "opendir: %s", path);
     return NULL;
   }
 
diff --git a/daemon/mknod.c b/daemon/mknod.c
index 46a1839..0fb1962 100644
--- a/daemon/mknod.c
+++ b/daemon/mknod.c
@@ -50,13 +50,15 @@ do_mknod (int mode, int devmajor, int devminor, const char
*path)
 {
 #ifdef HAVE_MKNOD
   int r;
+  errtype err;
 
   CHROOT_IN;
   r = mknod (path, mode, makedev (devmajor, devminor));
+  err = get_error ();
   CHROOT_OUT;
 
   if (r == -1) {
-    reply_with_perror ("mknod: %s", path);
+    reply_with_perror_with_err (err, "mknod: %s", path);
     return -1;
   }
 
diff --git a/daemon/mount.c b/daemon/mount.c
index 49a0eab..f447380 100644
--- a/daemon/mount.c
+++ b/daemon/mount.c
@@ -354,16 +354,18 @@ int
 do_mkmountpoint (const char *path)
 {
   int r;
+  errtype err;
 
   /* NEED_ROOT (return -1); - we don't want this test for this call. */
   ABS_PATH (path, return -1);
 
   CHROOT_IN;
   r = mkdir (path, 0777);
+  err = get_error ();
   CHROOT_OUT;
 
   if (r == -1) {
-    reply_with_perror ("mkmountpoint: %s", path);
+    reply_with_perror_with_err (err, "mkmountpoint: %s", path);
     return -1;
   }
 
@@ -379,16 +381,18 @@ int
 do_rmmountpoint (const char *path)
 {
   int r;
+  errtype err;
 
   /* NEED_ROOT (return -1); - we don't want this test for this call. */
   ABS_PATH (path, return -1);
 
   CHROOT_IN;
   r = rmdir (path);
+  err = get_error ();
   CHROOT_OUT;
 
   if (r == -1) {
-    reply_with_perror ("rmmountpoint: %s", path);
+    reply_with_perror_with_err (err, "rmmountpoint: %s", path);
     return -1;
   }
 
diff --git a/daemon/readdir.c b/daemon/readdir.c
index 876041e..0d72912 100644
--- a/daemon/readdir.c
+++ b/daemon/readdir.c
@@ -35,6 +35,7 @@ do_readdir (const char *path)
   DIR *dir;
   struct dirent *d;
   int i;
+  errtype err;
 
   ret = malloc (sizeof *ret);
   if (ret == NULL) {
@@ -47,10 +48,11 @@ do_readdir (const char *path)
 
   CHROOT_IN;
   dir = opendir (path);
+  err = get_error ();
   CHROOT_OUT;
 
   if (dir == NULL) {
-    reply_with_perror ("opendir: %s", path);
+    reply_with_perror_with_err (err, "opendir: %s", path);
     free (ret);
     return NULL;
   }
diff --git a/daemon/realpath.c b/daemon/realpath.c
index 3f67c1b..2760aff 100644
--- a/daemon/realpath.c
+++ b/daemon/realpath.c
@@ -51,12 +51,15 @@ do_realpath (const char *path)
 {
 #ifdef HAVE_REALPATH
   char *ret;
+  errtype err;
 
   CHROOT_IN;
   ret = realpath (path, NULL);
+  err = get_error ();
   CHROOT_OUT;
+
   if (ret == NULL) {
-    reply_with_perror ("realpath");
+    reply_with_perror_with_err (err, "realpath");
     return NULL;
   }
 
diff --git a/daemon/stat.c b/daemon/stat.c
index 20f4b70..c14d15e 100644
--- a/daemon/stat.c
+++ b/daemon/stat.c
@@ -36,13 +36,15 @@ do_stat (const char *path)
   int r;
   guestfs_int_stat *ret;
   struct stat statbuf;
+  errtype err;
 
   CHROOT_IN;
   r = stat (path, &statbuf);
+  err = get_error ();
   CHROOT_OUT;
 
   if (r == -1) {
-    reply_with_perror ("stat");
+    reply_with_perror_with_err (err, "stat");
     return NULL;
   }
 
@@ -83,13 +85,15 @@ do_lstat (const char *path)
   int r;
   guestfs_int_stat *ret;
   struct stat statbuf;
+  errtype err;
 
   CHROOT_IN;
   r = lstat (path, &statbuf);
+  err = get_error ();
   CHROOT_OUT;
 
   if (r == -1) {
-    reply_with_perror ("stat");
+    reply_with_perror_with_err (err, "stat");
     return NULL;
   }
 
@@ -130,6 +134,7 @@ do_lstatlist (const char *path, char *const *names)
   int path_fd;
   guestfs_int_stat_list *ret;
   size_t i, nr_names;
+  errtype err;
 
   nr_names = count_strings (names);
 
@@ -148,10 +153,11 @@ do_lstatlist (const char *path, char *const *names)
 
   CHROOT_IN;
   path_fd = open (path, O_RDONLY | O_DIRECTORY);
+  err = get_error ();
   CHROOT_OUT;
 
   if (path_fd == -1) {
-    reply_with_perror ("lstatlist: %s", path);
+    reply_with_perror_with_err (err, "lstatlist: %s", path);
     free (ret->guestfs_int_stat_list_val);
     free (ret);
     return NULL;
diff --git a/daemon/statvfs.c b/daemon/statvfs.c
index e71b19a..4fdcc24 100644
--- a/daemon/statvfs.c
+++ b/daemon/statvfs.c
@@ -46,13 +46,15 @@ do_statvfs (const char *path)
   int r;
   guestfs_int_statvfs *ret;
   struct statvfs statbuf;
+  errtype err;
 
   CHROOT_IN;
   r = statvfs (path, &statbuf);
+  err = get_error ();
   CHROOT_OUT;
 
   if (r == -1) {
-    reply_with_perror ("statvfs");
+    reply_with_perror_with_err (err, "statvfs");
     return NULL;
   }
 
diff --git a/daemon/truncate.c b/daemon/truncate.c
index c2da382..a0708b4 100644
--- a/daemon/truncate.c
+++ b/daemon/truncate.c
@@ -33,13 +33,15 @@ do_truncate_size (const char *path, int64_t size)
 {
   int fd;
   int r;
+  errtype err;
 
   CHROOT_IN;
   fd = open (path, O_WRONLY | O_NOCTTY);
+  err = get_error ();
   CHROOT_OUT;
 
   if (fd == -1) {
-    reply_with_perror ("open: %s", path);
+    reply_with_perror_with_err (err, "open: %s", path);
     return -1;
   }
 
diff --git a/daemon/upload.c b/daemon/upload.c
index 04aa21d..d4ae9d6 100644
--- a/daemon/upload.c
+++ b/daemon/upload.c
@@ -52,9 +52,9 @@ do_upload (const char *filename)
 
   if (!is_dev) CHROOT_IN;
   fd = open (filename, O_WRONLY|O_CREAT|O_TRUNC|O_NOCTTY, 0666);
+  err = get_error ();
   if (!is_dev) CHROOT_OUT;
   if (fd == -1) {
-    err = get_error ();
     cancel_receive ();
     reply_with_perror_with_err (err, "%s", filename);
     return -1;
@@ -90,14 +90,16 @@ do_download (const char *filename)
 {
   int fd, r, is_dev;
   char buf[GUESTFS_MAX_CHUNK_SIZE];
+  errtype err;
 
   is_dev = STRPREFIX (filename, "/dev/");
 
   if (!is_dev) CHROOT_IN;
   fd = open (filename, O_RDONLY);
+  err = get_error ();
   if (!is_dev) CHROOT_OUT;
   if (fd == -1) {
-    reply_with_perror ("%s", filename);
+    reply_with_perror_with_err (err, "%s", filename);
     return -1;
   }
 
diff --git a/daemon/utimens.c b/daemon/utimens.c
index e836b4e..02c31f0 100644
--- a/daemon/utimens.c
+++ b/daemon/utimens.c
@@ -35,13 +35,15 @@ do_utimens (const char *path,
 {
   int fd;
   int r;
+  errtype err;
 
   CHROOT_IN;
   fd = open (path, O_WRONLY | O_NOCTTY);
+  err = get_error ();
   CHROOT_OUT;
 
   if (fd == -1) {
-    reply_with_perror ("open: %s", path);
+    reply_with_perror_with_err (err, "open: %s", path);
     return -1;
   }
 
diff --git a/daemon/xattr.c b/daemon/xattr.c
index e58dc7e..77092c4 100644
--- a/daemon/xattr.c
+++ b/daemon/xattr.c
@@ -122,12 +122,14 @@ getxattrs (const char *path,
   char *buf = NULL;
   int i, j;
   guestfs_int_xattr_list *r = NULL;
+  errtype err;
 
   CHROOT_IN;
   len = listxattr (path, NULL, 0);
+  err = get_error ();
   CHROOT_OUT;
   if (len == -1) {
-    reply_with_perror ("listxattr");
+    reply_with_perror_with_err (err, "listxattr");
     goto error;
   }
 
@@ -139,9 +141,10 @@ getxattrs (const char *path,
 
   CHROOT_IN;
   len = listxattr (path, buf, len);
+  err = get_error ();
   CHROOT_OUT;
   if (len == -1) {
-    reply_with_perror ("listxattr");
+    reply_with_perror_with_err (err, "listxattr");
     goto error;
   }
 
@@ -168,9 +171,10 @@ getxattrs (const char *path,
   for (i = 0, j = 0; i < len; i += strlen (&buf[i]) + 1, ++j) {
     CHROOT_IN;
     vlen = getxattr (path, &buf[i], NULL, 0);
+    err = get_error ();
     CHROOT_OUT;
     if (vlen == -1) {
-      reply_with_perror ("getxattr");
+      reply_with_perror_with_err (err, "getxattr");
       goto error;
     }
 
@@ -188,9 +192,10 @@ getxattrs (const char *path,
     vlen = getxattr (path, &buf[i],
                      r->guestfs_int_xattr_list_val[j].attrval.attrval_val,
                      vlen);
+    err = get_error ();
     CHROOT_OUT;
     if (vlen == -1) {
-      reply_with_perror ("getxattr");
+      reply_with_perror_with_err (err, "getxattr");
       goto error;
     }
   }
@@ -221,12 +226,14 @@ _setxattr (const char *xattr, const char *val, int vallen,
const char *path,
                             const void *value, size_t size, int flags))
 {
   int r;
+  errtype err;
 
   CHROOT_IN;
   r = setxattr (path, xattr, val, vallen, 0);
+  err = get_error ();
   CHROOT_OUT;
   if (r == -1) {
-    reply_with_perror ("setxattr");
+    reply_with_perror_with_err (err, "setxattr");
     return -1;
   }
 
@@ -238,12 +245,14 @@ _removexattr (const char *xattr, const char *path,
               int (*removexattr) (const char *path, const char *name))
 {
   int r;
+  errtype err;
 
   CHROOT_IN;
   r = removexattr (path, xattr);
+  err = get_error ();
   CHROOT_OUT;
   if (r == -1) {
-    reply_with_perror ("removexattr");
+    reply_with_perror_with_err (err, "removexattr");
     return -1;
   }
 
@@ -264,6 +273,7 @@ do_lxattrlist (const char *path, char *const *names)
   size_t k, m, nr_attrs;
   ssize_t len, vlen;
   char *buf = NULL;
+  errtype err;
 
   if (path_len >= PATH_MAX) {
     reply_with_perror ("lxattrlist: path longer than PATH_MAX");
@@ -366,9 +376,10 @@ do_lxattrlist (const char *path, char *const *names)
     for (i = 0, j = 0; i < len; i += strlen (&buf[i]) + 1, ++j) {
       CHROOT_IN;
       vlen = lgetxattr (pathname, &buf[i], NULL, 0);
+      err = get_error ();
       CHROOT_OUT;
       if (vlen == -1) {
-        reply_with_perror ("getxattr");
+        reply_with_perror_with_err (err, "getxattr");
         goto error;
       }
 
@@ -385,9 +396,10 @@ do_lxattrlist (const char *path, char *const *names)
       CHROOT_IN;
       vlen = lgetxattr (pathname, &buf[i],
                         entry[j+1].attrval.attrval_val, vlen);
+      err = get_error ();
       CHROOT_OUT;
       if (vlen == -1) {
-        reply_with_perror ("getxattr");
+        reply_with_perror_with_err (err, "getxattr");
         goto error;
       }
     }
-- 
1.6.5.2

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://et.redhat.com/~rjones/libguestfs/
See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html
-------------- next part --------------
>From 9d736b09a45fab349943058d60c7b4343167a3af Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones at redhat.com>
Date: Fri, 27 Nov 2009 17:56:51 +0000
Subject: [PATCH 9/9] daemon error handling: gnulib calls need separate error
handling.

---
 daemon/augeas.c      |    8 ++++----
 daemon/checksum.c    |    2 +-
 daemon/cmp.c         |    4 ++--
 daemon/command.c     |    2 +-
 daemon/cpmv.c        |    4 ++--
 daemon/daemon.h      |    9 +++++++++
 daemon/dd.c          |    4 ++--
 daemon/debug.c       |    6 +++---
 daemon/dir.c         |    4 ++--
 daemon/du.c          |    2 +-
 daemon/echo_daemon.c |    2 +-
 daemon/ext2.c        |    2 +-
 daemon/file.c        |   10 +++++-----
 daemon/find.c        |    8 ++++----
 daemon/grep.c        |    2 +-
 daemon/grub.c        |    2 +-
 daemon/guestfsd.c    |    6 +++---
 daemon/headtail.c    |    2 +-
 daemon/hexdump.c     |    2 +-
 daemon/initrd.c      |    2 +-
 daemon/inotify.c     |    8 ++++----
 daemon/link.c        |    6 +++---
 daemon/ls.c          |    2 +-
 daemon/lvm.c         |    4 ++--
 daemon/mount.c       |    8 ++++----
 daemon/parted.c      |    6 +++---
 daemon/proto.c       |    2 +-
 daemon/readdir.c     |    4 ++--
 daemon/realpath.c    |    4 ++--
 daemon/scrub.c       |    4 ++--
 daemon/selinux.c     |    2 +-
 daemon/stat.c        |    8 ++++----
 daemon/statvfs.c     |   10 +++++-----
 daemon/strings.c     |    2 +-
 daemon/swap.c        |    6 +++---
 daemon/tar.c         |    8 ++++----
 daemon/wc.c          |    2 +-
 daemon/xattr.c       |   22 +++++++++++-----------
 src/generator.ml     |    6 +++---
 39 files changed, 103 insertions(+), 94 deletions(-)

diff --git a/daemon/augeas.c b/daemon/augeas.c
index de325de..c04e345 100644
--- a/daemon/augeas.c
+++ b/daemon/augeas.c
@@ -74,7 +74,7 @@ do_aug_init (const char *root, int flags)
 
   buf = sysroot_path (root);
   if (!buf) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return -1;
   }
 
@@ -179,7 +179,7 @@ do_aug_get (const char *path)
   /* The value is an internal Augeas string, so we must copy it. GC FTW. */
   v = strdup (value);
   if (v == NULL) {
-    reply_with_perror ("strdup");
+    reply_with_perror_gnulib ("strdup");
     return NULL;
   }
 
@@ -290,7 +290,7 @@ do_aug_match (const char *path)
    */
   vp = realloc (matches, sizeof (char *) * (r+1));
   if (vp == NULL) {
-    reply_with_perror ("realloc");
+    reply_with_perror_gnulib ("realloc");
     free (vp);
     return NULL;
   }
@@ -365,7 +365,7 @@ do_aug_ls (const char *path)
     len += 3;			/* / * + terminating \0 */
     buf = malloc (len);
     if (buf == NULL) {
-      reply_with_perror ("malloc");
+      reply_with_perror_gnulib ("malloc");
       return NULL;
     }
 
diff --git a/daemon/checksum.c b/daemon/checksum.c
index 499d19d..6775a38 100644
--- a/daemon/checksum.c
+++ b/daemon/checksum.c
@@ -58,7 +58,7 @@ do_checksum (const char *csumtype, const char *path)
   /* Make the path relative to /sysroot. */
   buf = sysroot_path (path);
   if (!buf) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return NULL;
   }
 
diff --git a/daemon/cmp.c b/daemon/cmp.c
index 2b7e33b..2700bd0 100644
--- a/daemon/cmp.c
+++ b/daemon/cmp.c
@@ -36,13 +36,13 @@ do_equal (const char *file1, const char *file2)
 
   file1buf = sysroot_path (file1);
   if (file1buf == NULL) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return -1;
   }
 
   file2buf = sysroot_path (file2);
   if (file2buf == NULL) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     free (file1buf);
     return -1;
   }
diff --git a/daemon/command.c b/daemon/command.c
index b2350ec..d9ca2cb 100644
--- a/daemon/command.c
+++ b/daemon/command.c
@@ -64,7 +64,7 @@ do_command (char *const *argv)
   if (sysroot_dev == NULL || sysroot_dev_pts == NULL ||
       sysroot_proc == NULL || sysroot_selinux == NULL ||
       sysroot_sys == NULL) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     free (sysroot_dev);
     free (sysroot_dev_pts);
     free (sysroot_proc);
diff --git a/daemon/cpmv.c b/daemon/cpmv.c
index 47b8aa2..5288b69 100644
--- a/daemon/cpmv.c
+++ b/daemon/cpmv.c
@@ -54,13 +54,13 @@ cpmv_cmd (const char *cmd, const char *flags, const char
*src, const char *dest)
 
   srcbuf = sysroot_path (src);
   if (srcbuf == NULL) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return -1;
   }
 
   destbuf = sysroot_path (dest);
   if (destbuf == NULL) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     free (srcbuf);
     return -1;
   }
diff --git a/daemon/daemon.h b/daemon/daemon.h
index 96d08dd..167f1e5 100644
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -130,6 +130,10 @@ extern void main_loop (int sock) __attribute__((noreturn));
  *     return -1;
  *   }
  *
+ * The _gnulib_ variations are used for Gnulib replacement functions
+ * (eg. malloc, strdup, asprintf).  These always set errno, not the
+ * Windows equivalent.
+ *
  * The _sock_ variations are only used for socket calls (Win32 has a
  * different way to return errors from socket calls).  In reality
  * these variants are never used because the only socket we talk to is
@@ -139,6 +143,7 @@ extern void main_loop (int sock) __attribute__((noreturn));
 typedef int errtype;
 static inline void clear_error(void) { errno = 0; }
 static inline int get_error(void) { return errno; }
+static inline int get_gnulib_error(void) { return errno; }
 static inline int get_sock_error(void) { return errno; }
 #else
 typedef DWORD errtype;
@@ -149,6 +154,7 @@ static inline void clear_error(void)
   errno = 0;
 }
 static inline DWORD get_error(void) { return GetLastError (); }
+static inline DWORD get_gnulib_error(void) { return (DWORD) errno; }
 static inline DWORD get_sock_error(void) { return WSAGetLastError (); }
 #endif
 
@@ -174,6 +180,7 @@ static inline int get_errno(void) { return errno; }
 /* Ordinary daemon functions use these to indicate errors.
  * reply_with_error: Report an error.
  * reply_with_perror: Report an error followed by errno message.
+ * reply_with_perror_gnulib: Same as reply_with_perror for Gnulib calls.
  * reply_with_perror_sock: Same as reply_with_perror for socket calls.
  * reply_with_perror_with_err: Same as reply_with_perror but use an
  *   error code that you saved previously from get_error().
@@ -182,6 +189,8 @@ extern void reply_with_error (const char *fs, ...)
   __attribute__((format (printf,1,2)));
 #define reply_with_perror(...) \
   reply_with_perror_with_err (get_error(),__VA_ARGS__)
+#define reply_with_perror_gnulib(...) \
+  reply_with_perror_with_err (get_gnulib_error(),__VA_ARGS__)
 #define reply_with_perror_sock(...) \
   reply_with_perror_with_err (get_sock_error(),__VA_ARGS__)
 extern void reply_with_perror_with_err (errtype err, const char *fs, ...)
diff --git a/daemon/dd.c b/daemon/dd.c
index 0a53b31..907fa01 100644
--- a/daemon/dd.c
+++ b/daemon/dd.c
@@ -41,7 +41,7 @@ do_dd (const char *src, const char *dest)
   else
     r = asprintf (&if_arg, "if=%s%s", sysroot, src);
   if (r == -1) {
-    reply_with_perror ("asprintf");
+    reply_with_perror_gnulib ("asprintf");
     return -1;
   }
 
@@ -52,7 +52,7 @@ do_dd (const char *src, const char *dest)
   else
     r = asprintf (&of_arg, "of=%s%s", sysroot, dest);
   if (r == -1) {
-    reply_with_perror ("asprintf");
+    reply_with_perror_gnulib ("asprintf");
     free (if_arg);
     return -1;
   }
diff --git a/daemon/debug.c b/daemon/debug.c
index cb905cb..2b5fb06 100644
--- a/daemon/debug.c
+++ b/daemon/debug.c
@@ -101,7 +101,7 @@ debug_help (const char *subcmd, int argc, char *const *const
argv)
 
   r = strdup ("Commands supported:");
   if (!r) {
-    reply_with_perror ("strdup");
+    reply_with_perror_gnulib ("strdup");
     return NULL;
   }
 
@@ -110,7 +110,7 @@ debug_help (const char *subcmd, int argc, char *const *const
argv)
     len += strlen (cmds[i].cmd) + 1; /* space + new command */
     p = realloc (r, len + 1);	     /* +1 for the final NUL */
     if (p == NULL) {
-      reply_with_perror ("realloc");
+      reply_with_perror_gnulib ("realloc");
       free (r);
       return NULL;
     }
@@ -226,7 +226,7 @@ debug_sh (const char *subcmd, int argc, char *const *const
argv)
     len += strlen (argv[i]) + 1;
   cmd = malloc (len);
   if (!cmd) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return NULL;
   }
   for (i = j = 0; i < argc; ++i) {
diff --git a/daemon/dir.c b/daemon/dir.c
index 072dcf5..8eac736 100644
--- a/daemon/dir.c
+++ b/daemon/dir.c
@@ -65,7 +65,7 @@ do_rm_rf (const char *path)
 
   buf = sysroot_path (path);
   if (buf == NULL) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return -1;
   }
 
@@ -224,7 +224,7 @@ do_mkdtemp (const char *template)
 {
   char *writable = strdup (template);
   if (writable == NULL) {
-    reply_with_perror ("strdup");
+    reply_with_perror_gnulib ("strdup");
     return NULL;
   }
 
diff --git a/daemon/du.c b/daemon/du.c
index e6df245..21e6818 100644
--- a/daemon/du.c
+++ b/daemon/du.c
@@ -39,7 +39,7 @@ do_du (const char *path)
   /* Make the path relative to /sysroot. */
   buf = sysroot_path (path);
   if (!buf) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return -1;
   }
 
diff --git a/daemon/echo_daemon.c b/daemon/echo_daemon.c
index 658974b..e9ad742 100644
--- a/daemon/echo_daemon.c
+++ b/daemon/echo_daemon.c
@@ -49,7 +49,7 @@ do_echo_daemon (char *const *argv)
     /* Make the output buffer big enough for the string and its terminator */
     char *out_new = realloc (out, out_len + 1);
     if (NULL == out_new) {
-      reply_with_perror ("realloc");
+      reply_with_perror_gnulib ("realloc");
       free(out);
       return 0;
     }
diff --git a/daemon/ext2.c b/daemon/ext2.c
index f46bac9..b00e5fb 100644
--- a/daemon/ext2.c
+++ b/daemon/ext2.c
@@ -225,7 +225,7 @@ do_get_e2uuid (const char *device)
 
   p = strdup (p);
   if (!p) {
-    reply_with_perror ("strdup");
+    reply_with_perror_gnulib ("strdup");
     free (out);
     return NULL;
   }
diff --git a/daemon/file.c b/daemon/file.c
index bbb6426..da1f988 100644
--- a/daemon/file.c
+++ b/daemon/file.c
@@ -99,7 +99,7 @@ do_cat (const char *path)
       }
       buf2 = realloc (buf, alloc);
       if (buf2 == NULL) {
-        reply_with_perror ("realloc");
+        reply_with_perror_gnulib ("realloc");
         free (buf);
         close (fd);
         return NULL;
@@ -366,7 +366,7 @@ do_read_file (const char *path, size_t *size_r)
   }
   r = malloc (*size_r);
   if (r == NULL) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     close (fd);
     return NULL;
   }
@@ -417,7 +417,7 @@ do_pread (const char *path, int count, int64_t offset,
size_t *size_r)
 
   buf = malloc (count);
   if (buf == NULL) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     close (fd);
     return NULL;
   }
@@ -455,7 +455,7 @@ do_file (const char *path)
   else {
     buf = sysroot_path (path);
     if (!buf) {
-      reply_with_perror ("malloc");
+      reply_with_perror_gnulib ("malloc");
       return NULL;
     }
     freeit = 1;
@@ -511,7 +511,7 @@ do_zfile (const char *method, const char *path)
   }
 
   if (asprintf_nowarn (&cmd, "%s %R | file -bsL -", zcat, path)
== -1) {
-    reply_with_perror ("asprintf");
+    reply_with_perror_gnulib ("asprintf");
     return NULL;
   }
 
diff --git a/daemon/find.c b/daemon/find.c
index 391b87b..2382634 100644
--- a/daemon/find.c
+++ b/daemon/find.c
@@ -62,7 +62,7 @@ do_find (const char *dir)
 
   sysrootdir = sysroot_path (dir);
   if (!sysrootdir) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return NULL;
   }
 
@@ -82,7 +82,7 @@ do_find (const char *dir)
 
   /* Assemble the external find command. */
   if (asprintf_nowarn (&cmd, "find %Q -print0", sysrootdir) ==
-1) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("gnulib");
     free (sysrootdir);
     return NULL;
   }
@@ -154,7 +154,7 @@ do_find0 (const char *dir)
 
   sysrootdir = sysroot_path (dir);
   if (!sysrootdir) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return -1;
   }
 
@@ -173,7 +173,7 @@ do_find0 (const char *dir)
   sysrootdirlen = strlen (sysrootdir);
 
   if (asprintf_nowarn (&cmd, "find %Q -print0", sysrootdir) ==
-1) {
-    reply_with_perror ("asprintf");
+    reply_with_perror_gnulib ("asprintf");
     free (sysrootdir);
     return -1;
   }
diff --git a/daemon/grep.c b/daemon/grep.c
index d1f5a3f..bb0faa3 100644
--- a/daemon/grep.c
+++ b/daemon/grep.c
@@ -38,7 +38,7 @@ grep (const char *prog, const char *flag, const char *regex,
const char *path)
   /* Make the path relative to /sysroot. */
   buf = sysroot_path (path);
   if (!buf) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return NULL;
   }
 
diff --git a/daemon/grub.c b/daemon/grub.c
index 657abbb..a073313 100644
--- a/daemon/grub.c
+++ b/daemon/grub.c
@@ -33,7 +33,7 @@ do_grub_install (const char *root, const char *device)
   char *buf;
 
   if (asprintf_nowarn (&buf, "--root-directory=%R", root) == -1)
{
-    reply_with_perror ("asprintf");
+    reply_with_perror_gnulib ("asprintf");
     return -1;
   }
 
diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
index c3af8c5..b293ef6 100644
--- a/daemon/guestfsd.c
+++ b/daemon/guestfsd.c
@@ -445,7 +445,7 @@ read_cmdline (void)
 
 /* Turn "/path" into "/sysroot/path".
  *
- * Caller must check for NULL and call reply_with_perror ("malloc")
+ * Caller must check for NULL and call reply_with_perror_gnulib
("malloc")
  * if it is.  Caller must also free the string.
  *
  * See also the custom %R printf formatter which does shell quoting too.
@@ -516,7 +516,7 @@ add_string (char ***argv, int *size, int *alloc, const char
*str)
     *alloc += 64;
     new_argv = realloc (*argv, *alloc * sizeof (char *));
     if (new_argv == NULL) {
-      reply_with_perror ("realloc");
+      reply_with_perror_gnulib ("realloc");
       free_strings (*argv);
       return -1;
     }
@@ -526,7 +526,7 @@ add_string (char ***argv, int *size, int *alloc, const char
*str)
   if (str) {
     new_str = strdup (str);
     if (new_str == NULL) {
-      reply_with_perror ("strdup");
+      reply_with_perror_gnulib ("strdup");
       free_strings (*argv);
     }
   } else
diff --git a/daemon/headtail.c b/daemon/headtail.c
index 9175cf0..52fe060 100644
--- a/daemon/headtail.c
+++ b/daemon/headtail.c
@@ -38,7 +38,7 @@ headtail (const char *prog, const char *flag, const char *n,
const char *path)
   /* Make the path relative to /sysroot. */
   buf = sysroot_path (path);
   if (!buf) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return NULL;
   }
 
diff --git a/daemon/hexdump.c b/daemon/hexdump.c
index 7016faf..55409af 100644
--- a/daemon/hexdump.c
+++ b/daemon/hexdump.c
@@ -34,7 +34,7 @@ do_hexdump (const char *path)
 
   buf = sysroot_path (path);
   if (!buf) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return NULL;
   }
 
diff --git a/daemon/initrd.c b/daemon/initrd.c
index 9c92fee..0d8fdc0 100644
--- a/daemon/initrd.c
+++ b/daemon/initrd.c
@@ -40,7 +40,7 @@ do_initrd_list (const char *path)
 
   /* "zcat /sysroot/<path> | cpio --quiet -it", but path must
be quoted. */
   if (asprintf_nowarn (&cmd, "zcat %R | cpio --quiet -it", path)
== -1) {
-    reply_with_perror ("asprintf");
+    reply_with_perror_gnulib ("asprintf");
     return NULL;
   }
 
diff --git a/daemon/inotify.c b/daemon/inotify.c
index 749b8b7..634c125 100644
--- a/daemon/inotify.c
+++ b/daemon/inotify.c
@@ -159,7 +159,7 @@ do_inotify_add_watch (const char *path, int mask)
 
   buf = sysroot_path (path);
   if (!buf) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return -1;
   }
 
@@ -204,7 +204,7 @@ do_inotify_read (void)
 
   ret = malloc (sizeof *ret);
   if (ret == NULL) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return NULL;
   }
   ret->guestfs_int_inotify_event_list_len = 0;
@@ -257,7 +257,7 @@ do_inotify_read (void)
                     (ret->guestfs_int_inotify_event_list_len + 1) *
                     sizeof (guestfs_int_inotify_event));
       if (np == NULL) {
-        reply_with_perror ("realloc");
+        reply_with_perror_gnulib ("realloc");
         goto error;
       }
       ret->guestfs_int_inotify_event_list_val = np;
@@ -273,7 +273,7 @@ do_inotify_read (void)
       else
         in->in_name = strdup (""); /* Should have optional string
fields XXX. */
       if (in->in_name == NULL) {
-        reply_with_perror ("strdup");
+        reply_with_perror_gnulib ("strdup");
         goto error;
       }
 
diff --git a/daemon/link.c b/daemon/link.c
index 97f6ac0..b6d0a6a 100644
--- a/daemon/link.c
+++ b/daemon/link.c
@@ -47,7 +47,7 @@ do_readlink (const char *path)
 
   ret = strndup (link, r);
   if (ret == NULL) {
-    reply_with_perror ("strndup");
+    reply_with_perror_gnulib ("strndup");
     return NULL;
   }
 
@@ -117,7 +117,7 @@ _link (const char *flag, int symbolic, const char *target,
const char *linkname)
   /* Prefix linkname with sysroot. */
   buf_linkname = sysroot_path (linkname);
   if (!buf_linkname) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return -1;
   }
 
@@ -130,7 +130,7 @@ _link (const char *flag, int symbolic, const char *target,
const char *linkname)
   if (!symbolic && target[0] == '/') {
     buf_target = sysroot_path (target);
     if (!buf_target) {
-      reply_with_perror ("malloc");
+      reply_with_perror_gnulib ("malloc");
       free (buf_linkname);
       return -1;
     }
diff --git a/daemon/ls.c b/daemon/ls.c
index a15ce62..d9bc3e8 100644
--- a/daemon/ls.c
+++ b/daemon/ls.c
@@ -88,7 +88,7 @@ do_ll (const char *path)
 
   spath = sysroot_path (path);
   if (!spath) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return NULL;
   }
 
diff --git a/daemon/lvm.c b/daemon/lvm.c
index 564517c..81d2fdd 100644
--- a/daemon/lvm.c
+++ b/daemon/lvm.c
@@ -209,7 +209,7 @@ do_vgcreate (const char *volgroup, char *const *physvols)
   argc = count_strings (physvols) + 3;
   argv = malloc (sizeof (char *) * (argc + 1));
   if (argv == NULL) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return -1;
   }
   argv[0] = "/sbin/lvm";
@@ -437,7 +437,7 @@ do_vg_activate (int activate, char *const *volgroups)
   argc = count_strings (volgroups) + 4;
   argv = malloc (sizeof (char *) * (argc+1));
   if (argv == NULL) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return -1;
   }
 
diff --git a/daemon/mount.c b/daemon/mount.c
index f447380..ef8b51f 100644
--- a/daemon/mount.c
+++ b/daemon/mount.c
@@ -59,7 +59,7 @@ do_mount_vfs (const char *options, const char *vfstype,
 
   mp = sysroot_path (mountpoint);
   if (!mp) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return -1;
   }
 
@@ -116,7 +116,7 @@ do_umount (const char *pathordevice)
   buf = is_dev ? strdup (pathordevice)
                : sysroot_path (pathordevice);
   if (buf == NULL) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return -1;
   }
 
@@ -323,13 +323,13 @@ do_mount_loop (const char *file, const char *mountpoint)
   /* We have to prefix /sysroot on both the filename and the mountpoint. */
   mp = sysroot_path (mountpoint);
   if (!mp) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return -1;
   }
 
   buf = sysroot_path (file);
   if (!file) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     free (mp);
     return -1;
   }
diff --git a/daemon/parted.c b/daemon/parted.c
index 2b0df44..5f17e99 100644
--- a/daemon/parted.c
+++ b/daemon/parted.c
@@ -291,7 +291,7 @@ do_part_get_parttype (const char *device)
 
   r = strdup (r);
   if (!r) {
-    reply_with_perror ("strdup");
+    reply_with_perror_gnulib ("strdup");
     free_strings (lines);
     return NULL;
   }
@@ -321,14 +321,14 @@ do_part_list (const char *device)
 
   r = malloc (sizeof *r);
   if (r == NULL) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     goto error1;
   }
   r->guestfs_int_partition_list_len = nr_rows;
   r->guestfs_int_partition_list_val      malloc (nr_rows * sizeof
(guestfs_int_partition));
   if (r->guestfs_int_partition_list_val == NULL) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     goto error2;
   }
 
diff --git a/daemon/proto.c b/daemon/proto.c
index 23748a4..f4fb017 100644
--- a/daemon/proto.c
+++ b/daemon/proto.c
@@ -81,7 +81,7 @@ main_loop (int _sock)
 
     buf = malloc (len);
     if (!buf) {
-      reply_with_perror ("malloc");
+      reply_with_perror_gnulib ("malloc");
       continue;
     }
 
diff --git a/daemon/readdir.c b/daemon/readdir.c
index 0d72912..849cf45 100644
--- a/daemon/readdir.c
+++ b/daemon/readdir.c
@@ -39,7 +39,7 @@ do_readdir (const char *path)
 
   ret = malloc (sizeof *ret);
   if (ret == NULL) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return NULL;
   }
 
@@ -65,7 +65,7 @@ do_readdir (const char *path)
                  sizeof (guestfs_int_dirent) * (i+1));
     v.name = strdup (d->d_name);
     if (!p || !v.name) {
-      reply_with_perror ("allocate");
+      reply_with_perror_gnulib ("realloc");
       free (ret->guestfs_int_dirent_list_val);
       free (p);
       free (v.name);
diff --git a/daemon/realpath.c b/daemon/realpath.c
index 2760aff..0d50bb9 100644
--- a/daemon/realpath.c
+++ b/daemon/realpath.c
@@ -191,7 +191,7 @@ do_case_sensitive_path (const char *path)
   ret[next] = '\0';
   char *retp = strdup (ret);
   if (retp == NULL) {
-    reply_with_perror ("strdup");
+    reply_with_perror_gnulib ("strdup");
     return NULL;
   }
   return retp;                  /* caller frees */
@@ -205,7 +205,7 @@ do_case_sensitive_path (const char *path)
    */
   char *ret = strdup (path);
   if (ret == NULL) {
-    reply_with_perror ("strdup");
+    reply_with_perror_gnulib ("strdup");
     return NULL;
   }
   return ret;
diff --git a/daemon/scrub.c b/daemon/scrub.c
index e37a1e1..f7cdddf 100644
--- a/daemon/scrub.c
+++ b/daemon/scrub.c
@@ -63,7 +63,7 @@ do_scrub_file (const char *file)
   /* Make the path relative to /sysroot. */
   buf = sysroot_path (file);
   if (!buf) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return -1;
   }
 
@@ -90,7 +90,7 @@ do_scrub_freespace (const char *dir)
   /* Make the path relative to /sysroot. */
   buf = sysroot_path (dir);
   if (!buf) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return -1;
   }
 
diff --git a/daemon/selinux.c b/daemon/selinux.c
index e49e657..aac44e7 100644
--- a/daemon/selinux.c
+++ b/daemon/selinux.c
@@ -79,7 +79,7 @@ do_getcon (void)
   r = strdup (context);
   freecon (context);
   if (r == NULL) {
-    reply_with_perror ("strdup");
+    reply_with_perror_gnulib ("strdup");
     return NULL;
   }
 
diff --git a/daemon/stat.c b/daemon/stat.c
index c14d15e..ad999cb 100644
--- a/daemon/stat.c
+++ b/daemon/stat.c
@@ -50,7 +50,7 @@ do_stat (const char *path)
 
   ret = malloc (sizeof *ret);
   if (ret == NULL) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return NULL;
   }
 
@@ -99,7 +99,7 @@ do_lstat (const char *path)
 
   ret = malloc (sizeof *ret);
   if (ret == NULL) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return NULL;
   }
 
@@ -140,13 +140,13 @@ do_lstatlist (const char *path, char *const *names)
 
   ret = malloc (sizeof *ret);
   if (!ret) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return NULL;
   }
   ret->guestfs_int_stat_list_len = nr_names;
   ret->guestfs_int_stat_list_val = calloc (nr_names, sizeof
(guestfs_int_stat));
   if (ret->guestfs_int_stat_list_val == NULL) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     free (ret);
     return NULL;
   }
diff --git a/daemon/statvfs.c b/daemon/statvfs.c
index 4fdcc24..202e9db 100644
--- a/daemon/statvfs.c
+++ b/daemon/statvfs.c
@@ -60,7 +60,7 @@ do_statvfs (const char *path)
 
   ret = malloc (sizeof *ret);
   if (ret == NULL) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return NULL;
   }
 
@@ -89,7 +89,7 @@ do_statvfs (const char *path)
 
   disk = sysroot_path (path);
   if (!disk) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return NULL;
   }
 
@@ -105,7 +105,7 @@ do_statvfs (const char *path)
 
   ret = malloc (sizeof *ret);
   if (ret == NULL) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return NULL;
   }
 
@@ -150,7 +150,7 @@ do_statvfs (const char *path)
 
   disk = sysroot_path (path);
   if (!disk) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return NULL;
   }
 
@@ -164,7 +164,7 @@ do_statvfs (const char *path)
 
   ret = malloc (sizeof *ret);
   if (ret == NULL) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return NULL;
   }
 
diff --git a/daemon/strings.c b/daemon/strings.c
index d7dc392..051573b 100644
--- a/daemon/strings.c
+++ b/daemon/strings.c
@@ -35,7 +35,7 @@ do_strings_e (const char *encoding, const char *path)
 
   buf = sysroot_path (path);
   if (!buf) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return NULL;
   }
 
diff --git a/daemon/swap.c b/daemon/swap.c
index 2d3d9ff..6c86d30 100644
--- a/daemon/swap.c
+++ b/daemon/swap.c
@@ -94,7 +94,7 @@ do_mkswap_file (const char *path)
 
   buf = sysroot_path (path);
   if (!buf) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return -1;
   }
 
@@ -145,7 +145,7 @@ do_swapon_file (const char *path)
 
   buf = sysroot_path (path);
   if (!buf) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return -1;
   }
 
@@ -162,7 +162,7 @@ do_swapoff_file (const char *path)
 
   buf = sysroot_path (path);
   if (!buf) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return -1;
   }
 
diff --git a/daemon/tar.c b/daemon/tar.c
index 56d291d..cbd3aa2 100644
--- a/daemon/tar.c
+++ b/daemon/tar.c
@@ -51,7 +51,7 @@ do_tar_in (const char *dir)
 
   /* "tar -C /sysroot%s -xf -" but we have to quote the dir. */
   if (asprintf_nowarn (&cmd, "tar -C %R -xf -", dir) == -1) {
-    err = get_error ();
+    err = get_gnulib_error ();
     cancel_receive ();
     reply_with_perror_with_err (err, "asprintf");
     return -1;
@@ -105,7 +105,7 @@ do_tar_out (const char *dir)
 
   /* "tar -C /sysroot%s -cf - ." but we have to quote the dir. */
   if (asprintf_nowarn (&cmd, "tar -C %R -cf - .", dir) == -1) {
-    reply_with_perror ("asprintf");
+    reply_with_perror_gnulib ("asprintf");
     return -1;
   }
 
@@ -169,7 +169,7 @@ do_tgz_in (const char *dir)
 
   /* "tar -C /sysroot%s -zxf -" but we have to quote the dir. */
   if (asprintf_nowarn (&cmd, "tar -C %R -zxf -", dir) == -1) {
-    err = get_error ();
+    err = get_gnulib_error ();
     cancel_receive ();
     reply_with_perror_with_err (err, "asprintf");
     return -1;
@@ -223,7 +223,7 @@ do_tgz_out (const char *dir)
 
   /* "tar -C /sysroot%s -zcf - ." but we have to quote the dir. */
   if (asprintf_nowarn (&cmd, "tar -C %R -zcf - .", dir) == -1) {
-    reply_with_perror ("asprintf");
+    reply_with_perror_gnulib ("asprintf");
     return -1;
   }
 
diff --git a/daemon/wc.c b/daemon/wc.c
index b64940b..53791fa 100644
--- a/daemon/wc.c
+++ b/daemon/wc.c
@@ -37,7 +37,7 @@ wc (const char *flag, const char *path)
   /* Make the path relative to /sysroot. */
   buf = sysroot_path (path);
   if (!buf) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     return -1;
   }
 
diff --git a/daemon/xattr.c b/daemon/xattr.c
index 77092c4..17afbae 100644
--- a/daemon/xattr.c
+++ b/daemon/xattr.c
@@ -135,7 +135,7 @@ getxattrs (const char *path,
 
   buf = malloc (len);
   if (buf == NULL) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     goto error;
   }
 
@@ -150,7 +150,7 @@ getxattrs (const char *path,
 
   r = calloc (1, sizeof (*r));
   if (r == NULL) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     goto error;
   }
 
@@ -164,7 +164,7 @@ getxattrs (const char *path,
   r->guestfs_int_xattr_list_val      calloc
(r->guestfs_int_xattr_list_len, sizeof (guestfs_int_xattr));
   if (r->guestfs_int_xattr_list_val == NULL) {
-    reply_with_perror ("calloc");
+    reply_with_perror_gnulib ("calloc");
     goto error;
   }
 
@@ -184,7 +184,7 @@ getxattrs (const char *path,
 
     if (r->guestfs_int_xattr_list_val[j].attrname == NULL ||
         r->guestfs_int_xattr_list_val[j].attrval.attrval_val == NULL) {
-      reply_with_perror ("malloc");
+      reply_with_perror_gnulib ("malloc");
       goto error;
     }
 
@@ -284,7 +284,7 @@ do_lxattrlist (const char *path, char *const *names)
 
   ret = malloc (sizeof (*ret));
   if (ret == NULL) {
-    reply_with_perror ("malloc");
+    reply_with_perror_gnulib ("malloc");
     goto error;
   }
 
@@ -309,7 +309,7 @@ do_lxattrlist (const char *path, char *const *names)
       realloc (ret->guestfs_int_xattr_list_val,
                (ret->guestfs_int_xattr_list_len+1)*sizeof
(guestfs_int_xattr));
     if (newptr == NULL) {
-      reply_with_perror ("realloc");
+      reply_with_perror_gnulib ("realloc");
       goto error;
     }
     ret->guestfs_int_xattr_list_val = newptr;
@@ -323,7 +323,7 @@ do_lxattrlist (const char *path, char *const *names)
 
     entry->attrname = strdup ("");
     if (entry->attrname == NULL) {
-      reply_with_perror ("strdup");
+      reply_with_perror_gnulib ("strdup");
       goto error;
     }
 
@@ -335,7 +335,7 @@ do_lxattrlist (const char *path, char *const *names)
 
     buf = malloc (len);
     if (buf == NULL) {
-      reply_with_perror ("malloc");
+      reply_with_perror_gnulib ("malloc");
       goto error;
     }
 
@@ -357,7 +357,7 @@ do_lxattrlist (const char *path, char *const *names)
                (ret->guestfs_int_xattr_list_len+nr_attrs) *
                sizeof (guestfs_int_xattr));
     if (newptr == NULL) {
-      reply_with_perror ("realloc");
+      reply_with_perror_gnulib ("realloc");
       goto error;
     }
     ret->guestfs_int_xattr_list_val = newptr;
@@ -389,7 +389,7 @@ do_lxattrlist (const char *path, char *const *names)
 
       if (entry[j+1].attrname == NULL ||
           entry[j+1].attrval.attrval_val == NULL) {
-        reply_with_perror ("malloc");
+        reply_with_perror_gnulib ("malloc");
         goto error;
       }
 
@@ -412,7 +412,7 @@ do_lxattrlist (const char *path, char *const *names)
     entry[0].attrval.attrval_val = strdup (num);
 
     if (entry[0].attrval.attrval_val == NULL) {
-      reply_with_perror ("strdup");
+      reply_with_perror_gnulib ("strdup");
       goto error;
     }
   }
diff --git a/src/generator.ml b/src/generator.ml
index 9e602ac..129041d 100755
--- a/src/generator.ml
+++ b/src/generator.ml
@@ -5646,7 +5646,7 @@ 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;
-             pr "    reply_with_perror (\"realloc\");\n";
+             pr "    reply_with_perror_gnulib
(\"realloc\");\n";
              pr "    goto done;\n";
              pr "  }\n";
              pr "  %s[args.%s.%s_len] = NULL;\n" n n n;
@@ -5898,7 +5898,7 @@ and generate_daemon_actions ()          pr "\n";
         pr "  ret = malloc (sizeof *ret);\n";
         pr "  if (!ret) {\n";
-        pr "    reply_with_perror (\"malloc\");\n";
+        pr "    reply_with_perror_gnulib (\"malloc\");\n";
         pr "    return NULL;\n";
         pr "  }\n";
         pr "\n";
@@ -5941,7 +5941,7 @@ and generate_daemon_actions ()          pr "    newp
= realloc (ret->guestfs_int_lvm_%s_list_val,\n" typ;
         pr "		    sizeof (guestfs_int_lvm_%s) * (i+1));\n" typ;
         pr "    if (newp == NULL) {\n";
-        pr "      reply_with_perror (\"realloc\");\n";
+        pr "      reply_with_perror_gnulib
(\"realloc\");\n";
         pr "      free (ret->guestfs_int_lvm_%s_list_val);\n" typ;
         pr "      free (ret);\n";
         pr "      free (out);\n";
-- 
1.6.5.2

On Fri, Nov 27, 2009 at 06:01:56PM +0000, Richard W.M. Jones
wrote:
> The more I look at this patch, the less I like it.
But I think some patches in this set do make sense to apply anyway:

[PATCH 1/9] daemon error handling: Formalize access to errno.

- I think the idea of formalizing access to errno, and in particular
  not having code that saves and restores errno, is a good one.

[PATCH 5/9] daemon error handling: fix case_sensitive_path for Windows.

- This is really a separate issue and I think we should just apply
  this one.

[PATCH 6/9] daemon error handling: clear errors before all functions

- Clearing errno before invoking stub functions seems like a
  reasonable change to me.  It can avoid incorrect error messages in
  cases where error handling paths that aren't tested very much are
  wrong, for virtually no cost.

[PATCH 7/9] daemon error handling: recursive_mkdir shouldn't need to
set errno.

- This one is basically a cleanup, it avoids setting errno in order to
  communicate an error to the local caller, and I think we should
  apply it.

[PATCH 8/9] daemon error handling: Change CHROOT_* macros handling of
errno.

- Chroot handling in general needs fixing for Win32, but I didn't like
  the way the macros save and restore errno, and so I think this
  change makes sense on its own.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://et.redhat.com/~rjones/virt-df/