Bug 107739

Summary: Improve test coverage
Product: dbus Reporter: Simon McVittie <smcv>
Component: coreAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: D-Bus Maintainers <dbus>
Severity: enhancement    
Priority: medium Keywords: patch
Version: git master   
Hardware: Other   
OS: All   
URL: https://github.com/smcv/dbus/commits/coverage-107739
Whiteboard: review+
i915 platform: i915 features:
Attachments: [1/9] dbus-cleanup-sockets: Mark functions noreturn as suggested by clang
[2/9] bus_connections_foreach, bus_connections_foreach_active: Remove
[3/9] bus_connections_get_context: Remove, unused
[4/9] bus_context_get_policy: Remove, unused
[5/9] BusConfigParser test: Check that all limits are equal, not just one
[6/9] containers test: Exercise GetConnectionInstance() on dbus-daemon itself
[7/9] bus_config_parser_check_doctype: Remove, unused
[8/9] Add more test coverage for config file parsing
[9/9] containers: Share code for what happens when we lose a connection

Description Simon McVittie 2018-08-29 18:18:18 UTC
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).
Comment 1 Simon McVittie 2018-08-29 18:19:38 UTC
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.
Comment 2 Simon McVittie 2018-08-29 18:20:10 UTC
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.
Comment 3 Simon McVittie 2018-08-29 18:20:27 UTC
Created attachment 141355 [details] [review]
[3/9] bus_connections_get_context: Remove, unused
Comment 4 Simon McVittie 2018-08-29 18:20:45 UTC
Created attachment 141356 [details] [review]
[4/9] bus_context_get_policy: Remove, unused
Comment 5 Simon McVittie 2018-08-29 18:21:04 UTC
Created attachment 141357 [details] [review]
[5/9] BusConfigParser test: Check that all limits are equal, not just one
Comment 6 Simon McVittie 2018-08-29 18:21:35 UTC
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.
Comment 7 Simon McVittie 2018-08-29 18:22:22 UTC
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.)
Comment 8 Simon McVittie 2018-08-29 18:23:35 UTC
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.
Comment 9 Simon McVittie 2018-08-29 18:24:29 UTC
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 10 Philip Withnall 2018-08-30 09:43:55 UTC
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 11 Philip Withnall 2018-08-30 09:45:06 UTC
Comment on attachment 141354 [details] [review]
[2/9] bus_connections_foreach, bus_connections_foreach_active: Remove

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

r+
Comment 12 Philip Withnall 2018-08-30 09:45:27 UTC
Comment on attachment 141355 [details] [review]
[3/9] bus_connections_get_context: Remove, unused

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

r+
Comment 13 Philip Withnall 2018-08-30 09:46:01 UTC
Comment on attachment 141356 [details] [review]
[4/9] bus_context_get_policy: Remove, unused

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

r+
Comment 14 Philip Withnall 2018-08-30 09:47:31 UTC
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 15 Philip Withnall 2018-08-30 09:49:24 UTC
Comment on attachment 141358 [details] [review]
[6/9] containers test: Exercise GetConnectionInstance() on dbus-daemon itself

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

r+
Comment 16 Philip Withnall 2018-08-30 09:56:13 UTC
Comment on attachment 141359 [details] [review]
[7/9] bus_config_parser_check_doctype: Remove, unused

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

r+
Comment 17 Philip Withnall 2018-08-30 09:59:53 UTC
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 18 Philip Withnall 2018-08-30 10:14:14 UTC
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
Comment 19 Simon McVittie 2018-08-30 16:48:12 UTC
Thanks, will merge when CI completes.
Comment 20 Simon McVittie 2018-08-30 17:33:59 UTC
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.