Libguestfs - Jun 2015 - Re: [PATCH 3/7] daemon: Add --cmdline option for testing.

Currently we are unable to properly run guestfsd (the daemon) under
valgrind.  Attempts to run valgrind inside the appliance have not been
successful (see patch 1/7).

However we desperately need better valgrind coverage of the daemon,
particularly because it is doing a lot of complex parsing of program
output.  This has been a problem for a long time.

A better way to attack this problem is to run the daemon as a regular
host process and connect libguestfs directly to it.  Because of
'libguestfs live' functionality this is quite simple: we are able to
connect the library to any Unix domain socket and talk directly to the
daemon.

This patch series modified the daemon so it can be run as a standalone
process listening on a Unix domain socket, adds new tests which run
the daemon as a host process, and a simple framework to run the daemon
under valgrind.

Rich.

If you've ever tried to use this option, you'll know that it didn't
work well.  It broke random things (probably RHBZ#1020216, definitely
RHBZ#1023630), and caused random failures generally, while often not
actually failing when valgrind itself found problems.
---
 appliance/Makefile.am           |  7 +------
 appliance/guestfsd.suppressions | 27 ---------------------------
 appliance/init                  | 20 +-------------------
 appliance/packagelist.in        |  2 --
 configure.ac                    | 14 --------------
 daemon/internal.c               | 15 ---------------
 generator/actions.ml            | 11 -----------
 src/conn-socket.c               | 20 --------------------
 src/guestfs-internal.h          | 12 ------------
 src/handle.c                    | 15 ---------------
 src/launch-uml.c                | 11 -----------
 src/launch.c                    |  3 ---
 12 files changed, 2 insertions(+), 155 deletions(-)
 delete mode 100644 appliance/guestfsd.suppressions

diff --git a/appliance/Makefile.am b/appliance/Makefile.am
index 3135219..cc4bfc2 100644
--- a/appliance/Makefile.am
+++ b/appliance/Makefile.am
@@ -20,7 +20,6 @@ include $(top_srcdir)/subdir-rules.mk
 EXTRA_DIST = \
 	99-guestfs-serial.rules \
 	excludefiles.in \
-	guestfsd.suppressions \
 	guestfs_lvm_conf.aug \
 	guestfs_shadow.aug \
 	hostfiles.in \
@@ -66,9 +65,6 @@ make.sh: make.sh.in $(top_builddir)/config.log
$(top_builddir)/config.status
 	rm -f $@-t
 
 PACKAGELIST_CPP_FLAGS = -D$(DISTRO)=1
-DEXTRA_PACKAGES="$(EXTRA_PACKAGES)"
-if VALGRIND_DAEMON
-PACKAGELIST_CPP_FLAGS += -DVALGRIND_DAEMON=1
-endif
 
 packagelist: packagelist.in Makefile
 	m4 $(PACKAGELIST_CPP_FLAGS) $< | \
@@ -76,12 +72,11 @@ packagelist: packagelist.in Makefile
 	cmp -s $@ $@-t || mv $@-t $@
 	rm -f $@-t
 
-supermin.d/daemon.tar.gz: ../daemon/guestfsd guestfsd.suppressions
guestfs_lvm_conf.aug guestfs_shadow.aug
+supermin.d/daemon.tar.gz: ../daemon/guestfsd guestfs_lvm_conf.aug
guestfs_shadow.aug
 	rm -f $@ $@-t
 	rm -rf tmp-d
 	mkdir -p tmp-d$(DAEMON_SUPERMIN_DIR) tmp-d/etc tmp-d/usr/share/guestfs
 	ln ../daemon/guestfsd tmp-d$(DAEMON_SUPERMIN_DIR)/guestfsd
-	ln $(srcdir)/guestfsd.suppressions tmp-d/etc/guestfsd.suppressions
 	ln $(srcdir)/guestfs_lvm_conf.aug tmp-d/usr/share/guestfs/guestfs_lvm_conf.aug
 	ln $(srcdir)/guestfs_shadow.aug tmp-d/usr/share/guestfs/guestfs_shadow.aug
 	( cd tmp-d && tar zcf - * ) > $@-t
diff --git a/appliance/guestfsd.suppressions b/appliance/guestfsd.suppressions
deleted file mode 100644
index 26ba1c8..0000000
--- a/appliance/guestfsd.suppressions
+++ /dev/null
@@ -1,27 +0,0 @@
-# This file is only used when libguestfs is configured with
-#
-#   ./configure --enable-valgrind-daemon
-#
-# (only used for development, and only used in the regular supermin
-# appliance, not libguestfs live).
-#
-# If there are any valgrind errors in the base libraries such as
-# glibc, then we can suppress them here, so we only see errors in
-# libguestfs daemon code.
-
-# libdl
-{
-  libdl_index_cond
-  Memcheck:Cond
-  fun:index
-  fun:expand_dynamic_string_token
-  fun:_dl_map_object
-}
-
-# aug_setm memory leak
-{
-  aug_setm_leak
-  Memcheck:Leak
-  ...
-  fun:aug_setm
-}
diff --git a/appliance/init b/appliance/init
index eb1b487..3a185c8 100755
--- a/appliance/init
+++ b/appliance/init
@@ -134,25 +134,7 @@ fi
 
 if ! grep -sq guestfs_rescue=1 /proc/cmdline; then
   # Run the daemon.
-
-  # Run the daemon under valgrind if ./configure --enable-valgrind-daemon
-  if grep -sq guestfs_valgrind_daemon=1 /proc/cmdline; then
-    if [ -r /etc/guestfsd.suppressions ]; then
-      suppressions="--suppressions=/etc/guestfsd.suppressions"
-    fi
-    vg="valgrind --leak-check=full --error-exitcode=119
--max-stackframe=8388608 --child-silent-after-fork=yes $suppressions"
-    echo "enabling valgrind: $vg"
-  fi
-
-  # Run guestfsd, under valgrind if asked.
-  $vg guestfsd
-  if [ $? -eq 119 ]; then
-      echo "DAEMON VALGRIND FAILED"
-      # Sleep so valgrind messages are seen by the host.  Note this
-      # only happens in non-production builds
-      # (--enable-valgrind-daemon) + on an error path.
-      sleep 10
-  fi
+  guestfsd
 else
   # Run virt-rescue shell.
 
diff --git a/appliance/packagelist.in b/appliance/packagelist.in
index 76c7293..0f0ae89 100644
--- a/appliance/packagelist.in
+++ b/appliance/packagelist.in
@@ -253,7 +253,5 @@ util-linux-ng
 xfsprogs
 zerofree
 
-ifelse(VALGRIND_DAEMON,1,valgrind)
-
 dnl Define this by doing: ./configure --with-extra-packages="..."
 EXTRA_PACKAGES
diff --git a/configure.ac b/configure.ac
index bee8c94..32fedd3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -435,19 +435,6 @@ if test "x$enable_daemon" = "xyes";
then
             [enable_install_daemon=no])
     AC_MSG_RESULT([$enable_install_daemon])
 
