Code review comment for lp://staging/~ted/dbus-test-runner/set-system-bus

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