From: "Richard W.M. Jones" <rjones(a)redhat.com&gt; This commit allows you to run the daemon under valgrind. You have to enable it at configure time: ./configure --enable-valgrind-daemon This should *not* be done for production builds. When this feature is enabled, valgrind is added to the appliance and the daemon is run under valgrind. Log messages from valgrind are passed back over a virtio-serial channel into a file called 'valgrind.log.$PID' in the top build directory. Running 'make check', 'make extra-tests' etc causes many valgrind.log.* files to be created which must be examined by hand. --- .gitignore | 1 + appliance/Makefile.am | 9 +++++++-- appliance/init | 10 +++++++++- appliance/packagelist.in | 4 ++++ configure.ac | 15 +++++++++++++++ src/guestfs.c | 8 ++++++++ src/launch.c | 10 ++++++++++ 7 files changed, 54 insertions(+), 3 deletions(-) diff --git a/.gitignore b/.gitignore index b75cafb..fc732bc 100644 --- a/.gitignore +++ b/.gitignore @@ -387,3 +387,4 @@ test-tool/libguestfs-test-tool-helper tools/test.img tools/virt-*.1 tools/virt-*.pl +/valgrind.log.* diff --git a/appliance/Makefile.am b/appliance/Makefile.am index e2e8b96..99896be 100644 --- a/appliance/Makefile.am +++ b/appliance/Makefile.am @@ -42,13 +42,18 @@ make.sh: make.sh.in chmod +x $@-t mv $@-t $@ +PACKAGELIST_CPP_FLAGS = -D$(DISTRO)=1 +if VALGRIND_DAEMON +PACKAGELIST_CPP_FLAGS += -DVALGRIND_DAEMON=1 +endif + packagelist: packagelist.in - cpp -undef -D$(DISTRO)=1 < $< | \ + cpp -undef $(PACKAGELIST_CPP_FLAGS) < $< | \ grep -v '^[[:space:]]*$$' | grep -v '^#' > $@-t mv $@-t $@ excludelist: excludelist.in - cpp -undef -D$(DISTRO)=1 < $< | \ + cpp -undef $(PACKAGELIST_CPP_FLAGS) < $< | \ grep -v '^[[:space:]]*$$' | grep -v '^#' > $@-t mv $@-t $@ diff --git a/appliance/init b/appliance/init index 0f32a55..1d69339 100755 --- a/appliance/init +++ b/appliance/init @@ -106,8 +106,16 @@ if grep -sq guestfs_verbose=1 /proc/cmdline; then fi if ! grep -sq guestfs_rescue=1 /proc/cmdline; then + # Run the daemon under valgrind if ./configure --enable-valgrind-daemon + vg_channel=/dev/virtio-ports/org.libguestfs.valgrind + if [ -w $vg_channel ]; then + exec 3>$vg_channel + vg="valgrind --leak-check=full --log-fd=3 --error-exitcode=119 --max-stackframe=8388608 --child-silent-after-fork=yes" + echo "enabling valgrind: $vg" + fi + # The host will kill qemu abruptly if guestfsd shuts down normally - guestfsd + $vg guestfsd # Otherwise we try to clean up gracefully. For example, this ensures that a # core dump generated by the guest daemon will be written to disk. diff --git a/appliance/packagelist.in b/appliance/packagelist.in index 5735a5a..b42c510 100644 --- a/appliance/packagelist.in +++ b/appliance/packagelist.in @@ -130,3 +130,7 @@ tar xfsprogs #endif zerofree + +#ifdef VALGRIND_DAEMON +valgrind +#endif diff --git a/configure.ac b/configure.ac index 3e364b1..ebc20aa 100644 --- a/configure.ac +++ b/configure.ac @@ -264,8 +264,23 @@ 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]) + AC_DEFINE_UNQUOTED([VALGRIND_LOG_PATH],["$(pwd)"],[Path to save valgrind log files]) + fi 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/src/guestfs.c b/src/guestfs.c index 31968e4..3607eaa 100644 --- a/src/guestfs.c +++ b/src/guestfs.c @@ -190,9 +190,17 @@ guestfs_close (guestfs_h *g) if (g->autosync && g->state == READY) guestfs_internal_autosync (g); + /* If we are valgrinding the daemon, then we *don't* want to kill + * the subprocess because we want the final valgrind messages sent + * when we close sockets below. However for normal production use, + * killing the subprocess is the right thing to do (in case the + * daemon or qemu is not responding). + */ +#ifndef VALGRIND_DAEMON /* Kill the qemu subprocess. */ if (g->state != CONFIG) guestfs_kill_subprocess (g); +#endif /* Run user close callbacks. */ guestfs___call_callbacks_void (g, GUESTFS_EVENT_CLOSE); diff --git a/src/launch.c b/src/launch.c index 1af74b9..4e2fba9 100644 --- a/src/launch.c +++ b/src/launch.c @@ -703,6 +703,16 @@ launch_appliance (guestfs_h *g) add_cmdline (g, "-device"); add_cmdline (g, "virtserialport,chardev=channel0,name=org.libguestfs.channel.0"); +#ifdef VALGRIND_DAEMON + /* Set up virtio-serial channel for valgrind messages. */ + add_cmdline (g, "-chardev"); + snprintf (buf, sizeof buf, "file,path=%s/valgrind.log.%d,id=valgrind", + VALGRIND_LOG_PATH, getpid ()); + add_cmdline (g, buf); + add_cmdline (g, "-device"); + add_cmdline (g, "virtserialport,chardev=valgrind,name=org.libguestfs.valgrind"); +#endif + /* Enable user networking. */ if (g->enable_network) { add_cmdline (g, "-netdev"); -- 1.7.6

