Libguestfs - Jun 2015 - Re: [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