[Spice-devel] [PATCH RFC] make code more aware of Valgrind

[Frediano Ziglio]

Hi,
  more than a patch review I'd like to start (another?)
discussion about some changes in the code.

This patch add some code to make valgrind work better.

The main issue is that we don't want code like this in
production. It simply does make no sense.

These kind of stuff are usually called code instrumentation.
Simply filling code with code that check itself or help
to do it.

In other projects I use instrumentation quite a lot and helps
avoiding lot of problems like having unexpected states.

One of the big issue of this patch is actually the configure
flag used to enable this code. It's correct to have a flag
to enable it as users should not use it however is not
generic enough. I think we should have a generic flag that
allows to enable such instrumentations.

Specific code that should go if this possible flag is enabled:
- if you have a buffer with a "pos" and "len" field
  pos should be 0 <= pos <= len;
- if you have a counter of items in a list this should be
  the number of items present in a list;
- the "prev" pointer of a list item should be the item
  which has this item as "next";
... well I think you get the idea, check even simply stuff but
which could be quite expensive (for instance the list checks
require O(n) which could easily decrease the speed of any
application if done for instance for each insertion in the list).
These are just examples and they may seem a bit useless but
from my experience they are quite useful.

Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
---
 configure.ac          |  8 ++++++++
 server/Makefile.am    |  1 +
 server/dispatcher.c   |  2 ++
 server/red-memcheck.h | 38 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 49 insertions(+)
 create mode 100644 server/red-memcheck.h

diff --git a/configure.ac b/configure.ac
index c743875..72eaf0b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -74,6 +74,14 @@ AC_ARG_ENABLE([automated_tests],
 AS_IF([test x"$enable_automated_tests" != "xno"], [enable_automated_tests="yes"])
 AM_CONDITIONAL(HAVE_AUTOMATED_TESTS, test "x$enable_automated_tests" != "xno")
 
+AC_ARG_ENABLE([valgrind-fixes],
+              AS_HELP_STRING([--enable-valgrind-fixes], [Add some code to make Valgrind work better]),,
+              [enable_valgrind_fixes=no])
+if test "$enable_valgrind_fixes" = yes; then
+    AC_CHECK_HEADERS([valgrind/memcheck.h])
+    AC_DEFINE(ENABLE_VALGRIND_FIXES, [1], [Define to enable code to make Vagrind work better])
+fi
+
 SPICE_CHECK_LZ4
 SPICE_CHECK_SASL
 
diff --git a/server/Makefile.am b/server/Makefile.am
index cca3b9b..41bca68 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -94,6 +94,7 @@ libserver_la_SOURCES =				\
 	red-channel.c				\
 	red-channel.h				\
 	red-common.h				\
+	red-memcheck.h				\
 	dispatcher.c				\
 	dispatcher.h				\
 	red-qxl.c				\
diff --git a/server/dispatcher.c b/server/dispatcher.c
index 8c55881..39f983a 100644
--- a/server/dispatcher.c
+++ b/server/dispatcher.c
@@ -31,6 +31,7 @@
 #include <common/mem.h>
 #include <common/spice_common.h>
 #include "dispatcher.h"
+#include "red-memcheck.h"
 
 //#define DEBUG_DISPATCHER
 
@@ -330,6 +331,7 @@ void dispatcher_send_message(Dispatcher *dispatcher, uint32_t message_type,
                    message_type);
         goto unlock;
     }
+    SPICE_MAKE_MEM_DEFINED_IF_ADDRESSABLE(payload, msg->size);
     if (write_safe(send_fd, payload, msg->size) == -1) {
         spice_printerr("error: failed to send message body for message %d",
                    message_type);
diff --git a/server/red-memcheck.h b/server/red-memcheck.h
new file mode 100644
index 0000000..2183314
--- /dev/null
+++ b/server/red-memcheck.h
@@ -0,0 +1,38 @@
+/*
+   Copyright (C) 2016 Red Hat, Inc.
+
+   This library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   This library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with this library; if not, see <http://www.gnu.org/licenses/>.
+*/
+
+/*
+ * Defines some macros for memory check debugging
+ */
+
+#ifndef RED_MEMCHECK_H_
+#define RED_MEMCHECK_H_
+
+#if ENABLE_VALGRIND_FIXES && HAVE_VALGRIND_MEMCHECK_H
+#include <valgrind/memcheck.h>
+
+#define SPICE_MAKE_MEM_DEFINED_IF_ADDRESSABLE(addr,len) \
+    VALGRIND_MAKE_MEM_DEFINED_IF_ADDRESSABLE(addr,len)
+
+#else
+
+#define SPICE_MAKE_MEM_DEFINED_IF_ADDRESSABLE(addr,len) \
+    do {} while(0)
+
+#endif
+
+#endif
-- 
2.7.4