Re: [Qemu-devel] [PATCH] Support running QEMU on Valgrind

[Markus Armbruster]

Stefan Weil <address@hidden> writes:

> Am 31.10.2011 07:38, schrieb Markus Armbruster:
>> Alexander Graf <address@hidden> writes:
>>> On 30.10.2011, at 13:07, Stefan Weil wrote:
>>>> Valgrind is a tool which can automatically detect many kinds of bugs.
>>>>
>>>> Running QEMU on Valgrind with x86_64 hosts was not possible because
>>>> Valgrind aborts when memalign is called with an alignment larger than
>>>> 1 MiB. QEMU normally uses 2 MiB on Linux x86_64.
>>>>
>>>> Now the alignment is reduced to the page size when QEMU is running on
>>>> Valgrind.
>>>>
>>>> valgrind.h is a copy from Valgrind svn trunk r12226 with trailing
>>>> whitespace stripped but otherwise unmodified, so it still raises lots
>>>> of errors when checked with scripts/checkpatch.pl.
>>>
>>> Can't we just require valgrind header files to be around when kvm
>>> is enabled? I would rather not copy code from other
>>> projects. Alternatively you could take the header and shrink it
>>> down to maybe 5 lines of inline asm code that would be a lot more
>>> readable :). You're #ifdef'ing on x86_64 already anyways.
>>>
>>>>
>>>> It is included here to avoid a dependency on Valgrind.
>> Our usual way to avoid a hard dependency on something we want is to
>> detect it in configure, then do something like
>>
>> #ifdef CONFIG_VALGRIND
>> #include "valgrind.h"
>> #else
>> #define RUNNING_ON_VALGRIND 0
>> #endif
>>
>> [...]
>
> Markus, you obviously did not read my last mail.
> I know how configure works, so there is no need to teach me.

Hope I didn't offend you; sorry if I did.

> I wrote that I decided against the configure solution because
> it is not adequate here. Adding a copy of valgrind.h which
> is explicitly made for being copied is simpler and better:
>
> * It avoids code in configure. There are already so many
>   checks in configure that it takes a rather long time to run,
>   and additional checks don't improve maintainability.

Configure does what it needs to do.  If it's slow or hard to maintain,
we can discuss how to better solve the problem.  Avoiding to solve parts
of the problem doesn't seem such a good idea, though.

> * It adds Valgrind support for any x86_64 QEMU binary
>   without enforcing a build dependency on Valgrind.
>   This is useful for QEMU packages in distributions.

I can't speak for other distributions, but over here we very much prefer
build dependencies over copies.  Anyone who ever searched a bunch of
packages for copies containing a hot security hole will understand why.

> You said that copies are evil without explaining why you
> think so. What about the other copies in QEMU? There are
> lots of them, and some (e.g. the Linux headers) were added
> recently.

Copies are evil because we need to pick the one version that's right for
all our users.  Repeatedly.

That's okay (sort of) when there's a tight coupling, and there's really
only one admissible version.

Or it may be a lesser evil when the thing copied isn't readily available
(not packaged in common distros).

I can't see either of that for valgrind.h.