From: "Richard W.M. Jones" <rjones(a)redhat.com&gt; --- daemon/initrd.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/daemon/initrd.c b/daemon/initrd.c index 7c4d03f..d9c7050 100644 --- a/daemon/initrd.c +++ b/daemon/initrd.c @@ -108,9 +108,11 @@ do_initrd_cat (const char *path, const char *filename, size_t *size_r) int r = system (cmd); if (r == -1) { reply_with_perror ("command failed: %s", cmd); + free (cmd); rmdir (tmpdir); return NULL; } + free (cmd); if (WEXITSTATUS (r) != 0) { reply_with_perror ("command failed with return code %d", WEXITSTATUS (r)); -- 1.7.6

From: "Richard W.M. Jones" <rjones(a)redhat.com&gt; --- daemon/proto.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/daemon/proto.c b/daemon/proto.c index 4ca2b70..ac62a6b 100644 --- a/daemon/proto.c +++ b/daemon/proto.c @@ -312,7 +312,7 @@ reply (xdrproc_t xdrp, char *ret) char buf[GUESTFS_MESSAGE_MAX]; char lenbuf[4]; struct guestfs_message_header hdr; - unsigned len; + uint32_t len; xdrmem_create (&xdr, buf, sizeof buf, XDR_ENCODE); @@ -352,7 +352,7 @@ reply (xdrproc_t xdrp, char *ret) fprintf (stderr, "guestfsd: xwrite failed\n"); exit (EXIT_FAILURE); } - if (xwrite (sock, buf, len) == -1) { + if (xwrite (sock, buf, (size_t) len) == -1) { fprintf (stderr, "guestfsd: xwrite failed\n"); exit (EXIT_FAILURE); } -- 1.7.6

From: "Richard W.M. Jones" <rjones(a)redhat.com&gt; --- daemon/guestfsd.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c index a28e090..721c169 100644 --- a/daemon/guestfsd.c +++ b/daemon/guestfsd.c @@ -377,7 +377,7 @@ sysroot_path (const char *path) int xwrite (int sock, const void *v_buf, size_t len) { - int r; + ssize_t r; const char *buf = v_buf; while (len > 0) { -- 1.7.6

From: "Richard W.M. Jones" <rjones(a)redhat.com&gt; --- daemon/blkid.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/daemon/blkid.c b/daemon/blkid.c index ada2d7b..8728c29 100644 --- a/daemon/blkid.c +++ b/daemon/blkid.c @@ -188,7 +188,7 @@ blkid_with_p_i_opt (const char *device) free (out); free (err); - free (lines); + free_strings (lines); return ret; @@ -196,7 +196,7 @@ error: free (out); free (err); if (lines) - free (lines); + free_strings (lines); if (ret) free_strings (ret); -- 1.7.6

From: "Richard W.M. Jones" <rjones(a)redhat.com&gt; --- daemon/md.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/daemon/md.c b/daemon/md.c index fea3cf4..f6dc603 100644 --- a/daemon/md.c +++ b/daemon/md.c @@ -297,7 +297,7 @@ do_md_detail(const char *md) free (out); free (err); - free (lines); /* We freed the contained strings when we freed out */ + free_strings (lines); if (add_string (&ret, &size, &alloc, NULL) == -1) return NULL; @@ -307,7 +307,7 @@ error: free (out); free (err); if (lines) - free (lines); + free_strings (lines); if (ret) free_strings (ret); -- 1.7.6

From: "Richard W.M. Jones" <rjones(a)redhat.com&gt; --- daemon/inotify.c | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-) diff --git a/daemon/inotify.c b/daemon/inotify.c index e7ce423..add1f14 100644 --- a/daemon/inotify.c +++ b/daemon/inotify.c @@ -40,6 +40,17 @@ static int inotify_fd = -1; static char inotify_buf[64*1024*1024]; /* Event buffer, [0..posn-1] is valid */ static size_t inotify_posn = 0; +/* Clean up the inotify handle on daemon exit. */ +static void inotify_finalize (void) __attribute__((destructor)); +static void +inotify_finalize (void) +{ + if (inotify_fd >= 0) { + close (inotify_fd); + inotify_fd = -1; + } +} + int optgroup_inotify_available (void) { -- 1.7.6

From: "Richard W.M. Jones" <rjones(a)redhat.com&gt; This commit tidies up the code by splitting out the path element-searching code into a separate function. Valgrind found that 'closedir' frees the 'struct dirent *', which wasn't immediately obvious. So now we do the 'closedir' after all operations which touch 'd->d_name'. --- daemon/realpath.c | 119 ++++++++++++++++++++++++++++++++++++----------------- 1 files changed, 81 insertions(+), 38 deletions(-) diff --git a/daemon/realpath.c b/daemon/realpath.c index c58fc6c..8ec9674 100644 --- a/daemon/realpath.c +++ b/daemon/realpath.c @@ -66,6 +66,8 @@ do_realpath (const char *path) #endif } +static int find_path_element (int fd_cwd, char *name, size_t *name_len_ret); + char * do_case_sensitive_path (const char *path) { @@ -110,57 +112,26 @@ do_case_sensitive_path (const char *path) path += i; /* Read the current directory looking (case insensitively) for - * this element of the path. + * this element of the path. This replaces 'name' with the + * correct case version. */ - int fd2 = dup (fd_cwd); /* because closedir will close it */ - if (fd2 == -1) { - reply_with_perror ("dup"); - goto error; - } - DIR *dir = fdopendir (fd2); - if (dir == NULL) { - reply_with_perror ("opendir"); - goto error; - } - - struct dirent *d = NULL; - - errno = 0; - while ((d = readdir (dir)) != NULL) { - if (STRCASEEQ (d->d_name, name)) - break; - } - - if (d == NULL && errno != 0) { - reply_with_perror ("readdir"); + if (find_path_element (fd_cwd, name, &i) == -1) goto error; - } - - if (closedir (dir) == -1) { - reply_with_perror ("closedir"); - goto error; - } - - if (d == NULL) { - reply_with_error ("%s: no file or directory found with this name", name); - goto error; - } /* Add the real name of this path element to the return value. */ if (next > 1) ret[next++] = '/'; - i = strlen (d->d_name); if (next + i >= PATH_MAX) { reply_with_error ("final path too long"); goto error; } - strcpy (&ret[next], d->d_name); + strcpy (&ret[next], name); next += i; /* Is it a directory? Try going into it. */ - fd2 = openat (fd_cwd, d->d_name, O_RDONLY | O_DIRECTORY); + int fd2 = openat (fd_cwd, name, O_RDONLY | O_DIRECTORY); int err = errno; close (fd_cwd); fd_cwd = fd2; @@ -168,12 +139,12 @@ do_case_sensitive_path (const char *path) if (fd_cwd == -1) { /* ENOTDIR is OK provided we've reached the end of the path. */ if (errno != ENOTDIR) { - reply_with_perror ("openat: %s", d->d_name); + reply_with_perror ("openat: %s", name); goto error; } if (*path) { - reply_with_error ("%s: non-directory element in path", d->d_name); + reply_with_error ("%s: non-directory element in path", name); goto error; } } @@ -196,3 +167,75 @@ do_case_sensitive_path (const char *path) return NULL; } + +/* 'fd_cwd' is a file descriptor pointing to an open directory. + * 'name' is a buffer of NAME_MAX+1 characters in size which initially + * contains the path element to search for. + * + * We search the directory looking for a path element that case + * insensitively matches 'name' and update the 'name' buffer. + * + * If this is successful, return 0. If it fails, reply with an error + * and return -1. + */ +static int +find_path_element (int fd_cwd, char *name, size_t *name_len_ret) +{ + int fd2; + DIR *dir; + struct dirent *d; + + fd2 = dup (fd_cwd); /* because closedir will close it */ + if (fd2 == -1) { + reply_with_perror ("dup"); + return -1; + } + dir = fdopendir (fd2); + if (dir == NULL) { + reply_with_perror ("opendir"); + close (fd2); + return -1; + } + + for (;;) { + errno = 0; + d = readdir (dir); + if (d == NULL) + break; + if (STRCASEEQ (d->d_name, name)) + break; + } + + if (d == NULL && errno != 0) { + reply_with_perror ("readdir"); + closedir (dir); + return -1; + } + + if (d == NULL) { + reply_with_error ("%s: no file or directory found with this name", name); + closedir (dir); + return -1; + } + + *name_len_ret = strlen (d->d_name); + if (*name_len_ret > NAME_MAX) { + /* Should never happen? */ + reply_with_error ("path element longer than NAME_MAX"); + closedir (dir); + return -1; + } + + /* Safe because name is a buffer of NAME_MAX+1 characters. */ + strcpy (name, d->d_name); + + /* NB: closedir frees the structure associated with 'd', so we must + * do this last. + */ + if (closedir (dir) == -1) { + reply_with_perror ("closedir"); + return -1; + } + + return 0; +} -- 1.7.6