While looking at test coverage results, I found some low-hanging fruit. Getting full coverage is outside the scope of this bug: something is better than nothing (and realistically, we won't get full coverage, because some code paths are reached under hard-to-simulate or painfully-slow-to-simulate conditions).
Created attachment 141353 [details] [review] [1/9] dbus-cleanup-sockets: Mark functions noreturn as suggested by clang --- This is not really code coverage, but I was looking at various code quality things in parallel (clang, asan/ubsan, valgrind, coverage) and it doesn't seem worth opening a separate bug for this.
Created attachment 141354 [details] [review] [2/9] bus_connections_foreach, bus_connections_foreach_active: Remove These do not appear in code coverage statistics, and `git grep` reveals that they are unused. --- We can always bring them back from git history if we want them later.
Created attachment 141355 [details] [review] [3/9] bus_connections_get_context: Remove, unused
Created attachment 141356 [details] [review] [4/9] bus_context_get_policy: Remove, unused
Created attachment 141357 [details] [review] [5/9] BusConfigParser test: Check that all limits are equal, not just one
Created attachment 141358 [details] [review] [6/9] containers test: Exercise GetConnectionInstance() on dbus-daemon itself This is an easy bit of missing test coverage detected by running the test suite with gcov.
Created attachment 141359 [details] [review] [7/9] bus_config_parser_check_doctype: Remove, unused We have never checked the <!DOCTYPE> of busconfig XML since the libxml parser was removed in 2013, and the libxml parser was broken before that anyway. The recommended Expat parser (our only parser since 2013) does not appear to have ever validated this, so now does not seem like the time to start. Just ignore the <!DOCTYPE> if there is one. (We never validated this particularly strictly anyway; <!DOCTYPE busconfig SYSTEM "http://example.com/bees"> would have been treated as perfectly valid.)
Created attachment 141360 [details] [review] [8/9] Add more test coverage for config file parsing minimal.conf is a valid config file added to make it obvious why the new invalid config files are invalid. --- There's a lot more low-hanging fruit here if someone wants to go for full coverage, but I got bored at this point. Note that these config files are trusted, so bugs in parsing them are not (in general) a security flaw.
Created attachment 141361 [details] [review] [9/9] containers: Share code for what happens when we lose a connection This improves test coverage, because bus_container_instance_lost_connection() was previously only called when we failed to set up a connection with a server due to OOM, but it is now also called (instead of being duplicated) when we are told to clean up a connection because it has disconnected. To make sure that connections from containers can never cheat their way into being treated as uncontained, do not set their contained_data_slot to NULL. --- I mentioned this on Bug #105656. This one is non-trivial and deserves careful review, unlike the rest of the branch.
Comment on attachment 141353 [details] [review] [1/9] dbus-cleanup-sockets: Mark functions noreturn as suggested by clang Review of attachment 141353 [details] [review]: ----------------------------------------------------------------- r+
Comment on attachment 141354 [details] [review] [2/9] bus_connections_foreach, bus_connections_foreach_active: Remove Review of attachment 141354 [details] [review]: ----------------------------------------------------------------- r+
Comment on attachment 141355 [details] [review] [3/9] bus_connections_get_context: Remove, unused Review of attachment 141355 [details] [review]: ----------------------------------------------------------------- r+
Comment on attachment 141356 [details] [review] [4/9] bus_context_get_policy: Remove, unused Review of attachment 141356 [details] [review]: ----------------------------------------------------------------- r+
Comment on attachment 141357 [details] [review] [5/9] BusConfigParser test: Check that all limits are equal, not just one Review of attachment 141357 [details] [review]: ----------------------------------------------------------------- ha, r+
Comment on attachment 141358 [details] [review] [6/9] containers test: Exercise GetConnectionInstance() on dbus-daemon itself Review of attachment 141358 [details] [review]: ----------------------------------------------------------------- r+
Comment on attachment 141359 [details] [review] [7/9] bus_config_parser_check_doctype: Remove, unused Review of attachment 141359 [details] [review]: ----------------------------------------------------------------- r+
Comment on attachment 141360 [details] [review] [8/9] Add more test coverage for config file parsing Review of attachment 141360 [details] [review]: ----------------------------------------------------------------- r+, a masterpiece ::: test/data/invalid-config-files/apparmor-bad-attribute.conf @@ +1,3 @@ > +<busconfig> > + <listen>tcp:port=1234</listen> > + <apparmor enforcement_style="Judge Dredd"/> ♥♥♥ ::: test/data/invalid-config-files/bad-attribute-2.conf @@ +1,2 @@ > +<busconfig> > + <listen carefully="yes">tcp:port=1234</listen> Were you bored when writing this? ::: test/data/invalid-config-files/bad-element.conf @@ +1,3 @@ > +<busconfig> > + <listen>tcp:port=1234</listen> > + <bees/> -1, unoriginal ::: test/data/invalid-config-files/non-numeric-limit.conf @@ +1,3 @@ > +<busconfig> > + <listen>tcp:port=1234</listen> > + <limit name="max_names_per_connection">bees</limit> They get everywhere, don’t they?
Comment on attachment 141361 [details] [review] [9/9] containers: Share code for what happens when we lose a connection Review of attachment 141361 [details] [review]: ----------------------------------------------------------------- r+, seems reasonable
Thanks, will merge when CI completes.
Fixed in git for 1.13.8
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.