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