Bug 88808

Summary: some easy patches for dbus
Product: dbus Reporter: Simon McVittie <smcv>
Component: coreAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: D-Bus Maintainers <dbus>
Severity: enhancement    
Priority: medium CC: bugzilla
Version: git masterKeywords: patch
Hardware: Other   
OS: All   
Whiteboard: review+
i915 platform: i915 features:
Attachments: [PATCH 1/4] socket_set_epoll_add: initialize all bytes of struct epoll_event
[PATCH 2/4] bus: exit on fatal errors even if not syslogging
[PATCH 3/4] lcov: use builddir, not srcdir
[PATCH 4/4] Enable subdir-objects Automake option
dbus-socket-set-epoll: initialize all bytes of struct epoll_event

Description Simon McVittie 2015-01-26 20:23:40 UTC
Here is some misc that I fixed while dealing with other bugs and writing more test cases.
Comment 1 Simon McVittie 2015-01-26 20:24:24 UTC
Created attachment 112837 [details] [review]
[PATCH 1/4] socket_set_epoll_add: initialize all bytes of struct  epoll_event
Comment 2 Simon McVittie 2015-01-26 20:24:38 UTC
Created attachment 112838 [details] [review]
[PATCH 2/4] bus: exit on fatal errors even if not syslogging
Comment 3 Simon McVittie 2015-01-26 20:24:55 UTC
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.
Comment 4 Simon McVittie 2015-01-26 20:25:14 UTC
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 5 Philip Withnall 2015-02-02 14:54:54 UTC
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 6 Philip Withnall 2015-02-02 14:57:01 UTC
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 7 Philip Withnall 2015-02-02 14:59:47 UTC
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 8 Philip Withnall 2015-02-02 15:00:55 UTC
Comment on attachment 112840 [details] [review]
[PATCH 4/4] Enable subdir-objects Automake option

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

++
Comment 9 Simon McVittie 2015-02-02 17:49:34 UTC
(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.
Comment 10 Simon McVittie 2015-02-02 17:50:17 UTC
(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 11 Simon McVittie 2015-02-02 18:39:47 UTC
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 12 Simon McVittie 2015-02-02 18:40:42 UTC
Comment on attachment 112839 [details] [review]
[PATCH 3/4] lcov: use builddir, not srcdir

fixed in git for 1.9.8
Comment 13 Simon McVittie 2015-02-02 18:41:57 UTC
Comment on attachment 112840 [details] [review]
[PATCH 4/4] Enable subdir-objects Automake option

fixed in git for 1.9.8
Comment 14 Philip Withnall 2017-02-22 10:58:54 UTC
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.
Comment 15 Philip Withnall 2017-02-22 10:59:53 UTC
(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.
Comment 16 Simon McVittie 2017-02-22 11:02:21 UTC
(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.
Comment 17 Simon McVittie 2017-02-22 11:08:12 UTC
I'm going to credit you as the author though - you ended up doing rather more in that commit than I did :-)
Comment 18 Simon McVittie 2017-02-22 11:23:08 UTC
Fixed in git for 1.11.12

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.