core/exec-invoke: rework $TERM fallback logic#37647
Conversation
github-actions
bot
added
util-lib
please-review
|
There should be description I guess. |
yuwata
left a comment
yuwata
left a comment
There was a problem hiding this comment.
LGTM. But let's wait for other reviewers.
| n = strlen_ptr(e); | ||
|
|
||
| if (n <= 0) | ||
| if (n == 0) |
There was a problem hiding this comment.
IIRC, @poettering prefers <= even for unsigned. I do not have any preference on it. So, feel free to keep it.
There was a problem hiding this comment.
FWIW, I find <= for unsigned types very confusing. I always do a double take and recheck that the type is indeed unsigned. Variables don't change type unexpectedly, so this kind of "safe coding" just in case somebody comes and changes the type of the variable we define ourselves seems completely unneeded.
The only place where this would make sense is for externally defined variables in libc and elsewhere where there is some confusion as to the exact type, or the documented type is iffy.
|
Added one more commit on top, PTAL. |
|
I think this should wait until #37538 is merged. I don't want to do a massive rebase. |
I don't see it being "massive" though. This essentially just moves things around wrt your PR... |
|
The main work happens in #37538 though, and it's not finished yet. |
|
If you try to literally "rebase" it it can be painful indeed, but from scratch it's only a couple of lines: diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c
index eb8ac14322..14c4d65b04 100644
--- a/src/core/exec-invoke.c
+++ b/src/core/exec-invoke.c
@@ -4578,9 +4578,16 @@ static int setup_term_environment(const ExecContext *context, char ***env) {
if (r < 0)
log_debug_errno(r, "Failed to read '%s' from kernel cmdline, ignoring: %m", key);
}
+
+ /* This handles real virtual terminals (returning "linux") and any terminals which support
+ * the DCS +q query sequence. */
+ _cleanup_free_ char *dcs_term = NULL;
+ r = query_term_for_tty(tty_path, &dcs_term);
+ if (r >= 0)
+ return strv_env_assign(env, "TERM", dcs_term);
}
- /* If no precise $TERM is known and we pick a fallback default, then let's also set
+ /* If $TERM is not known and we pick a fallback default, then let's also set
* $COLORTERM=truecolor. That's because our fallback default is vt220, which is
* generally a safe bet (as it supports PageUp/PageDown unlike vt100, and is quite
* universally available in terminfo/termcap), except for the fact that real DEC
@@ -4597,7 +4604,7 @@ static int setup_term_environment(const ExecContext *context, char ***env) {
if (r < 0)
return r;
- return strv_env_assign(env, "TERM", default_term_for_tty(tty_path));
+ return strv_env_assign(env, "TERM", FALLBACK_TERM);
}
int exec_invoke( |
This PR makes sense on its own as explained in the commit message. It contains follow-ups to the logic we merged in this release cycle and more cleanups. So either way it should go in IMO. |
|
I agree that this makes sense and should go in… But it increases work for both of us if there's a pull request open that touches the same function. So please wait with this one until the other one is done. |
Follow-up for 728dbae and ad6ca4a This is inspired by systemd#37538, see the discussion in systemd#37538 (comment). If the user already specifies $TERM (which is actually quite common if you look at run0), we'd needlessly invoke the "fallback" logic and a) possibly issue a DCS query whose result we end up simply discarding in strv_env_merge() b) set $COLORTERM to "truecolor" unconditionally, whereas the explicit $TERM value might intend to disable the color output To address this, the logic of setting fallback $TERM and friends has been split out of build_environment(), and we'd call into it only after all envvars have been collected.
keszybz
left a comment
keszybz
left a comment
There was a problem hiding this comment.
LGTM, thanks for the followup.
github-actions
bot
removed
the
please-review
4 participants
Inspired by #37538, see a detailed rationale in #37538 (comment).