Pino Toscano
2015-Jun-25 09:24 UTC
[Libguestfs] [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
Richard W.M. Jones
2015-Jun-25 13:50 UTC
Re: [Libguestfs] [PATCH] launch: rework handling of --enable-valgrind-daemon
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
Pino Toscano
2015-Jun-25 14:16 UTC
Re: [Libguestfs] [PATCH] launch: rework handling of --enable-valgrind-daemon
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
Reasonably Related Threads
- Re: [PATCH] launch: rework handling of --enable-valgrind-daemon
- Re: [PATCH v2 1/9] build: Remove ./configure --enable-valgrind-daemon.
- [PATCH] launch: rework handling of --enable-valgrind-daemon
- [PATCH v2 0/9] Better testing of the guestfsd daemon.
- Re: [PATCH RFC][Resend] New API: btrfs_convert