-    dnl Enable valgrind in the daemon.
-    AC_MSG_CHECKING([if we should run the daemon under valgrind])
-    AC_ARG_ENABLE([valgrind-daemon],
-        [AS_HELP_STRING([--enable-valgrind-daemon],
-            [run the daemon under valgrind (developers only)
@<:@default=no@:>@])],
-            [],
-            [enable_valgrind_daemon=no])
-    AC_MSG_RESULT([$enable_valgrind_daemon])
-
-    if test "x$enable_valgrind_daemon" = "xyes"; then
-        AC_DEFINE([VALGRIND_DAEMON],[1],[Define to 1 to run the daemon under
valgrind.])
-    fi
-
     dnl Which directory should we put the daemon in?  NOTE: This
     dnl is the "virtual" directory inside the appliance, not the
     dnl install directory for libguestfs live.  Since Fedora 17
@@ -487,7 +474,6 @@ This means you either have a very old glibc (pre-2.0) or you
 are using some other libc where this is not supported.])])])
 fi
 AM_CONDITIONAL([INSTALL_DAEMON],[test "x$enable_install_daemon" =
"xyes"])
-AM_CONDITIONAL([VALGRIND_DAEMON],[test "x$enable_valgrind_daemon" =
"xyes"])
 
 dnl Build the appliance?
 AC_MSG_CHECKING([if we should build the appliance])
diff --git a/daemon/internal.c b/daemon/internal.c
index 781366d..c8ae7fd 100644
--- a/daemon/internal.c
+++ b/daemon/internal.c
@@ -44,18 +44,3 @@ do_internal_autosync (void)
 
   return r;
 }
