Attachment #42466 [details] makes bus-test pass when given fds beyond 2 by the parent process (for instance under gdb). I'm splitting this off because it's not required to fix the bug it's attached to. Colin Walters reviewed it and said: > ::: dbus/dbus-message-private.h > @@ +139,3 @@ > va_list var_args); > > +void _dbus_close_excess_fds(void); > > Would prefer this lived in dbus-sysdeps.h. > > ::: dbus/dbus-message-util.c > @@ +182,3 @@ > + closedir(d); > + } > +#endif > > This needs a non-#ifdef __linux__ implementation (and should probably fall back > if the open of /prof/self/fd fails for some reason too). > > This code is definitely scattered around; look at e.g. > dbus-sysdeps-unix.c:3108 where we use sysconf (_SC_OPEN_MAX) and then a loop. > I think it'd be worth at least refactoring that bit to use this common > function. > > Also I suggest a better name is: > > _dbus_close_nonstandard_fds (); > > Or possibly > > _dbus_close_fds_from (3); ? to which I replied: > (In reply to comment #8) > > This needs a non-#ifdef __linux__ implementation (and should probably fall > back > > if the open of /prof/self/fd fails for some reason too). > > For this purpose, it doesn't need a fallback: it's meant to be an exact mirror > of the check for leaked fds in the same file. Both are #ifdef __linux__ and > only used in the regression tests. > > > _dbus_close_nonstandard_fds (); > > It'd be reasonable to put that function (and perhaps the leak check too) in > sysdeps-util, though, and if placed in that location I agree it should be > cross-platform. Thinking about this again, it'd be better still if we made the check for leaked fds work like this: * remember the initial set of fds * do the test * complain about any fds not found in the set we remembered This would avoid breaking any tools that relied on having bonus fds open for their own purposes; for instance, Valgrind can be told to report to fd 3 if you don't want its output to get interleaved with your stdout and/or stderr.
Created attachment 44447 [details] [review] Don't report file descriptors as "leaked" if they were already open This is necessary to run the regression tests under valgrind (if telling it to output to a dedicated fd), gdb, fakeroot etc. --- This will fail for fds beyond FD_SETSIZE, but if you're using fds outside that range then you're weird. :-)
Comment on attachment 44447 [details] [review] Don't report file descriptors as "leaked" if they were already open Hmm; what about a _DBUS_TESTS_FD_WHITELIST environment variable or something? >+_dbus_check_fdleaks_enter (void) The indentation in this function is inconsistent: >+ if ((d = opendir("/proc/self/fd"))) Space after identifier. Otherwise looks OK.
(In reply to comment #2) > Hmm; what about a _DBUS_TESTS_FD_WHITELIST environment variable or something? Auto-detecting the initial set seems better than having to put it in the environment. (Rationale: I shouldn't have to remember which fd a particular version of gdb/fakeroot likes to use, and compensate for it, if we can pick it up automatically.) > >+_dbus_check_fdleaks_enter (void) > > The indentation in this function is inconsistent: The indentation looks fine to me? I assume you meant "whitespace"? > >+ if ((d = opendir("/proc/self/fd"))) > > Space after identifier. > > Otherwise looks OK. Thanks, I'll fix that and merge.
Created attachment 44668 [details] [review] _dbus_check_fdleaks_enter, _dbus_check_fdleaks_leave: whitespace It turns out the code I converted into _dbus_check_fdleaks_leave (then modified for _dbus_check_fdleaks_enter) consistently got this wrong, so here's a separate patch that fixes both.
(In reply to comment #4) > Created an attachment (id=44668) [details] > _dbus_check_fdleaks_enter, _dbus_check_fdleaks_leave: whitespace > > It turns out the code I converted into _dbus_check_fdleaks_leave (then modified > for _dbus_check_fdleaks_enter) consistently got this wrong, so here's a > separate patch that fixes both. Looks OK.
Fixed in git for 1.4.10, 1.5.2
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.