Re: [Libguestfs] [PATCH] launch: rework handling of --enable-valgrind-daemon

On Thu, Jun 25, 2015 at 04:16:59PM +0200, Pino Toscano wrote: > In data giovedì 25 giugno 2015 14:50:03, Richard W.M. Jones ha scritto: > > 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? There are two objections: (1) That we currently have separate shutdown paths in the valgrind/non-valgrind case: https://github.com/libguestfs/libguestfs/blob/master/src/handle.c#L429-L443 https://github.com/libguestfs/libguestfs/blob/master/src/conn-socket.c#L3... We shouldn't have two different paths that developers and non- developers use. (2) This actually causes bugs. The second one is the race. https://bugzilla.redhat.com/show_bug.cgi?id=1023630 https://bugzilla.redhat.com/show_bug.cgi?id=1020216 There's also another race which I don't think we have a bug about where the output from valgrind is truncated (because valgrind is quite slow at shutdown) so that you don't see valgrind errors reliably. Attempting (not always successfully) to work around that race is the purpose of the sleep here: https://github.com/libguestfs/libguestfs/blob/master/appliance/init#L154 In summary, I think it's a hack. I cannot use it because of the bugs above, which makes it useless for me. Finding memory leaks is certainly important, but crashes in the daemon are way more important. I'm not saying we couldn't fix the memory leak problem later.