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. This does not remove the guestfs_internal_exit API. That will instead be modified in a future commit. --- appliance/Makefile.am | 7 +------ appliance/guestfsd.suppressions | 27 --------------------------- appliance/init | 20 +------------------- appliance/packagelist.in | 2 -- configure.ac | 14 -------------- generator/actions.ml | 3 +-- src/conn-socket.c | 20 -------------------- src/guestfs-internal.h | 12 ------------ src/handle.c | 15 --------------- src/launch-uml.c | 11 ----------- src/launch.c | 3 --- 11 files changed, 3 insertions(+), 131 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/generator/actions.ml b/generator/actions.ml index 11b8652..24e84b5 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -11955,8 +11955,7 @@ This function is used internally when setting up the appliance." }; 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." }; +This function is used internally when testing the appliance." }; { defaults with name = "copy_attributes"; added = (1, 25, 21); 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

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 eef24d5..8285d27 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,9 +139,10 @@ usage (void) int main (int argc, char *argv[]) { - static const char *options = "rv?"; + static const char *options = "rtv?"; static const struct option long_options[] = { { "help", 0, 0, '?' }, + { "test", 0, 0, 't' }, { "verbose", 0, 0, 'v' }, { 0, 0, 0, 0 } }; @@ -191,6 +195,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; @@ -244,7 +253,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); @@ -263,7 +273,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 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 | 125 ++++++++++++++++++++++++++++++++++++++ tests/daemon/test-daemon-start.pl | 35 +++++++++++ 6 files changed, 200 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..991a9a1 --- /dev/null +++ b/tests/daemon/captive-daemon.pm.in @@ -0,0 +1,125 @@ +# 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 --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

In data giovedì 25 giugno 2015 15:56:59, Richard W.M. Jones ha scritto: An option here could be turning internal_exit from a daemon API into a debug subcommand. It would be undocumented as it should, not leaving it exposed more. Thanks, -- Pino Toscano

--- tests/daemon/test-btrfs.pl | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/daemon/test-btrfs.pl b/tests/daemon/test-btrfs.pl index fff927e..815ab1d 100755 --- a/tests/daemon/test-btrfs.pl +++ b/tests/daemon/test-btrfs.pl @@ -62,6 +62,19 @@ EOF die unless $r[2]->{btrfssubvolume_top_level_id} == 5; die unless $r[2]->{btrfssubvolume_path} eq "test3"; + # Test btrfs_qgroup_show. + my $output = <<EOF; +qgroupid rfer excl +-------- ---- ---- +0/5 4096 4096 +EOF + set_btrfs_output ($output); + my @r = $g->btrfs_qgroup_show ("/"); + die unless @r == 1; + die unless $r[0]->{btrfsqgroup_id} == "0/5"; + die unless $r[0]->{btrfsqgroup_rfer} == 4096; + die unless $r[0]->{btrfsqgroup_excl} == 4096; + # Return true to indicate the test succeeded. 1; } -- 2.3.1