Richard W.M. Jones
2015-Jun-23 22:49 UTC
[Libguestfs] [PATCH 0/7] Better testing of the guestfsd daemon.
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.
Richard W.M. Jones
2015-Jun-23 22:49 UTC
[Libguestfs] [PATCH 1/7] build: Remove ./configure --enable-valgrind-daemon.
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
Richard W.M. Jones
2015-Jun-23 22:49 UTC
[Libguestfs] [PATCH 2/7] configure: Remove test for valgrind --vgdb option.
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
Richard W.M. Jones
2015-Jun-23 22:49 UTC
[Libguestfs] [PATCH 3/7] daemon: Add --cmdline option for testing.
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
Richard W.M. Jones
2015-Jun-23 22:49 UTC
[Libguestfs] [PATCH 4/7] daemon: Add undocumented --test / -t option to enable test mode.
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
Richard W.M. Jones
2015-Jun-23 22:49 UTC
[Libguestfs] [PATCH 5/7] daemon: Add -l / --listen flag.
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
Richard W.M. Jones
2015-Jun-23 22:49 UTC
[Libguestfs] [PATCH 6/7] tests: Add tests using a captive daemon process.
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
Richard W.M. Jones
2015-Jun-23 22:49 UTC
[Libguestfs] [PATCH 7/7] tests: Add a test of btrfs-subvolume-list using the captive daemon.
--- 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
Pino Toscano
2015-Jun-24 13:22 UTC
Re: [Libguestfs] [PATCH 3/7] daemon: Add --cmdline option for testing.
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
Pino Toscano
2015-Jun-24 13:51 UTC
Re: [Libguestfs] [PATCH 6/7] tests: Add tests using a captive daemon process.
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.plShould 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
Pino Toscano
2015-Jun-24 13:58 UTC
Re: [Libguestfs] [PATCH 0/7] Better testing of the guestfsd daemon.
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
Maybe Matching Threads
- [PATCH v2 0/9] Better testing of the guestfsd daemon.
- [PATCH] [repost] build: Remove ./configure --enable-valgrind-daemon.
- [PATCH 00/14] Run the daemon under valgrind and fix resultant errors.
- [PATCH] build: Choose a virtual directory for the daemon which is not a symlink.
- [PATCH] Enable running the daemon under valgrind.