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

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

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#L346-L364

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.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

In data giovedì 25 giugno 2015 15:54:56, Richard W.M. Jones ha
scritto:
> 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#L346-L364
> 
> 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.
Thanks for the explanations.

I'm trying to get a better valgrind mode, using the actual
implementation as starting point, and surely knowing about the current
shortcomings will help in that.

-- 
Pino Toscano