Merge lp://staging/~ted/dbus-test-runner/set-system-bus into lp://staging/dbus-test-runner/15.04

Proposed by Ted Gould
Status: Merged
Approved by: Ted Gould
Approved revision: 113
Merged at revision: 85
Proposed branch: lp://staging/~ted/dbus-test-runner/set-system-bus
Merge into: lp://staging/dbus-test-runner/15.04
Diff against target: 825 lines (+377/-66)
17 files modified
configure.ac (+2/-2)
data/Makefile.am (+2/-2)
data/system.conf (+42/-0)
debian/changelog (+8/-0)
debian/control (+1/-0)
debian/dbus-test-runner.install (+0/-2)
debian/libdbustest1.install (+1/-0)
debian/libdbustest1.symbols (+49/-0)
debian/rules (+5/-0)
libdbustest/Makefile.am (+1/-0)
libdbustest/dbus-mock.c (+6/-0)
libdbustest/service.c (+131/-58)
libdbustest/service.h (+8/-0)
libdbustest/task.c (+36/-2)
libdbustest/task.h (+4/-0)
src/dbus-test-runner.c (+47/-0)
tests/Makefile.am (+34/-0)
To merge this branch: bzr merge lp://staging/~ted/dbus-test-runner/set-system-bus
Reviewer Review Type Date Requested Status
Charles Kerr (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Ken VanDine Approve
Review via email: mp+240936@code.staging.launchpad.net

Commit message

Add ability to run as the system bus

Description of the change

A handy little feature

To post a comment you must log in.
Revision history for this message
Ted Gould (ted) wrote :

Realizing that I need a test and a different config.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Ken VanDine (ken-vandine) wrote :

I did a packaging review, which looks fine.

review: Approve
109. By Ted Gould

Enforce the symbols checking

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Charles Kerr (charlesk) wrote :

A handful of things, mostly minor, commented below. OK to top-approve with or without changes.

review: Approve
Revision history for this message
Ted Gould (ted) wrote :

On Tue, 2014-12-09 at 21:28 +0000, Charles Kerr wrote:

> > +static gboolean
> > +all_tasks_bus_match (G_GNUC_UNUSED DbusTestService * service, G_GNUC_UNUSED DbusTestTask * task, G_GNUC_UNUSED gpointer user_data)
>
> service and task aren't unused

Not sure why that didn't fail to compileā€¦

Fixed r110

> > + if (service->priv->bus_type == DBUS_TEST_SERVICE_BUS_SESSION ||
>
> This block would be cleaner as a switch statement s.t. DBUS_TEST_SERVICE_BUS_BOTH gets handled as its own case rather than as an add-on to _BUS_SESSION and _BUS_SYSTEM.
>
> > + service->priv->bus_type == DBUS_TEST_SERVICE_BUS_BOTH) {
> > + g_setenv("DBUS_SESSION_BUS_ADDRESS", line, TRUE);
> > + g_setenv("DBUS_STARTER_BUS_TYPE", "session", TRUE);
> > + }
> > +
> > + if (service->priv->bus_type == DBUS_TEST_SERVICE_BUS_SYSTEM ||
> > + service->priv->bus_type == DBUS_TEST_SERVICE_BUS_BOTH) {
> > + g_setenv("DBUS_SYSTEM_BUS_ADDRESS", line, TRUE);
> > + }
> > +
> > + if (service->priv->bus_type != DBUS_TEST_SERVICE_BUS_SESSION) {
> > + g_setenv("DBUS_STARTER_BUS_TYPE", "system", TRUE);
> > + }

Was skeptical, but tried it and I like it!
r111

> > +void dbus_test_service_set_bus (DbusTestService * service, DbusTestServiceBus bus)
> > +{
> > + g_return_if_fail(DBUS_TEST_IS_SERVICE(service));
> > + service->priv->bus_type = bus;
> > +
> > + if (bus == DBUS_TEST_SERVICE_BUS_SYSTEM) {
> > + g_free(service->priv->dbus_configfile);
> > + service->priv->dbus_configfile = g_strdup(DEFAULT_SYSTEM_CONF);
> > + }
>
>
> This is kind of a brittle function since (1) it doesn't handle
> changing the bus from system to session and (2) it's ambiguous what
> the Right Thing is if _BUS_BOTH gets passed in.
>
>
>
> Fortunately these situations don't seem to be possible in this MP
> thanks to the safeguards in the command-line parser's new code. But
> still, it might be safer to add more invariants to the front of this
> function to be safe; e..g. return_if_fail (bus_type == SESSION)

So I did a few things. I provided warnings if we're switching to BOTH, I
really consider that a default value for backwards compatibility. But if
someone sets it to to BOTH or SESSION restoring the old config value.
Also added a warning if changing it results in some of the task list not
working.

r112

110. By Ted Gould

Fixing unused marking of parameters

111. By Ted Gould

Restructure if's as a switch statement

112. By Ted Gould

Make setting bus type more robust

113. By Ted Gould

Adding the default value to the command line help

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches