Bug 88808

Created attachment 112837 [details] [review]
[PATCH 1/4] socket_set_epoll_add: initialize all bytes of struct  epoll_event

Created attachment 112838 [details] [review]
[PATCH 2/4] bus: exit on fatal errors even if not syslogging

Created attachment 112839 [details] [review]
[PATCH 3/4] lcov: use builddir, not srcdir

It seems lcov (or gcc?) has changed its paths since last time this
worked.

Created attachment 112840 [details] [review]
[PATCH 4/4] Enable subdir-objects Automake option

It has been supported since at least 1.10, and its absence is
deprecated since 1.14.

Comment on attachment 112837 [details] [review]
[PATCH 1/4] socket_set_epoll_add: initialize all bytes of struct  epoll_event

Review of attachment 112837 [details] [review]:
-----------------------------------------------------------------

What about socket_set_epoll_enable() and socket_set_epoll_disable()? Probably also socket_set_epoll_remove() since it only initialises the first member of the struct epoll_event. Also main().

Comment on attachment 112838 [details] [review]
[PATCH 2/4] bus: exit on fatal errors even if not syslogging

Review of attachment 112838 [details] [review]:
-----------------------------------------------------------------

++

Comment on attachment 112839 [details] [review]
[PATCH 3/4] lcov: use builddir, not srcdir

Review of attachment 112839 [details] [review]:
-----------------------------------------------------------------

No idea what’s changed, but ++ from me if this fixes the build. Although perhaps we should bump the LCOV dependency? Or, in fact, add one to configure.ac in an AX_CHECK_TOOL() macro? See what AX_CODE_COVERAGE does (perhaps we should be using that instead, for consistency?): http://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=blob_plain;f=m4/ax_code_coverage.m4

Comment on attachment 112840 [details] [review]
[PATCH 4/4] Enable subdir-objects Automake option

Review of attachment 112840 [details] [review]:
-----------------------------------------------------------------

++

(In reply to Philip Withnall from comment #5)
> What about socket_set_epoll_enable() and socket_set_epoll_disable()?
> Probably also socket_set_epoll_remove() since it only initialises the first
> member of the struct epoll_event. Also main().

A fair point. The test I was looking at didn't seem to exercise those.

(In reply to Philip Withnall from comment #7)
> No idea what’s changed, but ++ from me if this fixes the build. Although
> perhaps we should bump the LCOV dependency? Or, in fact, add one to
> configure.ac in an AX_CHECK_TOOL() macro? See what AX_CODE_COVERAGE does
> (perhaps we should be using that instead, for consistency?)

I'd rather land the obvious fix, then switch to AX_CODE_COVERAGE some other time. Bug #88922.

Comment on attachment 112838 [details] [review]
[PATCH 2/4] bus: exit on fatal errors even if not syslogging

fixed in git for 1.9.8

Comment on attachment 112839 [details] [review]
[PATCH 3/4] lcov: use builddir, not srcdir

fixed in git for 1.9.8

Comment on attachment 112840 [details] [review]
[PATCH 4/4] Enable subdir-objects Automake option

fixed in git for 1.9.8

Created attachment 129821 [details] [review]
dbus-socket-set-epoll: initialize all bytes of struct epoll_event

This should be a no-op, but it shuts Valgrind up.

The reason for the warning is that we fill in event.events and
event.data.fd, but the union event.data actually contains more bytes
than that. We'll get the same partially initialized union back from the
kernel in socket_set_epoll_poll(), where we take events[i].data.fd and
ignore the rest. So the current code is safe, but valgrind is right to
worry.

(In reply to Philip Withnall from comment #14)
> Created attachment 129821 [details] [review] [review]
> dbus-socket-set-epoll: initialize all bytes of struct epoll_event
> 
> This should be a no-op, but it shuts Valgrind up.
> 
> The reason for the warning is that we fill in event.events and
> event.data.fd, but the union event.data actually contains more bytes
> than that. We'll get the same partially initialized union back from the
> kernel in socket_set_epoll_poll(), where we take events[i].data.fd and
> ignore the rest. So the current code is safe, but valgrind is right to
> worry.

This is an updated version of Simon’s patch from comment #1, which covers all the instances of epoll_event I could find. It quells the Valgrind warnings I was seeing when running test-bus as per bug #99839.

(In reply to Philip Withnall from comment #15)
(In reply to Philip Withnall from comment #14)
> Created attachment 129821 [details] [review]
> dbus-socket-set-epoll: initialize all bytes of struct epoll_event

Looks good.

I'm going to credit you as the author though - you ended up doing rather more in that commit than I did :-)

Fixed in git for 1.11.12