[Libguestfs] [PATCH v2 1/9] 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. 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