-
-/* NB: Only called when valgrinding the daemon. */
-int
-do_internal_exit (void)
-{
-  if (!autosync_umount) {
-    reply_with_error ("guestfsd -r flag used, ignoring");
-    return -1;
-  }
-
-  /* Send a reply before exiting so the protocol doesn't get confused. */
-  reply (NULL, NULL);
-
-  exit (EXIT_SUCCESS);
-}
diff --git a/generator/actions.ml b/generator/actions.ml
index 11b8652..c35fe0c 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -11948,17 +11948,6 @@ this function returns an error." };
 This function is used internally when setting up the appliance." };
 
   { defaults with
-    name = "internal_exit"; added = (1, 23, 30);
-    style = RErr, [], [];
-    proc_nr = Some 414;
-    visibility = VInternal;
-    cancellable = true;
-    shortdesc = "cause the daemon to exit (internal use only)";
-    longdesc = "\
-This function is used internally when closing the appliance.  Note
-it's only called when ./configure --enable-valgrind-daemon is used."
};
-
-  { defaults with
     name = "copy_attributes"; added = (1, 25, 21);
     style = RErr, [Pathname "src"; Pathname "dest"], [OBool
"all"; OBool "mode"; OBool "xattributes"; OBool
"ownership"];
     proc_nr = Some 415;
diff --git a/src/conn-socket.c b/src/conn-socket.c
index 3ead48f..980d7df 100644
--- a/src/conn-socket.c
+++ b/src/conn-socket.c
@@ -343,26 +343,6 @@ handle_log_message (guestfs_h *g,
   /* It's an actual log message, send it upwards. */
   guestfs_int_log_message_callback (g, buf, n);
 
-#ifdef VALGRIND_DAEMON
-  /* Find the canary printed by appliance/init if valgrinding of the
-   * daemon fails, and exit abruptly.  Note this is only used in
-   * developer builds, and should never be enabled in ordinary/
-   * production builds.
-   */
-  if (g->verbose) {
-    const char *valgrind_canary = "DAEMON VALGRIND FAILED";
-
-    if (memmem (buf, n, valgrind_canary, strlen (valgrind_canary)) != NULL) {
-      fprintf (stderr,
-               "Detected valgrind failure in the daemon!  Exiting with
exit code 119.\n"
-               "See log messages printed above.\n"
-               "Note: This happens because libguestfs was configured
with\n"
-               "'--enable-valgrind-daemon' which should not be
used in production builds.\n");
-      exit (119);
-    }
-  }
-#endif
-
   return 1;
 }
 
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index bbd7fb4..cf6e534 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -79,18 +79,6 @@
 #  define MIN_MEMSIZE 256
 #endif
 
-/* Valgrind has a fairly hefty memory overhead.  Using the defaults
- * caused the C API tests to fail.
- */
-#ifdef VALGRIND_DAEMON
-#  ifndef DEFAULT_MEMSIZE
-#    define DEFAULT_MEMSIZE 768
-#  endif
-#  ifndef MIN_MEMSIZE
-#    define MIN_MEMSIZE 256
-#  endif
-#endif
-
 /* The default and minimum memory size for most users. */
 #ifndef DEFAULT_MEMSIZE
 #  define DEFAULT_MEMSIZE 500
diff --git a/src/handle.c b/src/handle.c
index 51b9572..b15a15d 100644
--- a/src/handle.c
+++ b/src/handle.c
@@ -426,21 +426,6 @@ shutdown_backend (guestfs_h *g, int check_for_errors)
   if (g->autosync && g->state == READY) {
     if (guestfs_internal_autosync (g) == -1)
       ret = -1;
-#ifdef VALGRIND_DAEMON
-    /* When valgrinding the daemon, this causes the daemon to exit
-     * properly, so we'll see any valgrind problems.  Don't do this in
-     * production builds because it's slow and unnecessary.
-     */
-    guestfs_internal_exit (g);
-    if (g->conn) {
-      /* internal_exit above will cause the daemon to close the
-       * connection.  The purpose of the read_data here is to read the
-       * remaining log messages.
-       */
-      char buf[1];
-      g->conn->ops->read_data (g, g->conn, buf, sizeof buf);
-    }
-#endif
   }
 
   /* Shut down the backend. */
diff --git a/src/launch-uml.c b/src/launch-uml.c
index 785548c..6a63b6b 100644
--- a/src/launch-uml.c
+++ b/src/launch-uml.c
@@ -267,17 +267,6 @@ launch_uml (guestfs_h *g, void *datav, const char *arg)
   ADD_CMDLINE_PRINTF ("ssl3=fd:%d", dsv[1]);
   ADD_CMDLINE ("guestfs_channel=/dev/ttyS3");
 
-#if 0 /* XXX This could be made to work. */
-#ifdef VALGRIND_DAEMON
-  /* Set up virtio-serial channel for valgrind messages. */
-  ADD_CMDLINE ("-chardev");
-  ADD_CMDLINE_PRINTF ("file,path=%s/valgrind.log.%d,id=valgrind",
-                      VALGRIND_LOG_PATH, getpid ());
-  ADD_CMDLINE ("-device");
-  ADD_CMDLINE
("virtserialport,chardev=valgrind,name=org.libguestfs.valgrind");
-#endif
-#endif
-
   /* Add any vmlinux parameters. */
   for (hp = g->hv_params; hp; hp = hp->next) {
     ADD_CMDLINE (hp->hv_param);
diff --git a/src/launch.c b/src/launch.c
index fd5479e..987d2db 100644
--- a/src/launch.c
+++ b/src/launch.c
@@ -335,9 +335,6 @@ guestfs_int_appliance_command_line (guestfs_h *g, const char
*appliance_dev,
 #ifdef __arm__
      " mem=%dM"
 #endif
-#ifdef VALGRIND_DAEMON
-     " guestfs_valgrind_daemon=1"
-#endif
 #ifdef __i386__
      " noapic"                  /* workaround for RHBZ#857026 */
 #endif
-- 
2.3.1

We'll now just require that valgrind is new enough.

As best as I can tell from the valgrind subversion(!) repository, this
option was added to valgrind in around 2011.
---
 configure.ac | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/configure.ac b/configure.ac
index 32fedd3..8775c4b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1069,21 +1069,10 @@ AS_IF([$CXX --version >&AS_MESSAGE_LOG_FD
2>&1],[have_cxx=yes],[have_cxx=no])
 AC_MSG_RESULT([$have_cxx])
 AM_CONDITIONAL([HAVE_CXX], [test "$have_cxx" = "yes"])
 
-dnl If valgrind is present (it's not required), check whether or not
-dnl it supports the new 'valgrind --vgdb' option.
 AC_CHECK_PROG([VALGRIND],[valgrind],[valgrind],[no])
 AS_IF([test "x$VALGRIND" != "xno"],[
-    AC_MSG_CHECKING([if $VALGRIND supports the --vgdb option])
-    if $VALGRIND --help | grep -sq -- --vgdb; then
-        AC_MSG_RESULT([yes])
-        AC_SUBST([VALGRIND_NO_VGDB],[--vgdb=no])
-    else
-        AC_MSG_RESULT([no])
-        AC_SUBST([VALGRIND_NO_VGDB],[])
-    fi
-
     # Substitute the whole valgrind command.
-    VG='$(VALGRIND) $(VALGRIND_NO_VGDB)
--log-file=$(abs_top_builddir)/tmp/valgrind-%q{T}-%p.log --leak-check=full
--error-exitcode=119 --suppressions=$(abs_top_srcdir)/valgrind-suppressions'
+    VG='$(VALGRIND) --vgdb=no
--log-file=$(abs_top_builddir)/tmp/valgrind-%q{T}-%p.log --leak-check=full
--error-exitcode=119 --suppressions=$(abs_top_srcdir)/valgrind-suppressions'
     ],[
     # No valgrind, so substitute VG with something that will break.
     VG=VALGRIND_IS_NOT_INSTALLED
-- 
2.3.1

This allows the Linux kernel command line to be specified on the
program command line.
---
 daemon/guestfsd.c   | 22 ++++++++++++++++++----
 daemon/guestfsd.pod |  6 ++++++
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
index eef24d5..190ade2 100644
--- a/daemon/guestfsd.c
+++ b/daemon/guestfsd.c
@@ -130,7 +130,7 @@ static void
 usage (void)
 {
   fprintf (stderr,
-    "guestfsd [-r] [-v|--verbose]\n");
+    "guestfsd [--cmdline CMDLINE] [-r] [-v|--verbose]\n");
 }
 
 int
@@ -138,12 +138,14 @@ main (int argc, char *argv[])
 {
   static const char *options = "rv?";
   static const struct option long_options[] = {
+    { "cmdline", 1, 0, 0 },
     { "help", 0, 0, '?' },
     { "verbose", 0, 0, 'v' },
     { 0, 0, 0, 0 }
   };
   int c;
-  char *cmdline;
+  char *cmdline = NULL;
+  int option_index;
 
   ignore_value (chdir ("/"));
 
@@ -178,10 +180,21 @@ main (int argc, char *argv[])
     root_device = statbuf.st_dev;
 
   for (;;) {
-    c = getopt_long (argc, argv, options, long_options, NULL);
+    c = getopt_long (argc, argv, options, long_options, &option_index);
     if (c == -1) break;
 
     switch (c) {
+    case 0:                     /* options which are long only */
+      if (STREQ (long_options[option_index].name, "cmdline")) {
+        cmdline = strdup (optarg);
+        if (!cmdline)
+          error (EXIT_FAILURE, errno, "strdup");
+      }
+      else
+        error (EXIT_FAILURE, 0, "unknown long option: %s (%d)\n",
+               long_options[option_index].name, option_index);
+      break;
+
       /* The -r flag is used when running standalone.  It changes
        * several aspects of the daemon.
        */
@@ -210,7 +223,8 @@ main (int argc, char *argv[])
     exit (EXIT_FAILURE);
   }
 
-  cmdline = read_cmdline ();
+  if (!cmdline)
+    cmdline = read_cmdline ();
 
   /* Set the verbose flag. */
   verbose = verbose ||
diff --git a/daemon/guestfsd.pod b/daemon/guestfsd.pod
index 1ed31a9..96bbbd6 100644
--- a/daemon/guestfsd.pod
+++ b/daemon/guestfsd.pod
@@ -50,6 +50,12 @@ removed.
 
 Display brief help.
 
+=item B<--cmdline> CMDLINE
+
+This option is used for testing.  Instead of reading the Linux kernel
+command line from F</proc/cmdline>, use the value given on the
+program's command line.
+
 =item B<-r>
 
 Set the root filesystem to be F</> (instead of the default which is
-- 
2.3.1

This is a catch-all mode for turning off daemon features which
interfere with our testing of the daemon.
---
 daemon/daemon.h   |  2 ++
 daemon/guestfsd.c | 17 ++++++++++++++---
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/daemon/daemon.h b/daemon/daemon.h
index 136e9a9..e977095 100644
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -47,6 +47,8 @@ extern int enable_network;
 
 extern int autosync_umount;
 
+extern int test_mode;
+
 extern const char *sysroot;
 extern size_t sysroot_len;
 
diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
index 190ade2..522c51a 100644
--- a/daemon/guestfsd.c
+++ b/daemon/guestfsd.c
@@ -120,6 +120,9 @@ size_t sysroot_len = 8;
 /* If set (the default), do 'umount-all' when performing autosync. */
 int autosync_umount = 1;
 
+/* If set, we are testing the daemon as part of the libguestfs tests. */
+int test_mode = 0;
+
 /* Not used explicitly, but required by the gnulib 'error' module. */
 const char *program_name = "guestfsd";
 
@@ -136,10 +139,11 @@ usage (void)
 int
 main (int argc, char *argv[])
 {
-  static const char *options = "rv?";
+  static const char *options = "rtv?";
   static const struct option long_options[] = {
     { "cmdline", 1, 0, 0 },
     { "help", 0, 0, '?' },
+    { "test", 0, 0, 't' },
     { "verbose", 0, 0, 'v' },
     { 0, 0, 0, 0 }
   };
@@ -204,6 +208,11 @@ main (int argc, char *argv[])
       autosync_umount = 0;
       break;
 
+      /* Undocumented --test option used for testing guestfsd. */
+    case 't':
+      test_mode = 1;
+      break;
+
     case 'v':
       verbose = 1;
       break;
@@ -258,7 +267,8 @@ main (int argc, char *argv[])
    * environment is essentially empty.
    * https://bugzilla.redhat.com/show_bug.cgi?id=502074#c5
    */
-  setenv ("PATH", "/sbin:/usr/sbin:/bin:/usr/bin", 1);
+  if (!test_mode)
+    setenv ("PATH", "/sbin:/usr/sbin:/bin:/usr/bin", 1);
   setenv ("SHELL", "/bin/sh", 1);
   setenv ("LC_ALL", "C", 1);
   setenv ("TERM", "dumb", 1);
@@ -277,7 +287,8 @@ main (int argc, char *argv[])
   /* Make a private copy of /etc/lvm so we can change the config (see
    * daemon/lvm-filter.c).
    */
-  copy_lvm ();
+  if (!test_mode)
+    copy_lvm ();
 
   /* Connect to virtio-serial channel. */
   char *channel, *p;
-- 
2.3.1

This option, used for testing, causes the daemon to create the Unix
domain socket (from guestfs_channel), listen on it, and accept a
single connection.
---
 daemon/guestfsd.c   | 65 ++++++++++++++++++++++++++++++++++++++---------------
 daemon/guestfsd.pod |  9 ++++++++
 2 files changed, 56 insertions(+), 18 deletions(-)

diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
index 522c51a..2fd17cc 100644
--- a/daemon/guestfsd.c
+++ b/daemon/guestfsd.c
@@ -139,10 +139,11 @@ usage (void)
 int
 main (int argc, char *argv[])
 {
-  static const char *options = "rtv?";
+  static const char *options = "lrtv?";
   static const struct option long_options[] = {
     { "cmdline", 1, 0, 0 },
     { "help", 0, 0, '?' },
+    { "listen", 0, 0, 'l' },
     { "test", 0, 0, 't' },
     { "verbose", 0, 0, 'v' },
     { 0, 0, 0, 0 }
@@ -150,6 +151,7 @@ main (int argc, char *argv[])
   int c;
   char *cmdline = NULL;
   int option_index;
+  int listen_mode = 0;
 
   ignore_value (chdir ("/"));
 
@@ -199,6 +201,10 @@ main (int argc, char *argv[])
                long_options[option_index].name, option_index);
       break;
 
+    case 'l':
+      listen_mode = 1;
+      break;
+
       /* The -r flag is used when running standalone.  It changes
        * several aspects of the daemon.
        */
@@ -306,23 +312,46 @@ main (int argc, char *argv[])
   if (verbose)
     printf ("trying to open virtio-serial channel '%s'\n",
channel);
 
-  int sock = open (channel, O_RDWR|O_CLOEXEC);
-  if (sock == -1) {
-    fprintf (stderr,
-             "\n"
-             "Failed to connect to virtio-serial channel.\n"
-             "\n"
-             "This is a fatal error and the appliance will now
exit.\n"
-             "\n"
-             "Usually this error is caused by either QEMU or the
appliance\n"
-             "kernel not supporting the vmchannel method that the\n"
-             "libguestfs library chose to use.  Please run\n"
-             "'libguestfs-test-tool' and provide the complete,
unedited\n"
-             "output to the libguestfs developers, either in a bug
report\n"
-             "or on the libguestfs redhat com mailing list.\n"
-             "\n");
-    perror (channel);
-    exit (EXIT_FAILURE);
+  int sock;
+  if (!listen_mode) {
+    sock = open (channel, O_RDWR|O_CLOEXEC);
+    if (sock == -1) {
+      fprintf (stderr,
+               "\n"
+               "Failed to connect to virtio-serial channel.\n"
+               "\n"
+               "This is a fatal error and the appliance will now
exit.\n"
+               "\n"
+               "Usually this error is caused by either QEMU or the
appliance\n"
+               "kernel not supporting the vmchannel method that
the\n"
+               "libguestfs library chose to use.  Please run\n"
+               "'libguestfs-test-tool' and provide the complete,
unedited\n"
+               "output to the libguestfs developers, either in a bug
report\n"
+               "or on the libguestfs redhat com mailing list.\n"
+               "\n");
+      perror (channel);
+      exit (EXIT_FAILURE);
+    }
+  }
+  else {
+    struct sockaddr_un addr;
+
+    sock = socket (AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0);
+    if (sock == -1)
+      error (EXIT_FAILURE, errno, "socket");
+    addr.sun_family = AF_UNIX;
+    strncpy (addr.sun_path, channel, UNIX_PATH_MAX);
+    addr.sun_path[UNIX_PATH_MAX-1] = '\0';
+
+    if (bind (sock, (struct sockaddr *) &addr, sizeof addr) == -1)
+      error (EXIT_FAILURE, errno, "bind: %s", channel);
+
+    if (listen (sock, 4) == -1)
+      error (EXIT_FAILURE, errno, "listen");
+
+    sock = accept4 (sock, NULL, NULL, SOCK_CLOEXEC);
+    if (sock == -1)
+      error (EXIT_FAILURE, errno, "accept");
   }
 
   /* If it's a serial-port like device then it probably has echoing
diff --git a/daemon/guestfsd.pod b/daemon/guestfsd.pod
index 96bbbd6..dbfbfc4 100644
--- a/daemon/guestfsd.pod
+++ b/daemon/guestfsd.pod
@@ -56,6 +56,15 @@ This option is used for testing.  Instead of reading the
Linux kernel
 command line from F</proc/cmdline>, use the value given on the
 program's command line.
 
+=item B<-l>
+
+=item B<--listen>
+
+Instead of opening the C<guestfs_channel> and thus expecting that it
+already exists, create the channel as a Unix domain socket, listen on
+it, and accept a single connection.  This is mainly used for testing
+the daemon.
+
 =item B<-r>
 
 Set the root filesystem to be F</> (instead of the default which is
-- 
2.3.1

This allows us to test the daemon running as a host process, allowing
us to meaningfully test it using valgrind.

This commit only adds a single test that check that the daemon starts
up, can be pinged, and exits.
---
 .gitignore                        |   1 +
 Makefile.am                       |   1 +
 configure.ac                      |   2 +
 tests/daemon/Makefile.am          |  36 +++++++++++
 tests/daemon/captive-daemon.pm.in | 126 ++++++++++++++++++++++++++++++++++++++
 tests/daemon/test-daemon-start.pl |  35 +++++++++++
 6 files changed, 201 insertions(+)
 create mode 100644 tests/daemon/Makefile.am
 create mode 100644 tests/daemon/captive-daemon.pm.in
 create mode 100755 tests/daemon/test-daemon-start.pl

diff --git a/.gitignore b/.gitignore
index 6f14915..6089122 100644
--- a/.gitignore
+++ b/.gitignore
@@ -528,6 +528,7 @@ Makefile.in
 /tests/data/lib-i586.so.xz
 /tests/data/test-grep.txt.gz
 /tests/data/test.iso
+/tests/daemon/captive-daemon.pm
 /tests/disks/test-qemu-drive-libvirt.xml
 /tests/events/test-libvirt-auth-callbacks
 /tests/guests/blank-*.img
diff --git a/Makefile.am b/Makefile.am
index ad6d9d3..c545ea1 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -30,6 +30,7 @@ SUBDIRS += tests/data generator src examples po
 
 if ENABLE_DAEMON
 SUBDIRS += daemon
+SUBDIRS += tests/daemon
 endif
 if ENABLE_APPLIANCE
 SUBDIRS += appliance
diff --git a/configure.ac b/configure.ac
index 8775c4b..66ac5df 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1775,6 +1775,8 @@ AC_CONFIG_FILES([Makefile
                  tests/charsets/Makefile
                  tests/create/Makefile
                  tests/data/Makefile
+                 tests/daemon/Makefile
+                 tests/daemon/captive-daemon.pm
                  tests/discard/Makefile
                  tests/disks/Makefile
                  tests/disks/test-qemu-drive-libvirt.xml
diff --git a/tests/daemon/Makefile.am b/tests/daemon/Makefile.am
new file mode 100644
index 0000000..0d2f1cd
--- /dev/null
+++ b/tests/daemon/Makefile.am
@@ -0,0 +1,36 @@
+# libguestfs
+# Copyright (C) 2015 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., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# Tests in this subdirectory run the daemon as a captive host process
+# and send commands directly to it.  There is a small Perl library
+# called 'captive-daemon.pm' in this directory to help with this.
+
+include $(top_srcdir)/subdir-rules.mk
+
+check_DATA = captive-daemon.pm
+
+TESTS = \
+	test-daemon-start.pl
+
+TESTS_ENVIRONMENT = $(top_builddir)/run --test $(VG)
+
+EXTRA_DIST = \
+	captive-daemon.pm \
+	$(TESTS)
+
+check-valgrind:
+	$(MAKE) check TEST_WITH_VALGRIND=1
diff --git a/tests/daemon/captive-daemon.pm.in
b/tests/daemon/captive-daemon.pm.in
new file mode 100644
index 0000000..22e8baa
--- /dev/null
+++ b/tests/daemon/captive-daemon.pm.in
@@ -0,0 +1,126 @@
+# libguestfs
+# Copyright (C) 2015 Red Hat Inc.
+# @configure_input@
+#
+# 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., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# Small library to help testing the daemon directly.
+
+package CaptiveDaemon;
+
+use strict;
+use warnings;
+
+use Sys::Guestfs;
+
+$| = 1;
+
+# Filled in by autoconf.
+my %var;
+$var{top_builddir} = "@top_builddir@";
+$var{abs_top_srcdir} = "@abs_top_srcdir@";
+$var{abs_top_builddir} = "@abs_top_builddir@";
+$var{VALGRIND} = "@VALGRIND@";
+
+# Now we have to substitute the above variables into this one:
+my $VG = '@VG@';
+$VG =~ s/\$\(([A-Za-z_]+)\)/ $var{"$1"} /ge;
+
+# Refuse to run if the user is trying to run tests as root.  There's
+# too much risk that things will go badly wrong.
+if ($> == 0) {
+    print "$0: don't run the libguestfs tests as root!\n";
+    exit 77
+}
+
+sub run_tests {
+    my $g = Sys::Guestfs->new();
+    my $tmpdir = $g->get_tmpdir;
+    my $verbose = $g->get_verbose;
+    $g->close;
+
+    # Choose a random name for the socket.  The daemon will create the
+    # socket so we don't need to do that.
+    my @chars = ("A".."Z", "a".."z",
"0".."9");
+    my $sockname = $tmpdir;
+    $sockname .= "/";
+    $sockname .= $chars[rand @chars] for 1..8;
+
+    # Assemble the command we will run in the subprocess.
+    my $cmd +        "$var{top_builddir}/daemon/guestfsd" .
+        " --cmdline=guestfs_channel=$sockname -r -t -l";
+    if ($verbose) {
+        $cmd = $cmd . " -v"
+    }
+    if ($ENV{TEST_WITH_VALGRIND}) {
+        $cmd = $VG . " " . $cmd
+    }
+
+    if ($verbose) {
+        print "$0: running: $cmd\n";
+    }
+
+    # Fork to run the daemon in the background.
+    my $pid = fork ();
+    die "fork: $!" unless defined $pid;
+    if ($pid == 0) {
+        # Child process: the daemon.
+        exec $cmd or die "guestfsd: $!";
+    }
+
+    # Wait for the daemon to create the socket, but if it doesn't
+    # appear after a short timeout, assume there has been a failure.
+    for (my $i = 0; $i < 10; ++$i) {
+        last if -S $sockname;
+        sleep 1;
+    }
+    die "subprocess did not create the socket, check earlier
messages\n"
+        unless -S $sockname;
+
+    # Create the libguestfs handle and connect to the daemon using
+    # libguestfs live.
+    $g = Sys::Guestfs->new ();
+    $g->set_backend ("unix:" . $sockname);
+    $g->launch;
+
+    # Run the user tests.
+    my $r = ::tests ($g);
+
+    # Close the socket.  The daemon should now exit.
+    $g->shutdown ();
+    $g->close ();
+    unlink $sockname;
+
+    waitpid ($pid, 0) or die "waitpid: $!";
+    if ($? != 0) {
+        my $signal = $? & 127;
+        die "ERROR: guestfsd died on signal $signal\n" if $signal;
+        my $crash = $? & 128;
+        die "ERROR: guestfsd core dumped\n" if $crash;
+        my $status = $? >> 8;
+        die "ERROR: guestfsd died with exit code 119 (valgrind
failure)\n"
+            if $status == 119;
+
+        # Note we allow guestfsd to die with exit code 1, because
+        # that indicates a read failure from the socket.
+        die "ERROR: guestfsd died with exit code $status\n" if
$status > 1;
+    }
+
+    # Exit with failure if the user test failed.
+    exit 1 unless $r
+}
+
+1;
diff --git a/tests/daemon/test-daemon-start.pl
b/tests/daemon/test-daemon-start.pl
new file mode 100755
index 0000000..91e008a
--- /dev/null
+++ b/tests/daemon/test-daemon-start.pl
@@ -0,0 +1,35 @@
+#!/usr/bin/perl -w
+# libguestfs
+# Copyright (C) 2015 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., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# Test that the daemon starts and stops.
+
+use strict;
+use warnings;
+
+require 'captive-daemon.pm';
+
+sub tests {
+    my $g = shift;
+
+    $g->ping_daemon;
+
+    # Return true to indicate the test succeeded.
+    1;
+}
+
+CaptiveDaemon::run_tests ()
-- 
2.3.1

---
 daemon/btrfs.c             |  3 +-
 tests/daemon/Makefile.am   |  3 +-
 tests/daemon/test-btrfs.pl | 68 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 2 deletions(-)
 create mode 100755 tests/daemon/test-btrfs.pl

diff --git a/daemon/btrfs.c b/daemon/btrfs.c
index 69e306b..7e29e9d 100644
--- a/daemon/btrfs.c
+++ b/daemon/btrfs.c
@@ -42,7 +42,8 @@ GUESTFSD_EXT_CMD(str_btrfsimage, btrfs-image);
 int
 optgroup_btrfs_available (void)
 {
-  return prog_exists (str_btrfs) && filesystem_available
("btrfs") > 0;
+  return test_mode ||
+    (prog_exists (str_btrfs) && filesystem_available
("btrfs") > 0);
 }
 
 char *
diff --git a/tests/daemon/Makefile.am b/tests/daemon/Makefile.am
index 0d2f1cd..bb380c5 100644
--- a/tests/daemon/Makefile.am
+++ b/tests/daemon/Makefile.am
@@ -24,7 +24,8 @@ include $(top_srcdir)/subdir-rules.mk
 check_DATA = captive-daemon.pm
 
 TESTS = \
-	test-daemon-start.pl
+	test-daemon-start.pl \
+	test-btrfs.pl
 
 TESTS_ENVIRONMENT = $(top_builddir)/run --test $(VG)
 
diff --git a/tests/daemon/test-btrfs.pl b/tests/daemon/test-btrfs.pl
new file mode 100755
index 0000000..b58416d
--- /dev/null
+++ b/tests/daemon/test-btrfs.pl
@@ -0,0 +1,68 @@
+#!/usr/bin/perl -w
+# libguestfs
+# Copyright (C) 2015 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., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# Test that the daemon starts and stops.
+
+use strict;
+use warnings;
+
+use File::Temp qw/tempdir/;
+
+require 'captive-daemon.pm';
+
+# Set $PATH to include directory that will have phony 'btrfs' binary.
+my $bindir = tempdir (CLEANUP => 1);
+$ENV{PATH} = $bindir . ":" . $ENV{PATH};
+
+sub set_btrfs_output {
+    my $output = shift;
+    open BTRFS, ">$bindir/btrfs" or die "$bindir/btrfs:
$!";
+    print BTRFS "#!/bin/sh\n";
+    print BTRFS "cat << '__EOF'\n";
+    print BTRFS $output;
+    print BTRFS "__EOF\n";
+    close BTRFS;
+    chmod 0755, "$bindir/btrfs" or die "chmod: $bindir/btrfs:
$!";
+}
+
+sub tests {
+    my $g = shift;
+
+    # Test btrfs_subvolume_list.
+    my $output = <<EOF;
+ID 256 gen 30 top level 5 path test1
+ID 257 gen 30 top level 5 path dir/test2
+ID 258 gen 30 top level 5 path test3
+EOF
+    set_btrfs_output ($output);
+    my @r = $g->btrfs_subvolume_list ("/");
+    die unless $r[0]->{btrfssubvolume_id} == 256;
+    die unless $r[0]->{btrfssubvolume_top_level_id} == 5;
+    die unless $r[0]->{btrfssubvolume_path} eq "test1";
+    die unless $r[1]->{btrfssubvolume_id} == 257;
+    die unless $r[1]->{btrfssubvolume_top_level_id} == 5;
+    die unless $r[1]->{btrfssubvolume_path} eq "dir/test2";
+    die unless $r[2]->{btrfssubvolume_id} == 258;
+    die unless $r[2]->{btrfssubvolume_top_level_id} == 5;
+    die unless $r[2]->{btrfssubvolume_path} eq "test3";
+
+    # Return true to indicate the test succeeded.
+    1;
+}
+
+CaptiveDaemon::run_tests ()
-- 
2.3.1

In data martedì 23 giugno 2015 23:49:51, Richard W.M. Jones ha
scritto:
> This allows the Linux kernel command line to be specified on the
> program command line.
What if, instead, we remove the direct /proc/cmdline reading from the
daemon, making the init read the options from it passing them as
options to the daemon?

After all, the options read from it so far are:
(a) guestfs_verbose
(b) guestfs_network
(c) guestfs_channel

(a) is covered already by -v, and a direct (c) could be useful to ease
this patch series (seen in patch 6).

-- 
Pino Toscano

In data martedì 23 giugno 2015 23:49:54, Richard W.M. Jones ha
scritto:
> This allows us to test the daemon running as a host process, allowing
> us to meaningfully test it using valgrind.
> 
> This commit only adds a single test that check that the daemon starts
> up, can be pinged, and exits.
> ---
> [...]
>  .gitignore                        |   1 +
>  Makefile.am                       |   1 +
>  configure.ac                      |   2 +
>  tests/daemon/Makefile.am          |  36 +++++++++++
>  tests/daemon/captive-daemon.pm.in | 126
++++++++++++++++++++++++++++++++++++++
>  tests/daemon/test-daemon-start.pl |  35 +++++++++++
>  6 files changed, 201 insertions(+)
>  create mode 100644 tests/daemon/Makefile.am
>  create mode 100644 tests/daemon/captive-daemon.pm.in
>  create mode 100755 tests/daemon/test-daemon-start.pl
> 
> diff --git a/.gitignore b/.gitignore
> index 6f14915..6089122 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -528,6 +528,7 @@ Makefile.in
>  /tests/data/lib-i586.so.xz
>  /tests/data/test-grep.txt.gz
>  /tests/data/test.iso
> +/tests/daemon/captive-daemon.pm
>  /tests/disks/test-qemu-drive-libvirt.xml
>  /tests/events/test-libvirt-auth-callbacks
>  /tests/guests/blank-*.img
> diff --git a/Makefile.am b/Makefile.am
> index ad6d9d3..c545ea1 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -30,6 +30,7 @@ SUBDIRS += tests/data generator src examples po
>  
>  if ENABLE_DAEMON
>  SUBDIRS += daemon
> +SUBDIRS += tests/daemon
>  endif
>  if ENABLE_APPLIANCE
>  SUBDIRS += appliance
> diff --git a/configure.ac b/configure.ac
> index 8775c4b..66ac5df 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1775,6 +1775,8 @@ AC_CONFIG_FILES([Makefile
>                   tests/charsets/Makefile
>                   tests/create/Makefile
>                   tests/data/Makefile
> +                 tests/daemon/Makefile
> +                 tests/daemon/captive-daemon.pm
>                   tests/discard/Makefile
>                   tests/disks/Makefile
>                   tests/disks/test-qemu-drive-libvirt.xml
> diff --git a/tests/daemon/Makefile.am b/tests/daemon/Makefile.am
> new file mode 100644
> index 0000000..0d2f1cd
> --- /dev/null
> +++ b/tests/daemon/Makefile.am
> @@ -0,0 +1,36 @@
> +# libguestfs
> +# Copyright (C) 2015 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., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
USA.
> +
> +# Tests in this subdirectory run the daemon as a captive host process
> +# and send commands directly to it.  There is a small Perl library
> +# called 'captive-daemon.pm' in this directory to help with this.
> +
> +include $(top_srcdir)/subdir-rules.mk
> +
> +check_DATA = captive-daemon.pm
> +
> +TESTS = \
> +	test-daemon-start.pl
Should this test be run only when valgrind is present? Or make
captive-daemon.pm exit with 77 if VG is empty/not found?
> +TESTS_ENVIRONMENT = $(top_builddir)/run --test $(VG)
> +
> +EXTRA_DIST = \
> +	captive-daemon.pm \
> +	$(TESTS)
> +
> +check-valgrind:
> +	$(MAKE) check TEST_WITH_VALGRIND=1
> diff --git a/tests/daemon/captive-daemon.pm.in
b/tests/daemon/captive-daemon.pm.in
> new file mode 100644
> index 0000000..22e8baa
> --- /dev/null
> +++ b/tests/daemon/captive-daemon.pm.in
> @@ -0,0 +1,126 @@
> +# libguestfs
> +# Copyright (C) 2015 Red Hat Inc.
> +# @configure_input@
> +#
> +# 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., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
USA.
> +
> +# Small library to help testing the daemon directly.
> +
> +package CaptiveDaemon;
> +
> +use strict;
> +use warnings;
> +
> +use Sys::Guestfs;
> +
> +$| = 1;
> +
> +# Filled in by autoconf.
> +my %var;
> +$var{top_builddir} = "@top_builddir@";
> +$var{abs_top_srcdir} = "@abs_top_srcdir@";
> +$var{abs_top_builddir} = "@abs_top_builddir@";
> +$var{VALGRIND} = "@VALGRIND@";
> +
> +# Now we have to substitute the above variables into this one:
> +my $VG = '@VG@';
> +$VG =~ s/\$\(([A-Za-z_]+)\)/ $var{"$1"} /ge;
> +
> +# Refuse to run if the user is trying to run tests as root.  There's
> +# too much risk that things will go badly wrong.
> +if ($> == 0) {
> +    print "$0: don't run the libguestfs tests as root!\n";
> +    exit 77
> +}
> +
> +sub run_tests {
> [...]
> +    # Run the user tests.
> +    my $r = ::tests ($g);
Minor detail: would it be possible to take as parameter of run_tests
an array of function pointers? This way more tests can be called
together, implementing them in different functions.

-- 
Pino Toscano

In data martedì 23 giugno 2015 23:49:48, Richard W.M. Jones ha
scritto:
> Currently we are unable to properly run guestfsd (the daemon) under
> valgrind.  Attempts to run valgrind inside the appliance have not been
> successful (see patch 1/7).
> 
> However we desperately need better valgrind coverage of the daemon,
> particularly because it is doing a lot of complex parsing of program
> output.  This has been a problem for a long time.
As also discussed on IRC, I will give a try in improving the current
--enable-valgrind-daemon, which works for me although it is a bit
cumbersome.
> A better way to attack this problem is to run the daemon as a regular
> host process and connect libguestfs directly to it.  Because of
> 'libguestfs live' functionality this is quite simple: we are able
to
> connect the library to any Unix domain socket and talk directly to the
> daemon.
> 
> This patch series modified the daemon so it can be run as a standalone
> process listening on a Unix domain socket, adds new tests which run
> the daemon as a host process, and a simple framework to run the daemon
> under valgrind.
For the rest of the series, i.e. patches 2-7, I've left a couple of
notes. Otherwise, the idea is nice and most of the implementation
likewise.

Thanks,
-- 
Pino Toscano