-
Notifications
You must be signed in to change notification settings - Fork 272
Support exporting variables into an starting environment #782
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
5fa5e74
to
49be101
Compare
f83bb7f
to
5682c69
Compare
i rebased this on top of #819, so that's now a dependency of this mr also reimplemented it with envdirs instead, makes it easier to unset variables, and we don't have to worry about escaping things at all |
added more details in commit messages |
ca810a6
to
d90077a
Compare
d90077a
to
b3f7d4c
Compare
src/librc/librc-misc.c
Outdated
fputs(value, envfile); | ||
return fclose(envfile) == 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also need to check the fputs
return value. Also, shouldn't we check of envfile
being NULL
as well?
b3f7d4c
to
8c16022
Compare
src/openrc-run/openrc-run.c
Outdated
rc_import_variables(svc->value); | ||
if (rc_service_state(svc->value) & RC_SERVICE_STOPPED) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So service specific env are supposed to overwrite the global one?
Also should we be importing the env of a stopped service?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also should we be importing the env of a stopped service?
no i'm just stupid and forgot the dep stop is about dependees, not dependers
So service specific env are supposed to overwrite the global one?
i dunno actually, i could see it being argued either way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dunno actually, i could see it being argued either way
Yes, that's why I am asking. Either way we need to document it.
Or we could also provide some sort of special "const" functionality for the global env which disallows services overriding it. E.g something like: rc.override
which will be sourced at the very end.
Or we can default to global env overriding service ones, and then have something like rc.preference
which sets a preference but allows services to take priority.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, i say service-specific ones should take priority, because:
1: allows you to set something generically for all services, then override it in specific conf.d's
2: services could "choose" to allow users to override vars by doing : ${FOO:=default}
3: matches the conf.d semantics (i.e. rc.conf goes first, conf.d/$svc overrides)
Also, wouldn't we be able to get rid of this and simply export them into the global starting env during init? Lines 67 to 71 in 6411b4b
Seems to be a cleaner and more deterministic approach. |
for XDG_RUNTIME_DIR, no, since the envdir would be located in $XDG_RUNTIME_DIR/openrc/environment -- for the others we could tho it'd be a lot of "setting the same values at every invocation" and the tmpfs IO related to that, just to re-export the same vars again later? |
8c16022
to
abf6e0a
Compare
The point is to have a deterministic environment for starting the services instead of getting the env from the whatever calling process' env is. But that's not a blocker, can be done in later PR. |
It'd be nice to have some user facing documentation on how this works. Maybe in |
If you are trying to change openrc-run, the man page for openrc-run should change as well, or people will not know about this. |
it's not ready yet, i'm still changing things docs will be updated before it's merged |
Does this pull request export variables into a starting environment and also new login sessions? There can be multiple login sessions. If I log in sway and then a linux virtual console, I have two login sessions. pam module should export DBUS_SESSIONS_BUS_ADDRESS to the second login session and the later ones as well as the first session that started dbus user service. |
47f797f
to
c014c02
Compare
new version. i'm unsure about the librc api, but seems okay enough? i've also for now added a new binary, rc-env, i still prefer pam_openrc now imports the variables of services added to 'boot' to the pam environment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just some comments :-)
src/librc/librc.c
Outdated
if (!do_getfileat(dirfd(envdir), d->d_name, &value, &size)) | ||
continue; | ||
|
||
xasprintf(&envlist[i++], "%s=%s", d->d_name, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still in favor of having a second function to retrieve these values, since it makes it easier to pretty-print or use in, say, an application that has 2 textboxes for each key/value. (otherwise you have to rely on strchr and all that). Is this still possible? :-)
src/openrc-run/openrc-run.c
Outdated
return; | ||
|
||
for (size_t i = 0; i < count; i++) | ||
putenv(env[i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should check putenv for errors and propagate them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
putenv only ever fails on OOM, i'm unsure how it's best to handle that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im not a fan of how librc handles errors to begin with. We can put this aside for now and address it later. But its still good to handle oom errors if possible, typically id make macros for doing that.
But this is openrc-run, so oom is tricky. Probably best to just use perror or strerror and hope for the best.
Also, ENOMEM can also be a sign of something wrong with malloc internal structures indicating a serious bug or worse corruption (but glibc malloc will usually yell regardless). They are always nice to check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we usually abort on allocation failure, which i really dislike and would like to handle errors properly
here i'll add a eerror but otherwise attempt to proceed with the service
Also, ENOMEM can also be a sign of something wrong with malloc internal structures indicating a serious bug or worse corruption (but glibc malloc will usually yell regardless). They are always nice to check.
that is not our problem, we assume a working malloc. if malloc is broken, we are to handle it the same as out of memory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If malloc is broken then it can at least help highlight issues. Malloc could be broken because we broke it. So, it's good to check it just incase. We have valgrind and asan but if someone reports such a bug, it's easier to see the error instead of a mysterious "malloc: blah blah broken arena"in who knows who where
It is entirely up to you though :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how would we have broken malloc? what scenario would that come up, and what "help" would we even give other than "error: ENOMEM"?
if you mean we go out of bounds and corrupt some malloc data, that's not on the field of "helping highlight broken malloc", that is "we did out of bound writes and that's bad™"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the latter. Course it's bad, but the earlier it's caught the easier it can be smelt out. I agree through ENOMEM is not helpful for that in particular
For now you can leave it alone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If internal malloc data structures are corrupted, the implementation is free to abort or not return an error. We hope that useful implementations would abort.
I think it's unlikely that they'll return ENOMEM in such a case: it seems unlikely that an implementation could reliably detect corruption but wouldn't then abort on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hard to detect corruption. Libcurl just returns a memory error. But this is an application, so abort is fine, Sam put it best
Glibc Malloc will abort anyway if something is effed up (i recall)
c014c02
to
c7fd7b9
Compare
librc api to interact with the script environemnt. it allows us to set variables that would live across invocations, like, exporting *DISPLAY for gui daemons (notification, xdg-desktop), a bus address dependees should use the implementation uses an "envdir", $RC_SVCDIR/environment is the root, and each directory inside is an envdir for the respectively named service, and 'rc' is a special directory for all services (global environment) each envdir contains files, whose names are the variables name, and contents the value to be exported
4e4a3e0
to
ec43354
Compare
used to export environments set with the `export` variable in openrc-run
ec43354
to
816eebc
Compare
included is a refactor of rc-update also