Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Environment generators #5131
Conversation
keszybz
added
the
pid1
label
Jan 23, 2017
poettering
reviewed
Feb 2, 2017
A couple of comments.
I am not overly enthusiastic about this, but OK.
What I really try to understand is why the sequential generators thing is a good idea. What's the real-life usecase for this? generators that patch around in each others generated data sounds wrong to me... The example used in the man page I don#t find overly convincing either...
I also wonder about the variable expansion within environment files. Do we really want to have that? I am pretty conservative on defining new magic macro formats like this I must say...
And if we need one of the above, do we really need both?
| + <para><filename>/run/user/<replaceable>uid</replaceable>/systemd/environment.d</filename></para> | ||
| + <para><filename>/etc/environment.d/*.conf</filename></para> | ||
| + <para><filename>/run/environment.d/*.conf</filename></para> | ||
| + <para><filename>/usr/lib/environment.d/*.conf</filename></para> |
poettering
Feb 2, 2017
Owner
should these files really have the .conf suffix? Maybe .env or so, instead?
| + <literal>KEY=VALUE</literal> environment variable assignments, separated | ||
| + by newlines. The right hand side of these assignments may reference | ||
| + previously defined environment variables, using the | ||
| + <literal>${OTHER_KEY}</literal> format.</para> |
poettering
Feb 2, 2017
Owner
what precisely is the usecase for variable expansion here?
This has been requested before for EnvironmentFile=, but I so far have been very conservative about it. If we do add this here, why not also in EnvironmentFile=?
Is this syntax shell compatible? environment files so far always have been considered something you could also source in...
| @@ -7,6 +7,8 @@ | ||
| # See tmpfiles.d(5) for details | ||
| +d /run/environment.d 0755 root root - |
poettering
Feb 2, 2017
Owner
why this? if you want to be able to put a file there you need privs anyway, and can hence create the dir... an there's nothing too special in the access mode tha twould prohibit that...
I don't think this file should exist, except when people actually need this...
poettering
added
the
needs-discussion
label
Feb 2, 2017
One common and special case if appending and prepending to variables, which may or may not be already defined. In particular, the following operations are needed:
This pattern of growing a list in a variable applies to $PATH, $PYTHONPATH, various $XDG_*_DIRs, possibly $CLASSPATH. I can see three ways to provide this:
I think the first two solutions would have a tendency to add more features until they expand into a turing-complete language, just like shell and make did. The third solution allows for arbitrarily complex stuff, but leaves the syntax extremely simple.
Strictly speaking, no. If generators run sequentially, we can force them to perform any expansions internally. Nevertheless, I imaging that many (most?) generators will not need to look at the old values of the variables, but would want to make use of them. E.g. it's easier to say There's precedent for allowing envvars to be expanded, without any other fanciness: our Environment= directive, pkgconfig files. I can drop this part, we can always add it later, but I think it makes the whole scheme a bit easier to user. Please note that there's less ambiguity, since there is no splitting into arguments, so $VAR just expands to a string of zero or more characters.
This was discussed back and forth in #5904. No other files are planned in those directories, so no need to use a suffix to distinguish them. We have .conf in modprobe.d, tmpfiles.d, modules-load.d, sysctl.d, binfmt.d, sysusers.d. I prefer .conf because of consistency.
Yes. It's definitely supposed to be 100%-shell compatible. Doing |
|
To add to the sequentiality question: |
|
well, but these generators would still block the login, and if that's shell, that's slow... I still don't get why we need both the seq generators and env var expansion. Wouldn't it suffice to have async generators, and then simply make sure they put they output their fragments in the right order, so that they can extend what others have written without having to actually read and parse the env var snippets from other generators? |
Let's say, for the sake of discussion, that env var expansion is removed.
No, unless there's some way I'm not seeing, of implementing the requirements described in #5131 (comment). |
|
hmm, but we get env vars from other sources too, such as PAM or login, or gdm and whatever else. how would such a generator take those into consideration? I wonder if it wouldn't be more natural to change the generators here to simply build a pipeline: we pass in the env vars we get from PAM/login/whatever starts us, then each process reads them from stdin, manipulates them, spits them out to stdout, and the next one does the same... I mean, unlikely unit generators we actually aren't interested at all in stuff that hits the file system, and instead of creating disjunct nodes all they do is manipulate an env var stream... I think this would make a lot of things nicer and easier.... (by pipeline i don't mean we necessarily have to implement this as actual shell-like pipeline where all processes run in parallel with actual pipes in between. I just mean that each one gets the output from the previous one, and spits it out again...) |
|
Yeah, I thought about something like that too. I'll try to rework the code to do this. I think we should still dump the final environment to a file somewhere, to allow external processes to pick it up easily. Not sure if we need environment.d for anything if this is implemented. |
|
admins shouldn't have to write a program to change the environment system wide. Likewise, users shouldn't have to write a program to set an environment variable for their session (including user services). And, it would be nice if software like flatpak could just drop a file to augment XDG_DATA_DIRS (or PATH or LD_LIBRARY_PATH) without having to ship a whole program (although, it's not the end of the world if it does). Running a bunch of programs at startup for filtering the environment gives a lot of flexibility, but I hope it's flexibility we don't have to use commonly... I mean status quo is running a bunch of scripts at startup in /etc/profile.d...we're trying get away from that model. I like environment.d because it provides an answer for users and admins, and isvs. |
|
Sure, a drop-in file is great, if it works. The problem with just allowing straightforward environment.d dir is that it's not powerful enough (or maybe it is powerful enough with "advanced" variable expansion, but I'm pretty sure that usecases where it's not powerful enough will crop up anyway).
On second thought, maybe we should have both:
In this scenario, the simplest cases would be handled by dropping a trivial X=Y file into the right place, and more complicated cases would be handled by a "program", but it'd still be quite trivial because it wouldn't need to parse anything, just pull some stuff out of its environment and print to stdout. |
|
I think that's better, but it still sucks for a user that wants to add ~/bin to their PATH (or whatever). If a user has to write a program to do that, they're going to grumble. |
|
Actually prepending stuff to $PATH could be trivially handled using the environment.d files. But even for more complicated cases, let's say you want to add something to path only if it's not already there: Not too ominous. (I mean: it's not any harder then putting the same in .bashrc. There's a certain minimal level of complexity below which it's hard to go.) |
|
Yea I guess $PATH was a bad example since it will always be set, and duplicates aren't a huge problem. A better example would be LD_LIBRARY_PATH, where if the environment.d snippet had: LD_LIBRARY_PATH=${HOME}/lib:${LD_LIBRARY_PATH} and LD_LIBRARY_PATH wasn't set, then current working directory would get added to LD_LIBRARY_PATH and potentially create a security problem. I don't think your generator is quite what @poettering was proposing, though. My understanding is it would have to be right? |
sure that makes sense to me. |
I have the suspicion that desktops such as GNOME would anyway prefer configuring env vars from a different place than a plain text drop-in file? I mean, dconf or so? By only providing executable "plugins" for population of the env vars we permit people to hack things up the way they like, using any source they like. One option would even be for some project to ship an executable that populates the environment from an enviuronment file, but systemd wouldn't be in the business of implementing that. |
|
and yeah @halfline is right, i had in mind that env vars would be fed in via stdin, be processed and then written out via stdout again. But I figure we could instead also pass them in via the usual env block, and then make the generate output simply an "override". However, that would mean there would be no way to unset an env var.... Not sure if that's a limitation though... I'd be much more comfortable with supporting drop-in env var snippets btw, if we wouldn't do variable expansion on them. i.e. dumb env var files sound quite acceptable to me. |
We pull specific environment variable settings from dconf. mainly locale related variables. But, only out of necessity. I don't think we'll ever have any dedicated general custom_environment dconf key or something.
So a big part of the appeal of this change is having a standardized place that projects and users can rely on. If we can't have a standard place for this, I think software will just keep using /etc/profile.d which is the de facto answer now. Also, pushing handling of /etc/environment to systemd from pam_env would mean distros could have one less pam module running as root. /usr/share/gnome/environment.d just wouldn't be as generally useful. From a flatpak perspective, having a little environment generator would probably be fine but it's not a great answer for sysadmins and users.
The problem is that some of the most common use cases wouldn't be covered without the flexibility of variable expansion (PATH, LD_LIBRARY_PATH, XDG_DATA_DIRS). |
Well, but key here is that there is a difference between projects and users. I think the "generators" concept would be perfectly fine for projects. Heck, in fact I think it's better suited for them, as only they really know what the right logic to extend/override their specific env vars is. For the user case I think that ~/.profile is actually sufficient... |
It's less convenient and it means those projects may have to come up with some other answer for non-systemd systems, but fair enough (it's not like the standard i was gunning for is implemented anywhere yet). env generators are certainly more flexible and achieve the end goal okay (though I was hoping we could lower the number of tiny programs started during login, but doesn't really matter).
It's not really sufficient as it stands right now. I mean gnome wayland sessions didn't run a login shell at all until a couple of weeks ago... and what about systemd --user services ? I mean I guess we could ship an env generator with systemd that ran a login shell and printed the full environment? At the moment, gnome-session will try to push the session environment up to systemd --user but that's racy with user services that start right at pam_open_session time, and really, races aside, the wrong way around imo. |
Uh, why would they even? Not following? Why would you run gnome-wayland from a any shell at all?
Wut? I'd leave ~/.profile for login shells, and not do that for anything else I must say. I seriously doubt the stuff set that way belongs anywhere else than in interactive shells by the user... |
Because users want their environment variables set ! See the gnome bug:
most terminal applications don't run a login shell by default, so ~/.profile isn't going to execute if not done earlier... and users want a way to set environment variables that affect all applications in the session, not just stuff they run in a terminal. for instance adding $HOME/bin to PATH , so they can run programs they compiled themselves, or putting G_MESSAGES_DEBUG=all so the session outputs debug information, or whatever. |
|
urks. shells in the clean code paths. that's just sick... Anyway, I'd probably be OK if we'd ship an env var generator like using the proposed interface that does what @halfline wants: reads a bunch of drop-ins and does basic variable substitution on them. I mean, we ship also the ugliness that is rc-local-generator, and hence it's kinda hard to say no to that... Still not liking it though, and I'd definitely prefer if such a generator would live somewhere else than in systemd, in some reasonably standard package. After all, if we have the generator concept, then people can provide generators easily externally... maybe it's time to start an xdg project for standardizing ~/.environment/ or so, and then do a reference implementation that everybody can adopt that among hooks into other stuff also provides hookups into systemd by shipping a generator? |
yea I tried to get away from it with wayland, but the backlash was too strong, since we don't have an alternative mechanism... (and thus #3904 and this pull request)
that'd be great!
I'm not super enthusiastic about maintaining another module…especially something dealing with environment variables (not saying i won't do it, but I don't want to do it) I was thinking we'd just add /etc/environment.d to https://www.freedesktop.org/wiki/Software/systemd/InterfacePortabilityAndStabilityChart/ with |
|
@halfline: what about this: you put a brief spec together that can be hosted on fdo, and describes the drop-in dir, and the relevant supported variable expansions? we'd then add a generator implementing it to systemd, but wouldn't be the ones inventing this, and can point to fdo for this. Or in other words: if you don't want to host the generator elsewhere, at least host the spec elsewhere, so that we can support this new "xdg-envvars" spec (or whatever you want to call it) the same way we already support "xdg-basedir" and suchlike? |
|
0kay sounds good. Will report back (though I have a few unrelated deadlines to attend to first!). |
|
@halfline before you start working on this, please wait a bit. I'm currently rehashing my PR. |
|
Sure |
keszybz
added some commits
Jan 22, 2017
|
I pushed a new version. I think this is ready for another round of review. tl;dr: systemd manager instance will run generators from /usr/lib/systemd/environment-generators which can write new variable definitions on stdout. Each generator is started with the current set of variables in the environment block. One generator is implemented: it reads environment.d directories. Simple variable substitution is supported. @poettering suggested creating a spec for environment.d through fdo. I think that's much more of an effort than it's worth. The generator in current patch set is 64 lines (according to sloccount), the documentation is ~100 lines. I don't think the final spec from fdo would be much different, but doing this through systemd ensures that we have a uniform solution that everybody can start using when 233 is released. In particular, releasing the environment generator support without the environment-d-generator doesn't seem useful, because it would provide half-broken functionality and force people to create generators when they could just use a drop-in in environment.d. (There's the question of allowing variable expansion or not. I don't think it is necessary, at least for now. It's always possible to extend the syntax later and allow substitution. I think we should get some feedback from people if the current version covers their needs.) |
| @@ -274,6 +274,19 @@ _pure_ static bool env_match(const char *t, const char *pattern) { | ||
| return false; | ||
| } | ||
| +_pure_ static bool env_entry_has_name(const char *entry, const char *name) { |
poettering
Feb 13, 2017
Owner
pure is kinda unnecessary if the function is static anyway, no? the compiler can figure this out on its own really..
| @@ -336,7 +349,7 @@ char **strv_env_unset(char **l, const char *p) { | ||
| for (f = t = l; *f; f++) { | ||
| - if (env_match(*f, p)) { | ||
| + if (env_entry_has_name(*f, p)) { |
poettering
Feb 13, 2017
Owner
is this change really the best idea? Previously, you could call strv_env_delete() with FOO=BAR and it would remove exactly FOO=BAR, but never FOO=QUUX. You could also call it with just FOO in which case FOO=BAR and FOO=QUUX are removed.
I think this behaviour makes a lot of sense, and we do expose it in our bus calls for seting the env vars, afaics. THis chnage doesn't look right to me hence?
keszybz
Feb 18, 2017
Owner
OK, thanks for the explanation. I was wondering why this is like this.
All APIs to read the environment entries (except for manually iterating over the third argument to main, but that's special) assume that keys are unique. So we should always keep the keys unique.
I'll change the set and replace functions to do that.
| @@ -365,7 +378,7 @@ char **strv_env_unset_many(char **l, ...) { | ||
| va_start(ap, l); | ||
| while ((p = va_arg(ap, const char*))) { | ||
| - if (env_match(*f, p)) { | ||
| + if (env_entry_has_name(*f, p)) { |
| #include "time-util.h" | ||
| -void execute_directories(const char* const* directories, usec_t timeout, char *argv[]); | ||
| +typedef int (*gather_stdout_callback_t) (int fd, void *arg); |
poettering
Feb 13, 2017
Owner
For most other cases where we have a callback that takes arbitrary userdata ptrs we called that ptr userdata. I think we should do that here, too!
| +typedef int (*gather_stdout_callback_t) (int fd, void *arg); | ||
| + | ||
| +typedef struct { | ||
| + gather_stdout_callback_t one; /* from generators to helper process */ |
poettering
Feb 13, 2017
Owner
in comments, maybe say explicitly which component wrote to the fds passed here before, from which context (and how often) this is called and what the callback probably should do with it. i.e. something maybe like:
Called once for each generator after the generator ran. Gets an fd passed that contains the generator's stdout output. Will be called from a process that is the child of the process invoking execute_directory(). Should process the output as necessary and then propagate whatever it wants to propagate to its own stdout, which will then be read by the next callback ("two").
Or something like that.
| + void *two_arg; | ||
| + gather_stdout_callback_t three; /* process data in main process */ | ||
| + void *three_arg; | ||
| +} gather_stdout_callbacks; |
poettering
Feb 13, 2017
Owner
for exported structures I think it would be nicer to use CamelCase naming.
poettering
Feb 13, 2017
Owner
Also, when I see something like this I wonder if this should better be a structure containing an array of callback/userdata pairs. After all you are even numerically indexing the entries by calling them "one", "two" and "three".
You could then define an enum with named indexes. Something like this:
enum {
STDOUT_GENERATE,
STDOUT_COLLECT,
STDOUT_CONSUME,
};I think naming the three callbacks like this would make a lot of sense. Of course, the names aren't entirely self-explanatory, but I think "generate", "collect" and "consume" still are better than "one", "to", "three" ;-)
| + path = getpid() == 1 ? "/run/systemd" : "/tmp"; | ||
| + fd = open_tmpfile_unlinkable(path, O_RDWR|O_CLOEXEC); | ||
| + if (fd < 0) | ||
| + return -errno; |
poettering
Feb 13, 2017
Owner
should return fd here... (not a bug in your change, this apparently was a pre-existing bug)
| + }; | ||
| + | ||
| + if (!generator_path_any(environment_generator_binary_paths)) | ||
| + return 0; |
poettering
Feb 13, 2017
Owner
hmm, so what's the story here? this is called for both --system and --user mode the way this is right now? Is this really the intention?
If this is the intention, shouldn't the dirs be called "system-environment-generators" and "user-environment-generators"? If that's not the intention, shouldn't the dir be called "user-environment-generators" anyway, simply that the door is open to maybe one day add this for the system generators too? (not saying htat would be a good idea, just want to keep the avenue open...) or am i missing something?
keszybz
Feb 14, 2017
Owner
There's a duality: environment.d dirs are only for users, but the generators themselves run for all manager instances, and are supposed to just do nothing in cases where they should do nothing. My motivation for this was that: a) it's quite possible to want to set the same set of variables for all environments, and having install two generators would be a bit more work, b) it's fairly easy to check if running as a user, c) doing just one dir simplifies the implementation and documentation.
But on second thought, it's not so obvious: in environment-d-generator, I used getuid, but it seems that environment.d directories should be also supported for the root's --user manager. In this case, that check is wrong. Instead, we should check if $USER is set.
I you think that there should be two separate sets of generators, I can change that. I still think that simply checking if $USER is set is simpler, but I don't feel particularly strongly about that.
poettering
Feb 14, 2017
Owner
hmm, so I am torn about this... I mean, I think that both uid==0 or $USER being set aren't the best choices for detecting in which context we are called. Using getppid() == 1 might be better, but breaks in we should ever want to run this from "systemd --system --test" or so...
another option would be to set $SYSTEMD_CONTEXT=user or so, and suggesting people use that for discerning the mode of invocation. That of course sucks, given that the whole concept of environment generators is to alter the environment, hence it might be a bit nasty to use the environment for this, too... Another option would be to use do what sd_bus_open() uses to figure out in which context it is run and hence to which bus to connect to: whether cg_pid_get_owner_uid() succeeds.
But the other thing, is of more philosophical nature: our units generators so far where placed in two distinct directories, and so where our static units, and everything else we did. I am tempted to say we should just continue to do it that way, simply to keep things reasonably uniform. And if people really want to use the same generator in both contexts, they are welcome to by symlinking the generator into both...
keszybz
Feb 14, 2017
Owner
We could also use an argument in argv[1]. Currently argv is not used at all.
takluyver
commented
Feb 13, 2017
As I've followed the issue through from the Gnome bug before Fedora enabled Wayland by default: what I want is a way to add directories to
It's possibly unfortunate that several important environment variables function as lists rather than atomic values, but I'd like it to be practical to manipulate these. |
|
Nah, expanding $PATH is quite well supported. It's even in one of the examples in the new man page. |
poettering
removed
the
needs-discussion
label
Feb 14, 2017
|
Updated. I fixed the stuff you mentioned, and added another commit on top to tighten handling of unexpected syntax. |
keszybz
added
the
reviewed/needs-rework
label
Feb 18, 2017
|
I made some small fixes and added /usr/lib/environment.d/00-xdg-basedir.conf to predefine the $XDG_* vars. This way it's possible to append to them later on without having to repeat the defaults and use variable expansion tricks. |
keszybz
removed
the
reviewed/needs-rework
label
Feb 18, 2017
poettering
reviewed
Feb 20, 2017
Hmm looks good in general, but I am really not convinced about the xdg variable drop-in now. The spec explicitly say the env vars should be used when set, but apps should have fall backs in place with the listed paths. I really doesn't look right to me to always "pollute" the env block with these env vars which are almost never necessary. I mean, there are only a handful of people in this world who want to change these paths, and I am sure if they are adventurous enough to set things up like that they are also smart enough to deal with set the fallbacks correctly.
REally, I don't think we should have that in place by default, it appears entirely redundant to me.
| + path = getpid() == 1 ? "/run/systemd" : "/tmp"; | ||
| + fd = open_tmpfile_unlinkable(path, O_RDWR|O_CLOEXEC); | ||
| + if (fd < 0) | ||
| + return -errno; |
poettering
Feb 13, 2017
Owner
should return fd here... (not a bug in your change, this apparently was a pre-existing bug)
|
thanks for putting so much work into this. I still think it's a little unfortunate that users wanting to set Having Honestly, my preference would be to just allow the extended substitution syntax. Then we can avoid unnecessarily adding XDG_DATA_DIRS to the environment block, let users set LD_LIBRARY_PATH without a generator, and still have generators for more complicated cases than the big 3 (PATH/LD_LIBRARY_PATH/XDG_DATA_DIRS). Anyway, regardless, this pull request puts us in good shape, and I'm happy. |
keszybz
and others
added some commits
Feb 11, 2017
|
Yeah, I think @halfline was right all along ;) I think current version would do the job, but somehow it doesn't seem a good fit: we'd either need to write generators for each and every thing, or make sure that all possible variables of list type are predefined. Both are just not satisfactory. I'll prep another version which restores the extended substitution syntax. |
keszybz
added
the
reviewed/needs-rework
label
Feb 20, 2017
keszybz
and others
added some commits
Feb 11, 2017
|
Here's another version. I forward-ported @halfline original patch to add "extended" syntax, but hooked it up just for the environment-d-generator. There are some tests, but just enough to confirm that things work if the correct syntax is used. If this approach is accepted, I'll add more tests for invalid syntax and various edge cases. |
keszybz
removed
the
reviewed/needs-rework
label
Feb 21, 2017
|
Another big question is how to hook this up to login sessions. Having the environment in the user manager is a nice step, but it isn't terribly useful if |
That's why I was requesting an XDG spec for the drop-ins. (not for the generators though) |
poettering
merged commit a4dde27
into
systemd:master
Feb 21, 2017
|
Hm |
evverx
referenced this pull request
Feb 21, 2017
Closed
test-env-util, test-fileio: LeakSanitizer: detected memory leaks #5405
| - v = strdup(value); | ||
| - if (!v) | ||
| - return -ENOMEM; | ||
| + if (value) { |
benjarobin
Feb 21, 2017
Contributor
Sorry (I am to curious) to ask that, but what is the use case to allow inserting NULL ? You use it in environment_dirs() but I don't really follow.
keszybz
Feb 21, 2017
Owner
Oh, I think this isn't used any more. It was used to insert an empty slot at the beginning of an argv array in some earlier version of the patchset.
benjarobin
Feb 21, 2017
•
Edited 1 time
-
benjarobin
Feb 21, 2017
Contributor
-
Feb 21, 2017
@keszybz Shouldn't this commit be reverted ? I personally think this is kind of dangerous to allow this kind of operation, we may leak element...
keszybz commentedJan 23, 2017
This is an mutation of #3904 that implements the idea of "environment file generators" as described in #3904 (comment). There's a bunch of cleanups and scaffolding, but the actual code changes are pretty small.
Wrt. to @halfline's version:
generators are executed by
systemd --userto populate it.