[PATCH] launch: rework handling of --enable-valgrind-daemon
Instead of forcing valgrind to be run when --enable-valgrind-daemon is passed to configure, enable it only when the backend setting "valgrind_daemon" is set. This allows developers to keep building with --enable-valgrind-daemon, which unconditionally adds valgrind in the appliance but using only when requested. When --enable-valgrind-daemon is not passed (typical for release builds), then no valgrind-related code is built at all, and "valgrind_daemon" is ignored. --- src/guestfs-internal.h | 1 + src/launch-direct.c | 4 ++++ src/launch-libvirt.c | 4 ++++ src/launch-uml.c | 14 ++++++++------ src/launch.c | 12 +++++++++--- 5 files changed, 26 insertions(+), 9 deletions(-) diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h index bbd7fb4..d07b61d 100644 --- a/src/guestfs-internal.h +++ b/src/guestfs-internal.h @@ -767,6 +767,7 @@ extern void guestfs_int_print_timestamped_message (guestfs_h *g, const char *fs, extern void guestfs_int_launch_send_progress (guestfs_h *g, int perdozen); extern char *guestfs_int_appliance_command_line (guestfs_h *g, const char *appliance_dev, int flags); #define APPLIANCE_COMMAND_LINE_IS_TCG 1 +#define APPLIANCE_COMMAND_LINE_VALGRIND_DAEMON 2 const char *guestfs_int_get_cpu_model (int kvm); extern void guestfs_int_register_backend (const char *name, const struct backend_ops *); extern int guestfs_int_set_backend (guestfs_h *g, const char *method); diff --git a/src/launch-direct.c b/src/launch-direct.c index ea67ec9..f6e90da 100644 --- a/src/launch-direct.c +++ b/src/launch-direct.c @@ -630,6 +630,10 @@ launch_direct (guestfs_h *g, void *datav, const char *arg) flags = 0; if (!has_kvm || force_tcg) flags |= APPLIANCE_COMMAND_LINE_IS_TCG; +#ifdef VALGRIND_DAEMON + if (guestfs_int_get_backend_setting_bool (g, "valgrind_daemon") > 0) + flags |= APPLIANCE_COMMAND_LINE_VALGRIND_DAEMON; +#endif ADD_CMDLINE_STRING_NODUP (guestfs_int_appliance_command_line (g, appliance_dev, flags)); diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c index f46782c..92f2922 100644 --- a/src/launch-libvirt.c +++ b/src/launch-libvirt.c @@ -1160,6 +1160,10 @@ construct_libvirt_xml_boot (guestfs_h *g, flags = 0; if (!params->data->is_kvm) flags |= APPLIANCE_COMMAND_LINE_IS_TCG; +#ifdef VALGRIND_DAEMON + if (guestfs_int_get_backend_setting_bool (g, "valgrind_daemon") > 0) + flags |= APPLIANCE_COMMAND_LINE_VALGRIND_DAEMON; +#endif cmdline = guestfs_int_appliance_command_line (g, params->appliance_dev, flags); start_element ("os") { diff --git a/src/launch-uml.c b/src/launch-uml.c index 785548c..1d92b5e 100644 --- a/src/launch-uml.c +++ b/src/launch-uml.c @@ -269,12 +269,14 @@ launch_uml (guestfs_h *g, void *datav, const char *arg) #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"); + if (guestfs_int_get_backend_setting_bool (g, "valgrind_daemon") > 0) { + /* 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 diff --git a/src/launch.c b/src/launch.c index fd5479e..bfc2dea 100644 --- a/src/launch.c +++ b/src/launch.c @@ -319,6 +319,9 @@ guestfs_int_appliance_command_line (guestfs_h *g, const char *appliance_dev, char *ret; bool tcg = flags & APPLIANCE_COMMAND_LINE_IS_TCG; char lpj_s[64] = ""; +#ifdef VALGRIND_DAEMON + int valgrind_daemon = flags & APPLIANCE_COMMAND_LINE_VALGRIND_DAEMON; +#endif if (appliance_dev) snprintf (root, sizeof root, " root=%s", appliance_dev); @@ -335,9 +338,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 @@ -363,6 +363,9 @@ guestfs_int_appliance_command_line (guestfs_h *g, const char *appliance_dev, " %s" /* selinux */ "%s" /* verbose */ "%s" /* network */ +#ifdef VALGRIND_DAEMON + "%s" /* valgrind */ +#endif " TERM=%s" /* TERM environment variable */ "%s%s", /* append */ #ifdef __arm__ @@ -373,6 +376,9 @@ guestfs_int_appliance_command_line (guestfs_h *g, const char *appliance_dev, g->selinux ? "selinux=1 enforcing=0" : "selinux=0", g->verbose ? " guestfs_verbose=1" : "", g->enable_network ? " guestfs_network=1" : "", +#ifdef VALGRIND_DAEMON + valgrind_daemon ? " guestfs_valgrind_daemon=1" : "", +#endif term ? term : "linux", g->append ? " " : "", g->append ? g->append : ""); -- 2.1.0
We had a chat about this on IRC, and I'm not very happy about any patch that requires a special ./configure flag. We should find a way to enable this functionality for everyone in all builds, without impacting anyone who doesn't want to use it. I think: - remove the --enable-valgrind-daemon, for reasons outlined above (I posted a patch to do that yesterday) - have a new backend setting, just like you proposed, except it wouldn't be conditional on any configure setting - when (1) the backend setting is true and (2) valgrind is present in the appliance, the init script should run `valgrind guestfsd' - output from valgrind would go to stderr, where it is picked up by the normal verbose output (so we don't need a special socket) This doesn't handle the guestfsd shutdown case (but nor does the current code, which is both racy on shutdown and introduces separate shutdown paths for --enable-valgrind-daemon and ordinary configurations). But we can punt on that until later. The above would detect all memory errors except for memory leaks. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
In data giovedì 25 giugno 2015 14:50:03, Richard W.M. Jones ha scritto:
We had a chat about this on IRC, and I'm not very happy about any patch that requires a special ./configure flag.
I'm not sure where you see any special ./configure flag, other than what is already there (and not used much because makes things cumbersome).
We should find a way to enable this functionality for everyone in all builds, without impacting anyone who doesn't want to use it.
I think:
- remove the --enable-valgrind-daemon, for reasons outlined above (I posted a patch to do that yesterday)
- have a new backend setting, just like you proposed, except it wouldn't be conditional on any configure setting
- when (1) the backend setting is true and (2) valgrind is present in the appliance, the init script should run `valgrind guestfsd'
- output from valgrind would go to stderr, where it is picked up by the normal verbose output (so we don't need a special socket)
This could be okay for me, ...
This doesn't handle the guestfsd shutdown case (but nor does the current code, which is both racy on shutdown and introduces separate shutdown paths for --enable-valgrind-daemon and ordinary configurations). But we can punt on that until later. The above would detect all memory errors except for memory leaks.
... although losing the leaks detection would be a no-go for me, since that's something I've been using from time to time, even if not often. Can you expand a bit more on the parts you consider racy? Thanks, -- Pino Toscano
On Thu, Jun 25, 2015 at 04:16:59PM +0200, Pino Toscano wrote:
In data giovedì 25 giugno 2015 14:50:03, Richard W.M. Jones ha scritto:
This doesn't handle the guestfsd shutdown case (but nor does the current code, which is both racy on shutdown and introduces separate shutdown paths for --enable-valgrind-daemon and ordinary configurations). But we can punt on that until later. The above would detect all memory errors except for memory leaks.
... although losing the leaks detection would be a no-go for me, since that's something I've been using from time to time, even if not often.
Can you expand a bit more on the parts you consider racy?
There are two objections: (1) That we currently have separate shutdown paths in the valgrind/non-valgrind case: https://github.com/libguestfs/libguestfs/blob/master/src/handle.c#L429-L443 https://github.com/libguestfs/libguestfs/blob/master/src/conn-socket.c#L346-... We shouldn't have two different paths that developers and non- developers use. (2) This actually causes bugs. The second one is the race. https://bugzilla.redhat.com/show_bug.cgi?id=1023630 https://bugzilla.redhat.com/show_bug.cgi?id=1020216 There's also another race which I don't think we have a bug about where the output from valgrind is truncated (because valgrind is quite slow at shutdown) so that you don't see valgrind errors reliably. Attempting (not always successfully) to work around that race is the purpose of the sleep here: https://github.com/libguestfs/libguestfs/blob/master/appliance/init#L154 In summary, I think it's a hack. I cannot use it because of the bugs above, which makes it useless for me. Finding memory leaks is certainly important, but crashes in the daemon are way more important. I'm not saying we couldn't fix the memory leak problem later. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
In data giovedì 25 giugno 2015 15:54:56, Richard W.M. Jones ha scritto:
On Thu, Jun 25, 2015 at 04:16:59PM +0200, Pino Toscano wrote:
In data giovedì 25 giugno 2015 14:50:03, Richard W.M. Jones ha scritto:
This doesn't handle the guestfsd shutdown case (but nor does the current code, which is both racy on shutdown and introduces separate shutdown paths for --enable-valgrind-daemon and ordinary configurations). But we can punt on that until later. The above would detect all memory errors except for memory leaks.
... although losing the leaks detection would be a no-go for me, since that's something I've been using from time to time, even if not often.
Can you expand a bit more on the parts you consider racy?
There are two objections:
(1) That we currently have separate shutdown paths in the valgrind/non-valgrind case:
https://github.com/libguestfs/libguestfs/blob/master/src/handle.c#L429-L443 https://github.com/libguestfs/libguestfs/blob/master/src/conn-socket.c#L346-...
We shouldn't have two different paths that developers and non- developers use.
(2) This actually causes bugs. The second one is the race.
https://bugzilla.redhat.com/show_bug.cgi?id=1023630 https://bugzilla.redhat.com/show_bug.cgi?id=1020216
There's also another race which I don't think we have a bug about where the output from valgrind is truncated (because valgrind is quite slow at shutdown) so that you don't see valgrind errors reliably. Attempting (not always successfully) to work around that race is the purpose of the sleep here:
https://github.com/libguestfs/libguestfs/blob/master/appliance/init#L154
In summary, I think it's a hack. I cannot use it because of the bugs above, which makes it useless for me. Finding memory leaks is certainly important, but crashes in the daemon are way more important. I'm not saying we couldn't fix the memory leak problem later.
Thanks for the explanations. I'm trying to get a better valgrind mode, using the actual implementation as starting point, and surely knowing about the current shortcomings will help in that. -- Pino Toscano
participants (2)
-
Pino Toscano -
Richard W.M. Jones