Implement listening on activation environment changes

[q66]

This implements a protocol that lets a connection to the control socket listen on changes in the global environment issued by dinitctl setenv. This opens up a range of possibilities - such as special services that trigger other services when an environment variable appears.

It also implements a dinit-monitor environment watching mode as a proof of functionality, though likely not acceptable as is (likely at least the string_view will need getting rid of?)

I intentionally did not implement things like protocol checking and whatnot yet since I want your feedback first.

I should probably also change the protocol a bit - at very least, the envevent should contain information about whether the envvar is newly created or if it overwrites a previous one.

Let me know what you think :)

[q66]

i think the code quality in this is ok now, what's missing from my side is imo:

1. documentation
2. tests
3. the protocol bit about new/overriden envvars

[q66]

i also added an option to dinit-monitor that makes it exit upon first issued command; that means you can invoke something like

$ dinit-monitor --initial --env --exit --command /usr/bin/true FOO BAR BAZ

as a way to wait until either of the given environment variables becomes available (with --initial it should be race-free and will simply exit when it's already available)

[q66]

There is another thing I would like to have while at it and that is unsetting of environment vars. As I see it, we don't have to add a protocol message for it; we can repurpose the SETENV message and instead of raising BADREQ when the value has no =, we could just change

    eq = envVar.find('=');
    if (!eq || eq == envVar.npos) {
        // Not found or at the beginning of the string
        goto badreq;
    }

    main_env.set_var(std::move(envVar));

to

    eq = envVar.find('=');
    if (eq == envVar.npos) {
        // Unset
        main_env.undefine_var(std::move(envVar));
    } else if (!eq) {
        // At the beginning of the string
        goto badreq;
    } else {
        main_env.set_var(std::move(envVar));
    }

and add a new command to dinitctl that will invoke the protocol in this form...

thoughts?

[davmac314]

Let me know what you think :)

This can go in if you need it. I'm sort of questioning whether this is really the best option though - seems like you basically just need a channel to communicate that something has happened (display server has started, or whatever), and dinit (with a little augmentation) just happens to be able to act as that channel. Could triggered services achieve the same thing? I.e. just trigger some service after setting the environment variable (possibly one service per variable, and you also have the option of having a dependent service which then will only start once all variables have been triggered, for example).

we can repurpose the SETENV message and instead of raising BADREQ when the value has no =, we could just change

This seems perfectly reasonable to me.

[q66]

well we're still using triggered services either way (a persistent watcher process, for now implemented via dinit-monitor, will take care of triggering the service - or well, be one of the sources that can trigger it)

the thing is stuff like gnome-session updates the activation environment in dbus, and dbus can in turn update the environment in dinit (which it should, especially with dbus-broker)

with this we can have dinit automatically react to it and trigger the right triggered service, without having to patch purpose-specific paths into either dbus, or every single desktop session program, so all the "specific" logic is in things that are particular to dinit services, and the only patched thing is dbus, which is patched with generic logic to update dinit when it's itself updated (logic that is already needed to support dbus services that are supervised via dinit; with dbus-broker we will not even be patching anything anymore, because the controller is specific to the service manager)

we are pretty much doing the same thing as systemd in this regard, so it plays along with what everybody already does; having to patch things for dinit in different places would be much more of a pain

[q66]

I implemented the rest of what I wanted to do. There is now dinitctl unsetenv and it works. The event protocol tracks override (hopefully correctly) and both sets and unsets (for unsets, the override flag is true when the variable previously existed).

I updated the manpages too, not sure what to do with tests considering this is only protocol and dinit-monitor and we don't have tests for that.

[davmac314]

Ok, looks fine. For tests you can add something to src/tests/cptests/cptests.cc, that is where control protocol tests live.

[q66]

I added a cptest, seems to be fine

[davmac314]

Could we name this read_vars_and_issue or something a little more specific? (I would tend to use process_initial_vars but whatever, just needs to be a bit more descriptive)

[q66]

done

[davmac314]

So this reads and processes the packet header and leaves the body of the reply pending? Could you add a comment explaining that

[q66]

yes, added

[davmac314]

Sorry, just a couple more minor nits (comments above).

One more thing is that really, dinit-monitor should check for a compatible protocol version before using the LISTENENV packet. Or raise the minimum version that dinit-monitor will talk. In either case "server is too old" is a better message than "protocol error".

[q66]

i fixed the above, and also incremented the protocol ver to allow it to work at all

[davmac314]

Great, thanks