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

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 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

--- 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:54, Richard W.M. Jones ha scritto: Should this test be run only when valgrind is present? Or make captive-daemon.pm exit with 77 if VG is empty/not found? 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