Discussion:
[PATCH 02/11] Use valid symlink for serial /dev/3270/tty1 in systemd-getty-generator
(too old to reply)
Werner Fink
2014-06-13 14:41:01 UTC
Permalink
Otherwise the add_symlink() function tries to make directories for each
slash even for the slash after the @ symbol in the final link name.

---
src/getty-generator/getty-generator.c | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git src/getty-generator/getty-generator.c src/getty-generator/getty-generator.c
index 6a4aa2c..5ee0bf0 100644
--- src/getty-generator/getty-generator.c
+++ src/getty-generator/getty-generator.c
@@ -67,6 +67,7 @@ static int add_symlink(const char *fservice, const char *tservice) {

static int add_serial_getty(const char *tty) {
_cleanup_free_ char *n = NULL;
+ char *at;

assert(tty);

@@ -76,6 +77,14 @@ static int add_serial_getty(const char *tty) {
if (!n)
return log_oom();

+ if ((at = strchr(n, '@'))) {
+ char *sl;
+ while (*(++at) && (sl = strchr(at, '/'))) {
+ *sl = '-';
+ at = sl;
+ }
+ }
+
return add_symlink("serial-***@.service", n);
}
--
1.7.9.2
Werner Fink
2014-06-13 14:41:04 UTC
Permalink
From: ***@gmail.com

will terminate emergency.service due to implicit dependencies on basic.target
and therefore sysinit.target which in turn conflict with emergency.target.

---
units/emergency.service.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git units/emergency.service.in units/emergency.service.in
index 94c090f..59f7cda 100644
--- units/emergency.service.in
+++ units/emergency.service.in
@@ -9,7 +9,7 @@
Description=Emergency Shell
Documentation=man:sulogin(8)
DefaultDependencies=no
-Conflicts=shutdown.target
+Conflicts=shutdown.target syslog.socket
Before=shutdown.target

[Service]
--
1.7.9.2
Andrey Borzenkov
2014-06-17 02:52:04 UTC
Permalink
В Fri, 13 Jun 2014 16:41:04 +0200
Post by Werner Fink
will terminate emergency.service due to implicit dependencies on basic.target
and therefore sysinit.target which in turn conflict with emergency.target.
I always considered it as a stopgap not suitable for upstream. But I
still do not know what can be done to fix it.

It still looks rather wrong that any arbitrary service can displace
the whole run-level. May be we need counterpart for RefuseManualStart
so that some targets can only be displaced manually, not as result of
implicit dependency.
Post by Werner Fink
---
units/emergency.service.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git units/emergency.service.in units/emergency.service.in
index 94c090f..59f7cda 100644
--- units/emergency.service.in
+++ units/emergency.service.in
@@ -9,7 +9,7 @@
Description=Emergency Shell
Documentation=man:sulogin(8)
DefaultDependencies=no
-Conflicts=shutdown.target
+Conflicts=shutdown.target syslog.socket
Before=shutdown.target
[Service]
Zbigniew Jędrzejewski-Szmek
2014-06-20 02:53:01 UTC
Permalink
Post by Andrey Borzenkov
В Fri, 13 Jun 2014 16:41:04 +0200
Post by Werner Fink
will terminate emergency.service due to implicit dependencies on basic.target
and therefore sysinit.target which in turn conflict with emergency.target.
I always considered it as a stopgap not suitable for upstream. But I
still do not know what can be done to fix it.
It still looks rather wrong that any arbitrary service can displace
the whole run-level. May be we need counterpart for RefuseManualStart
so that some targets can only be displaced manually, not as result of
implicit dependency.
Agreed. We cannot play whack-a-mole like this, there's simply too many
units which seemingly randomly change the "runlevel".

Zbyszek
Lennart Poettering
2014-06-20 16:38:27 UTC
Permalink
Post by Werner Fink
will terminate emergency.service due to implicit dependencies on basic.target
and therefore sysinit.target which in turn conflict with
emergency.target.
I am not entirely sure how emergecny.service should have been reached
with syslog.socket in place? isolate should not have left it around?
Post by Werner Fink
---
units/emergency.service.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git units/emergency.service.in units/emergency.service.in
index 94c090f..59f7cda 100644
--- units/emergency.service.in
+++ units/emergency.service.in
@@ -9,7 +9,7 @@
Description=Emergency Shell
Documentation=man:sulogin(8)
DefaultDependencies=no
-Conflicts=shutdown.target
+Conflicts=shutdown.target syslog.socket
Before=shutdown.target
[Service]
Lennart
--
Lennart Poettering, Red Hat
Andrey Borzenkov
2014-06-20 16:51:28 UTC
Permalink
В Fri, 20 Jun 2014 18:38:27 +0200
Post by Lennart Poettering
Post by Werner Fink
will terminate emergency.service due to implicit dependencies on basic.target
and therefore sysinit.target which in turn conflict with
emergency.target.
I am not entirely sure how emergecny.service should have been reached
with syslog.socket in place? isolate should not have left it around?
commit 80cfe9e163b1c92f917e0a5e053b148fca790677
Author: Dr. Tilmann Bubeck <***@reinform.de>
Date: Fri May 4 10:32:47 2012 +0200

Do no isolate in case of emergency or severe problems
Post by Lennart Poettering
Post by Werner Fink
---
units/emergency.service.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git units/emergency.service.in units/emergency.service.in
index 94c090f..59f7cda 100644
--- units/emergency.service.in
+++ units/emergency.service.in
@@ -9,7 +9,7 @@
Description=Emergency Shell
Documentation=man:sulogin(8)
DefaultDependencies=no
-Conflicts=shutdown.target
+Conflicts=shutdown.target syslog.socket
Before=shutdown.target
[Service]
Lennart
Lennart Poettering
2014-06-20 17:11:00 UTC
Permalink
Post by Andrey Borzenkov
Post by Lennart Poettering
I am not entirely sure how emergecny.service should have been reached
with syslog.socket in place? isolate should not have left it around?
commit 80cfe9e163b1c92f917e0a5e053b148fca790677
Date: Fri May 4 10:32:47 2012 +0200
Do no isolate in case of emergency or severe problems
Ah, indeed!

Hmm, not sure what to do then, both goals are mutally incompatible:
either we don't isolate, and then risk that running some commands will
trigger the substantial amount of code. Or we isolate and then things
are much harder to debug, because the system is gone...

Lennart
--
Lennart Poettering, Red Hat
Werner Fink
2014-06-13 14:41:05 UTC
Permalink
Therefore strip off the ANSI escape sequences for 3215 consoles
but support 3270 consoles if found.

---
src/core/manager.c | 24 ++++++++++++----
src/shared/util.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++---
src/shared/util.h | 1 +
3 files changed, 96 insertions(+), 9 deletions(-)

diff --git src/core/manager.c src/core/manager.c
index 0cb2044..f78ac18 100644
--- src/core/manager.c
+++ src/core/manager.c
@@ -112,7 +112,7 @@ static int manager_watch_jobs_in_progress(Manager *m) {

#define CYLON_BUFFER_EXTRA (2*(sizeof(ANSI_RED_ON)-1) + sizeof(ANSI_HIGHLIGHT_RED_ON)-1 + 2*(sizeof(ANSI_HIGHLIGHT_OFF)-1))

-static void draw_cylon(char buffer[], size_t buflen, unsigned width, unsigned pos) {
+static void draw_cylon(char buffer[], size_t buflen, unsigned width, unsigned pos, bool ansi_console) {
char *p = buffer;

assert(buflen >= CYLON_BUFFER_EXTRA + width + 1);
@@ -121,12 +121,14 @@ static void draw_cylon(char buffer[], size_t buflen, unsigned width, unsigned po
if (pos > 1) {
if (pos > 2)
p = mempset(p, ' ', pos-2);
- p = stpcpy(p, ANSI_RED_ON);
+ if (ansi_console)
+ p = stpcpy(p, ANSI_RED_ON);
*p++ = '*';
}

if (pos > 0 && pos <= width) {
- p = stpcpy(p, ANSI_HIGHLIGHT_RED_ON);
+ if (ansi_console)
+ p = stpcpy(p, ANSI_HIGHLIGHT_RED_ON);
*p++ = '*';
}

@@ -137,7 +139,8 @@ static void draw_cylon(char buffer[], size_t buflen, unsigned width, unsigned po
*p++ = '*';
if (pos < width-1)
p = mempset(p, ' ', width-1-pos);
- strcpy(p, ANSI_HIGHLIGHT_OFF);
+ if (ansi_console)
+ strcpy(p, ANSI_HIGHLIGHT_OFF);
}
}

@@ -154,6 +157,7 @@ void manager_flip_auto_status(Manager *m, bool enable) {
}

static void manager_print_jobs_in_progress(Manager *m) {
+ static int is_ansi_console = -1;
_cleanup_free_ char *job_of_n = NULL;
Iterator i;
Job *j;
@@ -178,10 +182,20 @@ static void manager_print_jobs_in_progress(Manager *m) {
assert(counter == print_nr + 1);
assert(j);

+ if (_unlikely_(is_ansi_console < 0)) {
+ int fd = open_terminal("/dev/console", O_RDONLY|O_NOCTTY|O_CLOEXEC);
+ if (fd < 0)
+ is_ansi_console = 0;
+ else {
+ is_ansi_console = (int)ansi_console(fd);
+ close(fd);
+ }
+ }
+
cylon_pos = m->jobs_in_progress_iteration % 14;
if (cylon_pos >= 8)
cylon_pos = 14 - cylon_pos;
- draw_cylon(cylon, sizeof(cylon), 6, cylon_pos);
+ draw_cylon(cylon, sizeof(cylon), 6, cylon_pos, (bool)is_ansi_console);

m->jobs_in_progress_iteration++;

diff --git src/shared/util.c src/shared/util.c
index a7aec5c..03b01de 100644
--- src/shared/util.c
+++ src/shared/util.c
@@ -2937,6 +2937,7 @@ int status_vprintf(const char *status, bool ellipse, bool ephemeral, const char
struct iovec iovec[6] = {};
int n = 0;
static bool prev_ephemeral;
+ static int is_ansi_console = -1;

assert(format);

@@ -2950,6 +2951,41 @@ int status_vprintf(const char *status, bool ellipse, bool ephemeral, const char
if (fd < 0)
return fd;

+ if (_unlikely_(is_ansi_console < 0))
+ is_ansi_console = (int)ansi_console(fd);
+
+ if (status && !is_ansi_console) {
+ const char *esc, *ptr;
+ esc = strchr(status, 0x1B);
+ if (esc && (ptr = strpbrk(esc, "SOFDTI*"))) {
+ switch(*ptr) {
+ case 'S':
+ status = " SKIP ";
+ break;
+ case 'O':
+ status = " OK ";
+ break;
+ case 'F':
+ status = "FAILED";
+ break;
+ case 'D':
+ status = "DEPEND";
+ break;
+ case 'T':
+ status = " TIME ";
+ break;
+ case 'I':
+ status = " INFO ";
+ break;
+ case '*':
+ status = " BUSY ";
+ break;
+ default:
+ break;
+ }
+ }
+ }
+
if (ellipse) {
char *e;
size_t emax, sl;
@@ -2972,8 +3008,12 @@ int status_vprintf(const char *status, bool ellipse, bool ephemeral, const char
}
}

- if (prev_ephemeral)
- IOVEC_SET_STRING(iovec[n++], "\r" ANSI_ERASE_TO_END_OF_LINE);
+ if (prev_ephemeral) {
+ if (is_ansi_console)
+ IOVEC_SET_STRING(iovec[n++], "\r" ANSI_ERASE_TO_END_OF_LINE);
+ else
+ IOVEC_SET_STRING(iovec[n++], "\r");
+ }
prev_ephemeral = ephemeral;

if (status) {
@@ -3220,8 +3260,22 @@ void columns_lines_cache_reset(int signum) {
bool on_tty(void) {
static int cached_on_tty = -1;

- if (_unlikely_(cached_on_tty < 0))
+ if (_unlikely_(cached_on_tty < 0)) {
cached_on_tty = isatty(STDOUT_FILENO) > 0;
+#if defined (__s390__) || defined (__s390x__)
+ if (cached_on_tty) {
+ const char *e = getenv("TERM");
+ if (!e)
+ return cached_on_tty;
+ if (streq(e, "dumb") || strneq(e, "ibm3", 4)) {
+ char *mode = NULL;
+ int r = parse_env_file("/proc/cmdline", WHITESPACE, "conmode", &mode, NULL);
+ if (r < 0 || !mode || !streq(mode, "3270"))
+ cached_on_tty = 0;
+ }
+ }
+#endif
+ }

return cached_on_tty;
}
@@ -3711,7 +3765,25 @@ bool tty_is_vc_resolve(const char *tty) {
const char *default_term_for_tty(const char *tty) {
assert(tty);

- return tty_is_vc_resolve(tty) ? "TERM=linux" : "TERM=vt102";
+ if (tty_is_vc_resolve(tty))
+ return "TERM=linux";
+
+ if (startswith(tty, "/dev/"))
+ tty += 5;
+
+#if defined (__s390__) || defined (__s390x__)
+ if (streq(tty, "ttyS0")) {
+ char *mode = NULL;
+ int r = parse_env_file("/proc/cmdline", WHITESPACE, "conmode", &mode, NULL);
+ if (r < 0 || !mode || !streq(mode, "3270"))
+ return "TERM=dumb";
+ if (streq(mode, "3270"))
+ return "TERM=ibm327x";
+ }
+ if (streq(tty, "ttyS1"))
+ return "TERM=vt220";
+#endif
+ return "TERM=vt102";
}

bool dirent_is_file(const struct dirent *de) {
diff --git src/shared/util.h src/shared/util.h
index 1796014..2dc918d 100644
--- src/shared/util.h
+++ src/shared/util.h
@@ -446,6 +446,7 @@ unsigned lines(void);
void columns_lines_cache_reset(int _unused_ signum);

bool on_tty(void);
+bool ansi_console(int fd);

static inline const char *ansi_highlight(void) {
return on_tty() ? ANSI_HIGHLIGHT_ON : "";
--
1.7.9.2
Zbigniew Jędrzejewski-Szmek
2014-06-20 03:06:06 UTC
Permalink
Post by Werner Fink
Therefore strip off the ANSI escape sequences for 3215 consoles
but support 3270 consoles if found.
---
src/core/manager.c | 24 ++++++++++++----
src/shared/util.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++---
src/shared/util.h | 1 +
3 files changed, 96 insertions(+), 9 deletions(-)
diff --git src/core/manager.c src/core/manager.c
index 0cb2044..f78ac18 100644
--- src/core/manager.c
+++ src/core/manager.c
@@ -112,7 +112,7 @@ static int manager_watch_jobs_in_progress(Manager *m) {
#define CYLON_BUFFER_EXTRA (2*(sizeof(ANSI_RED_ON)-1) + sizeof(ANSI_HIGHLIGHT_RED_ON)-1 + 2*(sizeof(ANSI_HIGHLIGHT_OFF)-1))
The goal of this patch seems correct. Nevertheless, it is quite
intrusive. Could you rework the patch to:

1. centralize the ansi console detection, like we do with on_tty(),
so it is implicitly checked on first access and then cached
inside of the function;
2. replace ANSI_RED_ON/OFF/... with ansi_red_on(), which calls the ansi console
status function and returns either ANSI_RED_ON/OFF/... or "";
3. instead of parsing the message to extract status in the status_printf
function, pass the status as a (numerical) argument, and use this
to decide the output. The mapping should be stored in a table;
4. use parse_proc_cmdline to parse /proc/cmdline.

Thanks,
Zbyszek
Post by Werner Fink
-static void draw_cylon(char buffer[], size_t buflen, unsigned width, unsigned pos) {
+static void draw_cylon(char buffer[], size_t buflen, unsigned width, unsigned pos, bool ansi_console) {
char *p = buffer;
assert(buflen >= CYLON_BUFFER_EXTRA + width + 1);
@@ -121,12 +121,14 @@ static void draw_cylon(char buffer[], size_t buflen, unsigned width, unsigned po
if (pos > 1) {
if (pos > 2)
p = mempset(p, ' ', pos-2);
- p = stpcpy(p, ANSI_RED_ON);
+ if (ansi_console)
+ p = stpcpy(p, ANSI_RED_ON);
*p++ = '*';
}
if (pos > 0 && pos <= width) {
- p = stpcpy(p, ANSI_HIGHLIGHT_RED_ON);
+ if (ansi_console)
+ p = stpcpy(p, ANSI_HIGHLIGHT_RED_ON);
*p++ = '*';
}
@@ -137,7 +139,8 @@ static void draw_cylon(char buffer[], size_t buflen, unsigned width, unsigned po
*p++ = '*';
if (pos < width-1)
p = mempset(p, ' ', width-1-pos);
- strcpy(p, ANSI_HIGHLIGHT_OFF);
+ if (ansi_console)
+ strcpy(p, ANSI_HIGHLIGHT_OFF);
}
}
@@ -154,6 +157,7 @@ void manager_flip_auto_status(Manager *m, bool enable) {
}
static void manager_print_jobs_in_progress(Manager *m) {
+ static int is_ansi_console = -1;
_cleanup_free_ char *job_of_n = NULL;
Iterator i;
Job *j;
@@ -178,10 +182,20 @@ static void manager_print_jobs_in_progress(Manager *m) {
assert(counter == print_nr + 1);
assert(j);
+ if (_unlikely_(is_ansi_console < 0)) {
+ int fd = open_terminal("/dev/console", O_RDONLY|O_NOCTTY|O_CLOEXEC);
+ if (fd < 0)
+ is_ansi_console = 0;
+ else {
+ is_ansi_console = (int)ansi_console(fd);
+ close(fd);
+ }
+ }
+
cylon_pos = m->jobs_in_progress_iteration % 14;
if (cylon_pos >= 8)
cylon_pos = 14 - cylon_pos;
- draw_cylon(cylon, sizeof(cylon), 6, cylon_pos);
+ draw_cylon(cylon, sizeof(cylon), 6, cylon_pos, (bool)is_ansi_console);
m->jobs_in_progress_iteration++;
diff --git src/shared/util.c src/shared/util.c
index a7aec5c..03b01de 100644
--- src/shared/util.c
+++ src/shared/util.c
@@ -2937,6 +2937,7 @@ int status_vprintf(const char *status, bool ellipse, bool ephemeral, const char
struct iovec iovec[6] = {};
int n = 0;
static bool prev_ephemeral;
+ static int is_ansi_console = -1;
assert(format);
@@ -2950,6 +2951,41 @@ int status_vprintf(const char *status, bool ellipse, bool ephemeral, const char
if (fd < 0)
return fd;
+ if (_unlikely_(is_ansi_console < 0))
+ is_ansi_console = (int)ansi_console(fd);
+
+ if (status && !is_ansi_console) {
+ const char *esc, *ptr;
+ esc = strchr(status, 0x1B);
+ if (esc && (ptr = strpbrk(esc, "SOFDTI*"))) {
+ switch(*ptr) {
+ status = " SKIP ";
+ break;
+ status = " OK ";
+ break;
+ status = "FAILED";
+ break;
+ status = "DEPEND";
+ break;
+ status = " TIME ";
+ break;
+ status = " INFO ";
+ break;
+ status = " BUSY ";
+ break;
+ break;
+ }
+ }
+ }
+
if (ellipse) {
char *e;
size_t emax, sl;
@@ -2972,8 +3008,12 @@ int status_vprintf(const char *status, bool ellipse, bool ephemeral, const char
}
}
- if (prev_ephemeral)
- IOVEC_SET_STRING(iovec[n++], "\r" ANSI_ERASE_TO_END_OF_LINE);
+ if (prev_ephemeral) {
+ if (is_ansi_console)
+ IOVEC_SET_STRING(iovec[n++], "\r" ANSI_ERASE_TO_END_OF_LINE);
+ else
+ IOVEC_SET_STRING(iovec[n++], "\r");
+ }
prev_ephemeral = ephemeral;
if (status) {
@@ -3220,8 +3260,22 @@ void columns_lines_cache_reset(int signum) {
bool on_tty(void) {
static int cached_on_tty = -1;
- if (_unlikely_(cached_on_tty < 0))
+ if (_unlikely_(cached_on_tty < 0)) {
cached_on_tty = isatty(STDOUT_FILENO) > 0;
+#if defined (__s390__) || defined (__s390x__)
+ if (cached_on_tty) {
+ const char *e = getenv("TERM");
+ if (!e)
+ return cached_on_tty;
+ if (streq(e, "dumb") || strneq(e, "ibm3", 4)) {
+ char *mode = NULL;
+ int r = parse_env_file("/proc/cmdline", WHITESPACE, "conmode", &mode, NULL);
+ if (r < 0 || !mode || !streq(mode, "3270"))
+ cached_on_tty = 0;
+ }
+ }
+#endif
+ }
return cached_on_tty;
}
@@ -3711,7 +3765,25 @@ bool tty_is_vc_resolve(const char *tty) {
const char *default_term_for_tty(const char *tty) {
assert(tty);
- return tty_is_vc_resolve(tty) ? "TERM=linux" : "TERM=vt102";
+ if (tty_is_vc_resolve(tty))
+ return "TERM=linux";
+
+ if (startswith(tty, "/dev/"))
+ tty += 5;
+
+#if defined (__s390__) || defined (__s390x__)
+ if (streq(tty, "ttyS0")) {
+ char *mode = NULL;
+ int r = parse_env_file("/proc/cmdline", WHITESPACE, "conmode", &mode, NULL);
+ if (r < 0 || !mode || !streq(mode, "3270"))
+ return "TERM=dumb";
+ if (streq(mode, "3270"))
+ return "TERM=ibm327x";
+ }
+ if (streq(tty, "ttyS1"))
+ return "TERM=vt220";
+#endif
+ return "TERM=vt102";
}
bool dirent_is_file(const struct dirent *de) {
diff --git src/shared/util.h src/shared/util.h
index 1796014..2dc918d 100644
--- src/shared/util.h
+++ src/shared/util.h
@@ -446,6 +446,7 @@ unsigned lines(void);
void columns_lines_cache_reset(int _unused_ signum);
bool on_tty(void);
+bool ansi_console(int fd);
static inline const char *ansi_highlight(void) {
return on_tty() ? ANSI_HIGHLIGHT_ON : "";
--
1.7.9.2
_______________________________________________
systemd-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Lennart Poettering
2014-06-20 17:03:47 UTC
Permalink
Post by Werner Fink
Therefore strip off the ANSI escape sequences for 3215 consoles
but support 3270 consoles if found.
Hmm, this looks messy.

Please add a global systemd PID 1 setting next to the arg_show_status
variable that controls whether color shall be enable for status
output. Then, make this configurable via the kernel cmdline, and in the
configuration file like show_status itself. That would be one patch.

And then, optionally prepare a second patch that adds support for
looking for TERM= on the kernel cmdline. If we see it there we will
initialize arg_show_status from it, and turn off color if it is set to
"dumb". Also, return this value as default terminal setting for $TERM
for /dev/console, but no other devices.

That way, s390 installations may simply set TERM=dumb on the kernel
cmdline and the right thing happens, their /dev/console will get color
disabled, and any raw tty clients will get TERM=dumb set for what they
need.
Post by Werner Fink
+#if defined (__s390__) || defined (__s390x__)
+ if (cached_on_tty) {
+ const char *e = getenv("TERM");
+ if (!e)
+ return cached_on_tty;
+ if (streq(e, "dumb") || strneq(e, "ibm3", 4)) {
+ char *mode = NULL;
+ int r = parse_env_file("/proc/cmdline", WHITESPACE, "conmode", &mode, NULL);
+ if (r < 0 || !mode || !streq(mode, "3270"))
+ cached_on_tty = 0;
+ }
+ }
+#endif
+
+#if defined (__s390__) || defined (__s390x__)
+ if (streq(tty, "ttyS0")) {
+ char *mode = NULL;
+ int r = parse_env_file("/proc/cmdline", WHITESPACE, "conmode", &mode, NULL);
+ if (r < 0 || !mode || !streq(mode, "3270"))
+ return "TERM=dumb";
+ if (streq(mode, "3270"))
+ return "TERM=ibm327x";
+ }
+ if (streq(tty, "ttyS1"))
+ return "TERM=vt220";
+#endif
+ return "TERM=vt102";
Yuck. No! We won't do such messy checks. No arch-specific stuff. No
hardcoding of ttyS0 or ttyS1 or whatever other tty. Support for checking
TERM= in the kernel cmdline should be something generic.

Sorry, but we will not add any specific per-arch hacks to systemd like
this. We can fix this, but then find nice generic ways to do this, but
not this messy pile of s390 hardcoded assumption. That's not how we do
things.

Also, we are not going to add code for any specific weird terminal
settings. We will do three levels: TERM=linux for the full Linux
console, TERM=vt102 otherwise, and TERM=dumb for the crap that can't do
TERM=vt102. But we are not going to hardcode support for any other
hardware into this. This is 2014. If IBM still can't build ANSI support in
their terminals, then that's the own fault, and they will not get any
support beyond TERM=dumb and no color. We will not work-around IBM's
choices in systemd's source tree.

Sorry.

Lennart
--
Lennart Poettering, Red Hat
Dr. Werner Fink
2014-08-01 14:07:28 UTC
Permalink
Post by Lennart Poettering
Also, we are not going to add code for any specific weird terminal
settings. We will do three levels: TERM=linux for the full Linux
console, TERM=vt102 otherwise, and TERM=dumb for the crap that can't do
TERM=vt102. But we are not going to hardcode support for any other
hardware into this. This is 2014. If IBM still can't build ANSI support in
their terminals, then that's the own fault, and they will not get any
support beyond TERM=dumb and no color. We will not work-around IBM's
choices in systemd's source tree.
Sorry.
Maybe I'm wrong but this looks like you have no idea how S390x works.
The terminal emulation is done by the linux kernel which uses the
capabilities of the two block oriented terminal interfaces of the
firmware of a s390 (https://en.wikipedia.org/wiki/IBM_3270). One of
the block oriented terminals are able to do coloring (3270) and the
other (3215) is not. Writing Escape Sequences (including carriage return)
will cause very strange effects with the later block oriented terminal
(3215) and this is the default interface if you to not use

conmode=3270

to switch to 3270 mode. With 3270 the kernel then maps Escape Sequences
with the help of linux/drivers/s390/char/tty3270.c onto the real firmware
interface for the chosen block oriented terminal.

I do not think that IBM will ``fix'' their s390 firmware due systemd.
Beside this, what happens if the main system console is a printer
and not a virtual device. Or what does happen if it is a serial
console which can not handle Escape Sequences or can not handle all
Escape Sequences or use other Escape Sequences as a common VT102?

Werner
--
"Having a smoking section in a restaurant is like having
a peeing section in a swimming pool." -- Edward Burr
Lennart Poettering
2014-08-01 14:27:17 UTC
Permalink
Post by Dr. Werner Fink
Post by Lennart Poettering
Also, we are not going to add code for any specific weird terminal
settings. We will do three levels: TERM=linux for the full Linux
console, TERM=vt102 otherwise, and TERM=dumb for the crap that can't do
TERM=vt102. But we are not going to hardcode support for any other
hardware into this. This is 2014. If IBM still can't build ANSI support in
their terminals, then that's the own fault, and they will not get any
support beyond TERM=dumb and no color. We will not work-around IBM's
choices in systemd's source tree.
Sorry.
Maybe I'm wrong but this looks like you have no idea how S390x works.
The terminal emulation is done by the linux kernel which uses the
capabilities of the two block oriented terminal interfaces of the
firmware of a s390 (https://en.wikipedia.org/wiki/IBM_3270). One of
the block oriented terminals are able to do coloring (3270) and the
other (3215) is not. Writing Escape Sequences (including carriage return)
will cause very strange effects with the later block oriented terminal
(3215) and this is the default interface if you to not use
conmode=3270
to switch to 3270 mode. With 3270 the kernel then maps Escape Sequences
with the help of linux/drivers/s390/char/tty3270.c onto the real firmware
interface for the chosen block oriented terminal.
I do not think that IBM will ``fix'' their s390 firmware due systemd.
Beside this, what happens if the main system console is a printer
and not a virtual device. Or what does happen if it is a serial
console which can not handle Escape Sequences or can not handle all
Escape Sequences or use other Escape Sequences as a common VT102?
That's all good and fine. But we are not going to add explicit support
for this exotic stuff in systemd.

For cases like this we should support TERM=dumb. People won't get color
during boot, but things will still work great otherwise.

Lennart
--
Lennart Poettering, Red Hat
Werner Fink
2014-06-13 14:41:06 UTC
Permalink
From: Ruediger Oertel <***@suse.de>

Process 1 (aka init) needs to be started with an empty signal mask.
That includes the process 1 that's started after the initrd is finished.
When the initrd is using systemd (as it does with dracut based initrds)
then it is systemd that calls the real init. Normally this is systemd
again, except when the user uses for instance "init=/bin/bash" on the
kernel command line.

---
src/core/main.c | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git src/core/main.c src/core/main.c
index d5d1ee2..c082354 100644
--- src/core/main.c
+++ src/core/main.c
@@ -1831,6 +1831,7 @@ finish:
if (reexecute) {
const char **args;
unsigned i, args_size;
+ sigset_t ss, o_ss;

/* Close and disarm the watchdog, so that the new
* instance can reinitialize it, but doesn't get
@@ -1914,6 +1915,11 @@ finish:
args[i++] = NULL;
assert(i <= args_size);

+ /* reenable any blocked signals, especially important
+ * if we switch from initial ramdisk to init=... */
+ sigemptyset(&ss);
+ sigprocmask(SIG_SETMASK,&ss,&o_ss);
+
if (switch_root_init) {
args[0] = switch_root_init;
execv(args[0], (char* const*) args);
@@ -1932,6 +1938,9 @@ finish:
log_error("Failed to execute /bin/sh, giving up: %m");
} else
log_warning("Failed to execute /sbin/init, giving up: %m");
+
+ /* back to saved state if reexec failed */
+ sigprocmask(SIG_SETMASK,&o_ss,NULL);
}

if (arg_serialization) {
--
1.7.9.2
Tom Gundersen
2014-06-13 15:49:24 UTC
Permalink
Post by Werner Fink
Process 1 (aka init) needs to be started with an empty signal mask.
That includes the process 1 that's started after the initrd is finished.
When the initrd is using systemd (as it does with dracut based initrds)
then it is systemd that calls the real init. Normally this is systemd
again, except when the user uses for instance "init=/bin/bash" on the
kernel command line.
Why is this necessary for /bin/bash, but not for /lib/systemd/systemd?
Please include the explanation in the commit message.

Cheers,

Tom
Post by Werner Fink
---
src/core/main.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git src/core/main.c src/core/main.c
index d5d1ee2..c082354 100644
--- src/core/main.c
+++ src/core/main.c
if (reexecute) {
const char **args;
unsigned i, args_size;
+ sigset_t ss, o_ss;
/* Close and disarm the watchdog, so that the new
* instance can reinitialize it, but doesn't get
args[i++] = NULL;
assert(i <= args_size);
+ /* reenable any blocked signals, especially important
+ * if we switch from initial ramdisk to init=... */
+ sigemptyset(&ss);
+ sigprocmask(SIG_SETMASK,&ss,&o_ss);
+
if (switch_root_init) {
args[0] = switch_root_init;
execv(args[0], (char* const*) args);
log_error("Failed to execute /bin/sh, giving up: %m");
} else
log_warning("Failed to execute /sbin/init, giving up: %m");
+
+ /* back to saved state if reexec failed */
+ sigprocmask(SIG_SETMASK,&o_ss,NULL);
}
if (arg_serialization) {
--
1.7.9.2
_______________________________________________
systemd-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Dr. Werner Fink
2014-06-16 12:42:07 UTC
Permalink
Post by Tom Gundersen
Post by Werner Fink
Process 1 (aka init) needs to be started with an empty signal mask.
That includes the process 1 that's started after the initrd is finished.
When the initrd is using systemd (as it does with dracut based initrds)
then it is systemd that calls the real init. Normally this is systemd
again, except when the user uses for instance "init=/bin/bash" on the
kernel command line.
Why is this necessary for /bin/bash, but not for /lib/systemd/systemd?
Please include the explanation in the commit message.
I do not understand your question. AFAIK the signal mask is set by
systemd accordingly to man:systemd.service(5) ... the only problem
with this schem is that /bin/bash nor any other shell does reset
its signal mask. I guess that systemd will support the init=/bin/bash
on the kernel command line. IMHO this requires a clean signal mask.

Werner
Post by Tom Gundersen
Post by Werner Fink
---
src/core/main.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git src/core/main.c src/core/main.c
index d5d1ee2..c082354 100644
--- src/core/main.c
+++ src/core/main.c
if (reexecute) {
const char **args;
unsigned i, args_size;
+ sigset_t ss, o_ss;
/* Close and disarm the watchdog, so that the new
* instance can reinitialize it, but doesn't get
args[i++] = NULL;
assert(i <= args_size);
+ /* reenable any blocked signals, especially important
+ * if we switch from initial ramdisk to init=... */
+ sigemptyset(&ss);
+ sigprocmask(SIG_SETMASK,&ss,&o_ss);
+
if (switch_root_init) {
args[0] = switch_root_init;
execv(args[0], (char* const*) args);
log_error("Failed to execute /bin/sh, giving up: %m");
} else
log_warning("Failed to execute /sbin/init, giving up: %m");
+
+ /* back to saved state if reexec failed */
+ sigprocmask(SIG_SETMASK,&o_ss,NULL);
}
if (arg_serialization) {
--
1.7.9.2
Werner
--
"Having a smoking section in a restaurant is like having
a peeing section in a swimming pool." -- Edward Burr
David Herrmann
2014-06-16 12:51:47 UTC
Permalink
Hi
Post by Dr. Werner Fink
Post by Tom Gundersen
Post by Werner Fink
Process 1 (aka init) needs to be started with an empty signal mask.
That includes the process 1 that's started after the initrd is finished.
When the initrd is using systemd (as it does with dracut based initrds)
then it is systemd that calls the real init. Normally this is systemd
again, except when the user uses for instance "init=/bin/bash" on the
kernel command line.
Why is this necessary for /bin/bash, but not for /lib/systemd/systemd?
Please include the explanation in the commit message.
I do not understand your question. AFAIK the signal mask is set by
systemd accordingly to man:systemd.service(5) ... the only problem
with this schem is that /bin/bash nor any other shell does reset
its signal mask. I guess that systemd will support the init=/bin/bash
on the kernel command line. IMHO this requires a clean signal mask.
The question was, why is this fix related to init=/bin/bash? What does
bash do different than systemd that it requires this fix? That
information should be placed in the commit-message.

The fix itself looks good and if you called it "restore signal-mask
before executing init=" it'd be fine. However, calling it "support
init=/bin/bash" implies that this is only needed for init=/bin/bash.
This, however, is just a side-effect of this fix, because systemd
simply doesn't care for the initial sigmask.

I'll fix up the commit-msg and apply it. No need to resend.

Thanks
David
Dr. Werner Fink
2014-06-16 13:01:07 UTC
Permalink
Post by David Herrmann
Hi
The question was, why is this fix related to init=/bin/bash? What does
bash do different than systemd that it requires this fix? That
information should be placed in the commit-message.
The fix itself looks good and if you called it "restore signal-mask
before executing init=" it'd be fine. However, calling it "support
init=/bin/bash" implies that this is only needed for init=/bin/bash.
This, however, is just a side-effect of this fix, because systemd
simply doesn't care for the initial sigmask.
I'll fix up the commit-msg and apply it. No need to resend.
Thanks a lot
Werner
--
"Having a smoking section in a restaurant is like having
a peeing section in a swimming pool." -- Edward Burr
Lennart Poettering
2014-06-20 16:39:57 UTC
Permalink
Post by David Herrmann
Hi
Post by Dr. Werner Fink
Post by Tom Gundersen
Post by Werner Fink
Process 1 (aka init) needs to be started with an empty signal mask.
That includes the process 1 that's started after the initrd is finished.
When the initrd is using systemd (as it does with dracut based initrds)
then it is systemd that calls the real init. Normally this is systemd
again, except when the user uses for instance "init=/bin/bash" on the
kernel command line.
Why is this necessary for /bin/bash, but not for /lib/systemd/systemd?
Please include the explanation in the commit message.
I do not understand your question. AFAIK the signal mask is set by
systemd accordingly to man:systemd.service(5) ... the only problem
with this schem is that /bin/bash nor any other shell does reset
its signal mask. I guess that systemd will support the init=/bin/bash
on the kernel command line. IMHO this requires a clean signal mask.
The question was, why is this fix related to init=/bin/bash? What does
bash do different than systemd that it requires this fix? That
information should be placed in the commit-message.
The fix itself looks good and if you called it "restore signal-mask
before executing init=" it'd be fine. However, calling it "support
init=/bin/bash" implies that this is only needed for init=/bin/bash.
This, however, is just a side-effect of this fix, because systemd
simply doesn't care for the initial sigmask.
I'll fix up the commit-msg and apply it. No need to resend.
I have made another change on top now: there's no need to remember the
old signal mask if we are going to die anyway. And also, we will now
also reset the signal handlers, so that we don't pass any SIG_IGN
handlers to the replacement process.

Lennart
--
Lennart Poettering, Red Hat
Werner Fink
2014-06-13 14:41:03 UTC
Permalink
From: Frederic Crozat <***@suse.com>

Otherwise any agetty on the devices of the system console will
conflict with sulogin and its input.

---
units/***@.service.m4 | 1 +
units/rescue.target | 1 +
units/serial-***@.service.m4 | 1 +
3 files changed, 3 insertions(+)

diff --git units/***@.service.m4 units/***@.service.m4
index aa853b8..7fb2db8 100644
--- units/***@.service.m4
+++ units/***@.service.m4
@@ -9,6 +9,7 @@
Description=Getty on %I
Documentation=man:agetty(8) man:systemd-getty-generator(8)
Documentation=http://0pointer.de/blog/projects/serial-console.html
+Conflicts=rescue.service
After=systemd-user-sessions.service plymouth-quit-wait.service
m4_ifdef(`HAVE_SYSV_COMPAT',
After=rc-local.service
diff --git units/rescue.target units/rescue.target
index 3f59b14..20f6841 100644
--- units/rescue.target
+++ units/rescue.target
@@ -10,6 +10,7 @@ Description=Rescue Mode
Documentation=man:systemd.special(7)
Requires=sysinit.target rescue.service
After=sysinit.target rescue.service
+Conflicts=getty.target
AllowIsolate=yes

[Install]
diff --git units/serial-***@.service.m4 units/serial-***@.service.m4
index 4ac51e7..4b1ab20 100644
--- units/serial-***@.service.m4
+++ units/serial-***@.service.m4
@@ -10,6 +10,7 @@ Description=Serial Getty on %I
Documentation=man:agetty(8) man:systemd-getty-generator(8)
Documentation=http://0pointer.de/blog/projects/serial-console.html
BindsTo=dev-%i.device
+Conflicts=rescue.service
After=dev-%i.device systemd-user-sessions.service plymouth-quit-wait.service
m4_ifdef(`HAVE_SYSV_COMPAT',
After=rc-local.service
--
1.7.9.2
Zbigniew Jędrzejewski-Szmek
2014-06-20 03:51:19 UTC
Permalink
Post by Werner Fink
Otherwise any agetty on the devices of the system console will
conflict with sulogin and its input.
---
units/rescue.target | 1 +
3 files changed, 3 insertions(+)
index aa853b8..7fb2db8 100644
@@ -9,6 +9,7 @@
Description=Getty on %I
Documentation=man:agetty(8) man:systemd-getty-generator(8)
Documentation=http://0pointer.de/blog/projects/serial-console.html
+Conflicts=rescue.service
systemd has tty arbitration logic. Maybe it is failing in this
case for some reason... rescue.service has StandardInput=tty-force,
which means that when it is started, previous owners of the tty
should be killed. And the other way around, getty@ should wait
until the previous owner goes away on its own. Explicit conflicts
should be unnecessary.

Zbyszek
Post by Werner Fink
After=systemd-user-sessions.service plymouth-quit-wait.service
m4_ifdef(`HAVE_SYSV_COMPAT',
After=rc-local.service
diff --git units/rescue.target units/rescue.target
index 3f59b14..20f6841 100644
--- units/rescue.target
+++ units/rescue.target
@@ -10,6 +10,7 @@ Description=Rescue Mode
Documentation=man:systemd.special(7)
Requires=sysinit.target rescue.service
After=sysinit.target rescue.service
+Conflicts=getty.target
AllowIsolate=yes
[Install]
index 4ac51e7..4b1ab20 100644
@@ -10,6 +10,7 @@ Description=Serial Getty on %I
Documentation=man:agetty(8) man:systemd-getty-generator(8)
Documentation=http://0pointer.de/blog/projects/serial-console.html
BindsTo=dev-%i.device
+Conflicts=rescue.service
After=dev-%i.device systemd-user-sessions.service plymouth-quit-wait.service
m4_ifdef(`HAVE_SYSV_COMPAT',
After=rc-local.service
--
1.7.9.2
_______________________________________________
systemd-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Werner Fink
2014-06-13 14:41:02 UTC
Permalink
From: Frederic Crozat <***@suse.com>

---
units/systemd-ask-password-wall.service.in | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git units/systemd-ask-password-wall.service.in units/systemd-ask-password-wall.service.in
index 0eaa274..179b010 100644
--- units/systemd-ask-password-wall.service.in
+++ units/systemd-ask-password-wall.service.in
@@ -8,7 +8,8 @@
[Unit]
Description=Forward Password Requests to Wall
Documentation=man:systemd-ask-password-console.service(8)
-After=systemd-user-sessions.service
+Wants=getty.target
+After=systemd-user-sessions.service getty.target

[Service]
ExecStartPre=-@SYSTEMCTL@ stop systemd-ask-password-console.path systemd-ask-password-console.service systemd-ask-password-plymouth.path systemd-ask-password-plymouth.service
--
1.7.9.2
Zbigniew Jędrzejewski-Szmek
2014-06-20 02:48:12 UTC
Permalink
Post by Werner Fink
---
units/systemd-ask-password-wall.service.in | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git units/systemd-ask-password-wall.service.in units/systemd-ask-password-wall.service.in
index 0eaa274..179b010 100644
--- units/systemd-ask-password-wall.service.in
+++ units/systemd-ask-password-wall.service.in
@@ -8,7 +8,8 @@
[Unit]
Description=Forward Password Requests to Wall
Documentation=man:systemd-ask-password-console.service(8)
-After=systemd-user-sessions.service
+Wants=getty.target
+After=systemd-user-sessions.service getty.target
Why? It feels wrong to pull in getty.target from a password
service. Password entry mechanisms and user login machanisms should be
loosely coupled. One might e.g. use one manually configured getty to
login into the system and e.g. enter passwords...

Zbyszek
Post by Werner Fink
[Service]
--
1.7.9.2
_______________________________________________
systemd-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Frederic Crozat
2014-06-20 08:06:52 UTC
Permalink
Le vendredi 20 juin 2014 à 04:48 +0200, Zbigniew Jędrzejewski-Szmek a
Post by Zbigniew Jędrzejewski-Szmek
Post by Werner Fink
---
units/systemd-ask-password-wall.service.in | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git units/systemd-ask-password-wall.service.in units/systemd-ask-password-wall.service.in
index 0eaa274..179b010 100644
--- units/systemd-ask-password-wall.service.in
+++ units/systemd-ask-password-wall.service.in
@@ -8,7 +8,8 @@
[Unit]
Description=Forward Password Requests to Wall
Documentation=man:systemd-ask-password-console.service(8)
-After=systemd-user-sessions.service
+Wants=getty.target
+After=systemd-user-sessions.service getty.target
Why? It feels wrong to pull in getty.target from a password
service. Password entry mechanisms and user login machanisms should be
loosely coupled. One might e.g. use one manually configured getty to
login into the system and e.g. enter passwords...
Hmm, the initial patch was ensuring this service was only started after
***@tty1.
Werner Fink
2014-06-13 14:41:07 UTC
Permalink
It was seen only once, nevertheless this change does avoid the
assert on s->user->slice in session_start_scope()

---
src/login/logind-session.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git src/login/logind-session.c src/login/logind-session.c
index fdeacb1..86ce418 100644
--- src/login/logind-session.c
+++ src/login/logind-session.c
@@ -560,6 +560,12 @@ int session_start(Session *s) {
if (r < 0)
return r;

+ if (!s->user->slice) {
+ if (errno)
+ return -errno;
+ return -ESTALE;
+ }
+
/* Create cgroup */
r = session_start_scope(s);
if (r < 0)
--
1.7.9.2
Zbigniew Jędrzejewski-Szmek
2014-06-20 03:41:04 UTC
Permalink
Post by Werner Fink
It was seen only once, nevertheless this change does avoid the
assert on s->user->slice in session_start_scope()
Yeah, it seems possible that the slice will not be started and
the assert can fail. It seems that instead of checking s->user->slice
after the fact, user_start() should propagate the error code.
Possibly < 0 for fatal failure, 0 for slice or service start
failure, 1 on success.

Zbyszek
Post by Werner Fink
diff --git src/login/logind-session.c src/login/logind-session.c
index fdeacb1..86ce418 100644
--- src/login/logind-session.c
+++ src/login/logind-session.c
@@ -560,6 +560,12 @@ int session_start(Session *s) {
if (r < 0)
return r;
+ if (!s->user->slice) {
+ if (errno)
+ return -errno;
+ return -ESTALE;
+ }
+
/* Create cgroup */
r = session_start_scope(s);
if (r < 0)
--
1.7.9.2
_______________________________________________
systemd-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Werner Fink
2014-06-13 14:41:08 UTC
Permalink
Otherwise this leads to a poweroff at boot time.

---
units/console-shell.service.m4.in | 3 ---
1 file changed, 3 deletions(-)

diff --git units/console-shell.service.m4.in units/console-shell.service.m4.in
index 3f4904a..295dc8d 100644
--- units/console-shell.service.m4.in
+++ units/console-shell.service.m4.in
@@ -26,6 +26,3 @@ StandardError=inherit
KillMode=process
IgnoreSIGPIPE=no
SendSIGHUP=yes
-
-[Install]
-WantedBy=getty.target
--
1.7.9.2
Andrey Borzenkov
2014-06-17 02:43:12 UTC
Permalink
В Fri, 13 Jun 2014 16:41:08 +0200
Post by Werner Fink
Otherwise this leads to a poweroff at boot time.
I'm not sure what conflict you mean here. Could you provide bug
reference?
Post by Werner Fink
---
units/console-shell.service.m4.in | 3 ---
1 file changed, 3 deletions(-)
diff --git units/console-shell.service.m4.in units/console-shell.service.m4.in
index 3f4904a..295dc8d 100644
--- units/console-shell.service.m4.in
+++ units/console-shell.service.m4.in
@@ -26,6 +26,3 @@ StandardError=inherit
KillMode=process
IgnoreSIGPIPE=no
SendSIGHUP=yes
-
-[Install]
-WantedBy=getty.target
Werner Fink
2014-06-13 14:41:09 UTC
Permalink
That is: set NOATIME, NOCOW, and NOCOMP for the journal directory

---
src/journal/journald-server.c | 29 +++++++++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)

diff --git src/journal/journald-server.c src/journal/journald-server.c
index eda5dcf..37d6dc3 100644
--- src/journal/journald-server.c
+++ src/journal/journald-server.c
@@ -21,6 +21,7 @@

#include <sys/signalfd.h>
#include <sys/ioctl.h>
+#include <linux/fs.h>
#include <linux/sockios.h>
#include <sys/statvfs.h>
#include <sys/mman.h>
@@ -920,7 +921,7 @@ finish:


static int system_journal_open(Server *s) {
- int r;
+ int r, fd;
char *fn;
sd_id128_t machine;
char ids[33];
@@ -947,7 +948,31 @@ static int system_journal_open(Server *s) {
(void) mkdir("/var/log/journal/", 0755);

fn = strappenda("/var/log/journal/", ids);
- (void) mkdir(fn, 0755);
+ (void)mkdir(fn, 0755);
+
+ /*
+ * On journaling and/or compressing file systems avoid doubling the
+ * efforts for the system, that is set NOCOW and NOCOMP inode flags.
+ * Check for every single flag as otherwise some of the file systems
+ * may return EOPNOTSUPP on one unkown flag (like BtrFS does).
+ */
+ if ((fd = open(fn, O_DIRECTORY)) >= 0) {
+ long flags;
+ if (ioctl(fd, FS_IOC_GETFLAGS, &flags) == 0) {
+ int old = flags;
+ if (!(flags&FS_NOATIME_FL) && ioctl(fd, FS_IOC_SETFLAGS, flags|FS_NOATIME_FL) == 0)
+ flags |= FS_NOATIME_FL;
+ if (!(flags&FS_NOCOW_FL) && ioctl(fd, FS_IOC_SETFLAGS, flags|FS_NOCOW_FL) == 0)
+ flags |= FS_NOCOW_FL;
+ if (!(flags&FS_NOCOMP_FL) && s->compress) {
+ flags &= ~FS_COMPR_FL;
+ flags |= FS_NOCOMP_FL;
+ }
+ if (old != flags)
+ ioctl(fd, FS_IOC_SETFLAGS, flags);
+ }
+ close(fd);
+ }

fn = strappenda(fn, "/system.journal");
r = journal_file_open_reliably(fn, O_RDWR|O_CREAT, 0640, s->compress, s->seal, &s->system_metrics, s->mmap, NULL, &s->system_journal);
--
1.7.9.2
Cristian Rodríguez
2014-06-16 15:10:15 UTC
Permalink
Post by Werner Fink
That is: set NOATIME, NOCOW, and NOCOMP for the journal directory
---
src/journal/journald-server.c | 29 +++++++++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)
diff --git src/journal/journald-server.c src/journal/journald-server.c
index eda5dcf..37d6dc3 100644
--- src/journal/journald-server.c
+++ src/journal/journald-server.c
@@ -21,6 +21,7 @@
#include <sys/signalfd.h>
#include <sys/ioctl.h>
+#include <linux/fs.h>
#include <linux/sockios.h>
#include <sys/statvfs.h>
#include <sys/mman.h>
static int system_journal_open(Server *s) {
- int r;
+ int r, fd;
_cleanup_close_ ...
Post by Werner Fink
+ /*
+ * On journaling and/or compressing file systems avoid doubling the
+ * efforts for the system, that is set NOCOW and NOCOMP inode flags.
+ * Check for every single flag as otherwise some of the file systems
+ * may return EOPNOTSUPP on one unkown flag (like BtrFS does).
+ */
+ if ((fd = open(fn, O_DIRECTORY)) >= 0) {
O_CLOEXEC...
Post by Werner Fink
+ long flags;
+ if (ioctl(fd, FS_IOC_GETFLAGS, &flags) == 0) {
+ int old = flags;
+ if (!(flags&FS_NOATIME_FL) && ioctl(fd, FS_IOC_SETFLAGS, flags|FS_NOATIME_FL) == 0)
+ flags |= FS_NOATIME_FL;
+ if (!(flags&FS_NOCOW_FL) && ioctl(fd, FS_IOC_SETFLAGS, flags|FS_NOCOW_FL) == 0)
+ flags |= FS_NOCOW_FL;
+ if (!(flags&FS_NOCOMP_FL) && s->compress) {
+ flags &= ~FS_COMPR_FL;
+ flags |= FS_NOCOMP_FL;
+ }
+ if (old != flags)
+ ioctl(fd, FS_IOC_SETFLAGS, flags);
+ }
+ close(fd);
+ }
I agree that this should be done, however I remain unconvinced this is
the right place to do it..
--
Cristian
"I don't know the key to success, but the key to failure is trying to
please everybody."
Dr. Werner Fink
2014-06-18 09:44:31 UTC
Permalink
Post by Cristian Rodríguez
Post by Werner Fink
That is: set NOATIME, NOCOW, and NOCOMP for the journal directory
---
src/journal/journald-server.c | 29 +++++++++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)
diff --git src/journal/journald-server.c src/journal/journald-server.c
index eda5dcf..37d6dc3 100644
--- src/journal/journald-server.c
+++ src/journal/journald-server.c
@@ -21,6 +21,7 @@
#include <sys/signalfd.h>
#include <sys/ioctl.h>
+#include <linux/fs.h>
#include <linux/sockios.h>
#include <sys/statvfs.h>
#include <sys/mman.h>
static int system_journal_open(Server *s) {
- int r;
+ int r, fd;
_cleanup_close_ ...
Post by Werner Fink
+ /*
+ * On journaling and/or compressing file systems avoid doubling the
+ * efforts for the system, that is set NOCOW and NOCOMP inode flags.
+ * Check for every single flag as otherwise some of the file systems
+ * may return EOPNOTSUPP on one unkown flag (like BtrFS does).
+ */
+ if ((fd = open(fn, O_DIRECTORY)) >= 0) {
O_CLOEXEC...
Post by Werner Fink
+ long flags;
+ if (ioctl(fd, FS_IOC_GETFLAGS, &flags) == 0) {
+ int old = flags;
+ if (!(flags&FS_NOATIME_FL) && ioctl(fd, FS_IOC_SETFLAGS, flags|FS_NOATIME_FL) == 0)
+ flags |= FS_NOATIME_FL;
+ if (!(flags&FS_NOCOW_FL) && ioctl(fd, FS_IOC_SETFLAGS, flags|FS_NOCOW_FL) == 0)
+ flags |= FS_NOCOW_FL;
+ if (!(flags&FS_NOCOMP_FL) && s->compress) {
+ flags &= ~FS_COMPR_FL;
+ flags |= FS_NOCOMP_FL;
+ }
+ if (old != flags)
+ ioctl(fd, FS_IOC_SETFLAGS, flags);
+ }
+ close(fd);
+ }
I agree that this should be done, however I remain unconvinced this is
the right place to do it..
IMHO this is the correct place as it helps to speed up systemd-journal
on BtrFS. This was the reason for this patch and is tested even if the
patch does not use _cleanup_close_ and O_CLOEXEC ;)


Werner
--
"Having a smoking section in a restaurant is like having
a peeing section in a swimming pool." -- Edward Burr
Goffredo Baroncelli
2014-06-18 18:10:03 UTC
Permalink
Hi Fink
Post by Werner Fink
That is: set NOATIME, NOCOW, and NOCOMP for the journal directory
---
src/journal/journald-server.c | 29 +++++++++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)
diff --git src/journal/journald-server.c src/journal/journald-server.c
index eda5dcf..37d6dc3 100644
--- src/journal/journald-server.c
+++ src/journal/journald-server.c
@@ -21,6 +21,7 @@
#include <sys/signalfd.h>
#include <sys/ioctl.h>
+#include <linux/fs.h>
#include <linux/sockios.h>
#include <sys/statvfs.h>
#include <sys/mman.h>
static int system_journal_open(Server *s) {
- int r;
+ int r, fd;
char *fn;
sd_id128_t machine;
char ids[33];
@@ -947,7 +948,31 @@ static int system_journal_open(Server *s) {
(void) mkdir("/var/log/journal/", 0755);
fn = strappenda("/var/log/journal/", ids);
- (void) mkdir(fn, 0755);
+ (void)mkdir(fn, 0755);
+
+ /*
+ * On journaling and/or compressing file systems avoid doubling the
+ * efforts for the system, that is set NOCOW and NOCOMP inode flags.
+ * Check for every single flag as otherwise some of the file systems
+ * may return EOPNOTSUPP on one unkown flag (like BtrFS does).
+ */
+ if ((fd = open(fn, O_DIRECTORY)) >= 0) {
+ long flags;
+ if (ioctl(fd, FS_IOC_GETFLAGS, &flags) == 0) {
+ int old = flags;
+ if (!(flags&FS_NOATIME_FL) && ioctl(fd, FS_IOC_SETFLAGS, flags|FS_NOATIME_FL) == 0)
+ flags |= FS_NOATIME_FL;
+ if (!(flags&FS_NOCOW_FL) && ioctl(fd, FS_IOC_SETFLAGS, flags|FS_NOCOW_FL) == 0)
+ flags |= FS_NOCOW_FL;
If I read correctly, you want set UN-conditionally the NOCOW behavior. Please, please, please DON'T DO that.

The NOCOW behavior is not without disadvantage: yes it increase the performance but
the file also lost the btrfs checksum protection; when BTRFS manage the disks in RAID mode and a corruption happens, it uses the checksum to select the correct mirror during the reading. If you set UN-conditionally the NOCOW behavior you lost this capability even if the user _want it_ (and if they spend moneys in two or more disks, it is likely they _want it_).

Moreover the NOCOW flags has some "strange" behavior when a NOCOW file is snapshotted (it lost the NOCOW property); this may lead to irregular performance.

If you want it, it must be configurable at least with a sane default (which IMHO should be "do nothing", following the "least surprise" rule).

If you are looking to something like that, I suggest also to defrag the journal file before the open (but still as configurable option, and considering the "least surprise" rule).

BR
G.Baroncelli
Post by Werner Fink
+ if (!(flags&FS_NOCOMP_FL) && s->compress) {
+ flags &= ~FS_COMPR_FL;
+ flags |= FS_NOCOMP_FL;
+ }
+ if (old != flags)
+ ioctl(fd, FS_IOC_SETFLAGS, flags);
+ }
+ close(fd);
+ }
fn = strappenda(fn, "/system.journal");
r = journal_file_open_reliably(fn, O_RDWR|O_CREAT, 0640, s->compress, s->seal, &s->system_metrics, s->mmap, NULL, &s->system_journal);
--
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
Zbigniew Jędrzejewski-Szmek
2014-06-20 03:21:52 UTC
Permalink
Post by Goffredo Baroncelli
Hi Fink
Post by Werner Fink
That is: set NOATIME, NOCOW, and NOCOMP for the journal directory
If I read correctly, you want set UN-conditionally the NOCOW behavior. Please, please, please DON'T DO that.
The NOCOW behavior is not without disadvantage: yes it increase the performance but
the file also lost the btrfs checksum protection; when BTRFS manage the disks in RAID mode and a corruption happens, it uses the checksum to select the correct mirror during the reading. If you set UN-conditionally the NOCOW behavior you lost this capability even if the user _want it_ (and if they spend moneys in two or more disks, it is likely they _want it_).
Moreover the NOCOW flags has some "strange" behavior when a NOCOW file is snapshotted (it lost the NOCOW property); this may lead to irregular performance.
If you want it, it must be configurable at least with a sane default (which IMHO should be "do nothing", following the "least surprise" rule).
If you are looking to something like that, I suggest also to defrag the journal file before the open (but still as configurable option, and considering the "least surprise" rule).
Yes, this is too much of a hack. Various defragging approaches
seem better.

Zbyszek
Lennart Poettering
2014-06-20 16:44:53 UTC
Permalink
Post by Werner Fink
fn = strappenda("/var/log/journal/", ids);
- (void) mkdir(fn, 0755);
+ (void)mkdir(fn, 0755);
+
+ /*
+ * On journaling and/or compressing file systems avoid doubling the
+ * efforts for the system, that is set NOCOW and NOCOMP inode flags.
+ * Check for every single flag as otherwise some of the file systems
+ * may return EOPNOTSUPP on one unkown flag (like BtrFS does).
+ */
+ if ((fd = open(fn, O_DIRECTORY)) >= 0) {
+ long flags;
+ if (ioctl(fd, FS_IOC_GETFLAGS, &flags) == 0) {
+ int old = flags;
+ if (!(flags&FS_NOATIME_FL) && ioctl(fd, FS_IOC_SETFLAGS, flags|FS_NOATIME_FL) == 0)
+ flags |= FS_NOATIME_FL;
+ if (!(flags&FS_NOCOW_FL) && ioctl(fd, FS_IOC_SETFLAGS, flags|FS_NOCOW_FL) == 0)
+ flags |= FS_NOCOW_FL;
+ if (!(flags&FS_NOCOMP_FL) && s->compress) {
+ flags &= ~FS_COMPR_FL;
+ flags |= FS_NOCOMP_FL;
+ }
+ if (old != flags)
+ ioctl(fd, FS_IOC_SETFLAGS, flags);
+ }
+ close(fd);
+ }
Humm, no. We won't tape over problems. If the defragging thing is a
performance issue we need to figure out why that happens, and as it
stands now it simply seems to be a weakness in the current btrfs kernel
code.

But we will not work around these problems from userspace just like
that.

I mean, I am fine with giving btrfs special support and stuff, but I
want input from the right kernel guys about about this. If they
recommend this, then sure, we can do something like this, but we don't
blindly tape over this.

Sorry,

Lennart
--
Lennart Poettering, Red Hat
Werner Fink
2014-06-13 14:41:10 UTC
Permalink
---
shell-completion/bash/hostnamectl | 6 +++++-
shell-completion/bash/journalctl | 6 +++++-
shell-completion/bash/kernel-install | 13 ++++++++++++-
shell-completion/bash/localectl | 6 +++++-
shell-completion/bash/loginctl | 6 +++++-
shell-completion/bash/systemctl | 6 +++++-
shell-completion/bash/systemd-analyze | 6 +++++-
shell-completion/bash/systemd-coredumpctl | 6 +++++-
shell-completion/bash/systemd-run | 14 +++++++++++++-
shell-completion/bash/timedatectl | 6 +++++-
shell-completion/bash/udevadm | 6 +++++-
11 files changed, 70 insertions(+), 11 deletions(-)

diff --git shell-completion/bash/hostnamectl shell-completion/bash/hostnamectl
index 9c75da9..2e947f6 100644
--- shell-completion/bash/hostnamectl
+++ shell-completion/bash/hostnamectl
@@ -30,6 +30,10 @@ _hostnamectl() {
local OPTS='-h --help --version --transient --static --pretty
--no-ask-password -H --host'

+ if __contains_word ">" ${COMP_WORDS[*]:0:COMP_CWORD}; then
+ return 0
+ fi
+
if [[ $cur = -* ]]; then
COMPREPLY=( $(compgen -W '${OPTS[*]}' -- "$cur") )
return 0
@@ -58,4 +62,4 @@ _hostnamectl() {
return 0
}

-complete -F _hostnamectl hostnamectl
+complete -o default -o bashdefault -F _hostnamectl hostnamectl
diff --git shell-completion/bash/journalctl shell-completion/bash/journalctl
index e4b2f4a..50f83e0 100644
--- shell-completion/bash/journalctl
+++ shell-completion/bash/journalctl
@@ -49,6 +49,10 @@ _journalctl() {
--verify-key'
)

+ if __contains_word ">" ${COMP_WORDS[*]:0:COMP_CWORD}; then
+ return 0
+ fi
+
if __contains_word "$prev" ${OPTS[ARG]} ${OPTS[ARGUNKNOWN]}; then
case $prev in
--boot|--this-boot|-b)
@@ -111,4 +115,4 @@ _journalctl() {
fi
}

-complete -F _journalctl journalctl
+complete -o default -o bashdefault -F _journalctl journalctl
diff --git shell-completion/bash/kernel-install shell-completion/bash/kernel-install
index 7cd2494..33cf27c 100644
--- shell-completion/bash/kernel-install
+++ shell-completion/bash/kernel-install
@@ -18,11 +18,22 @@
# You should have received a copy of the GNU Lesser General Public License
# along with systemd; If not, see <http://www.gnu.org/licenses/>.

+__contains_word () {
+ local w word=$1; shift
+ for w in "$@"; do
+ [[ $w = "$word" ]] && return
+ done
+}
+
_kernel_install() {
local comps
local MACHINE_ID
local cur=${COMP_WORDS[COMP_CWORD]}

+ if __contains_word ">" ${COMP_WORDS[*]:0:COMP_CWORD}; then
+ return 0
+ fi
+
case $COMP_CWORD in
1)
comps="add remove"
@@ -47,4 +58,4 @@ _kernel_install() {
return 0
}

-complete -F _kernel_install kernel-install
+complete -o default -o bashdefault -F _kernel_install kernel-install
diff --git shell-completion/bash/localectl shell-completion/bash/localectl
index c9e22af..3150f87 100644
--- shell-completion/bash/localectl
+++ shell-completion/bash/localectl
@@ -36,6 +36,10 @@ _localectl() {
local OPTS='-h --help --version --no-convert --no-pager --no-ask-password
-H --host'

+ if __contains_word ">" ${COMP_WORDS[*]:0:COMP_CWORD}; then
+ return 0
+ fi
+
if __contains_word "$prev" $OPTS; then
case $prev in
--host|-H)
@@ -89,4 +93,4 @@ _localectl() {
return 0
}

-complete -F _localectl localectl
+complete -o default -o bashdefault -F _localectl localectl
diff --git shell-completion/bash/loginctl shell-completion/bash/loginctl
index e7adb93..9b137b4 100644
--- shell-completion/bash/loginctl
+++ shell-completion/bash/loginctl
@@ -37,6 +37,10 @@ _loginctl () {
[ARG]='--host -H --kill-who --property -p --signal -s'
)

+ if __contains_word ">" ${COMP_WORDS[*]:0:COMP_CWORD}; then
+ return 0
+ fi
+
if __contains_word "$prev" ${OPTS[ARG]}; then
case $prev in
--signal|-s)
@@ -106,4 +110,4 @@ _loginctl () {
return 0
}

-complete -F _loginctl loginctl
+complete -o default -o bashdefault -F _loginctl loginctl
diff --git shell-completion/bash/systemctl shell-completion/bash/systemctl
index e1c8420..fa54e70 100644
--- shell-completion/bash/systemctl
+++ shell-completion/bash/systemctl
@@ -77,6 +77,10 @@ _systemctl () {
[ARG]='--host -H --kill-who --property -p --signal -s --type -t --state --root'
)

+ if __contains_word ">" ${COMP_WORDS[*]:0:COMP_CWORD}; then
+ return 0
+ fi
+
if __contains_word "--user" ${COMP_WORDS[*]}; then
mode=--user
else
@@ -223,4 +227,4 @@ _systemctl () {
return 0
}

-complete -F _systemctl systemctl
+complete -o default -o bashdefault -F _systemctl systemctl
diff --git shell-completion/bash/systemd-analyze shell-completion/bash/systemd-analyze
index 5575beb..2d195aa 100644
--- shell-completion/bash/systemd-analyze
+++ shell-completion/bash/systemd-analyze
@@ -46,6 +46,10 @@ _systemd_analyze() {
[LOG_LEVEL]='set-log-level'
)

+ if __contains_word ">" ${COMP_WORDS[*]:0:COMP_CWORD}; then
+ return 0
+ fi
+
_init_completion || return

for ((i=0; i < COMP_CWORD; i++)); do
@@ -105,4 +109,4 @@ _systemd_analyze() {
return 0
}

-complete -F _systemd_analyze systemd-analyze
+complete -o default -o bashdefault -F _systemd_analyze systemd-analyze
diff --git shell-completion/bash/systemd-coredumpctl shell-completion/bash/systemd-coredumpctl
index 805e848..8f29651 100644
--- shell-completion/bash/systemd-coredumpctl
+++ shell-completion/bash/systemd-coredumpctl
@@ -44,6 +44,10 @@ _coredumpctl() {
[DUMP]='dump gdb'
)

+ if __contains_word ">" ${COMP_WORDS[*]:0:COMP_CWORD}; then
+ return 0
+ fi
+
if __contains_word "$prev" '--output -o'; then
comps=$( compgen -A file -- "$cur" )
compopt -o filenames
@@ -82,4 +86,4 @@ _coredumpctl() {
return 0
}

-complete -F _coredumpctl systemd-coredumpctl
+complete -o default -o bashdefault -F _coredumpctl systemd-coredumpctl
diff --git shell-completion/bash/systemd-run shell-completion/bash/systemd-run
index 712655c..e547e95 100644
--- shell-completion/bash/systemd-run
+++ shell-completion/bash/systemd-run
@@ -17,6 +17,13 @@
# You should have received a copy of the GNU Lesser General Public License
# along with systemd; If not, see <http://www.gnu.org/licenses/>.

+__contains_word () {
+ local w word=$1; shift
+ for w in "$@"; do
+ [[ $w = "$word" ]] && return
+ done
+}
+
__systemctl() {
local mode=$1; shift 1
systemctl $mode --full --no-legend "$@"
@@ -38,6 +45,11 @@ _systemd_run() {

local mode=--system
local i
+
+ if __contains_word ">" ${COMP_WORDS[*]:0:COMP_CWORD}; then
+ return 0
+ fi
+
for (( i=1; i <= COMP_CWORD; i++ )); do
if [[ ${COMP_WORDS[i]} != -* ]]; then
local root_command=${COMP_WORDS[i]}
@@ -98,4 +110,4 @@ _systemd_run() {
return 0
}

-complete -F _systemd_run systemd-run
+complete -o default -o bashdefault -F _systemd_run systemd-run
diff --git shell-completion/bash/timedatectl shell-completion/bash/timedatectl
index 1a0acc6..d0ca51d 100644
--- shell-completion/bash/timedatectl
+++ shell-completion/bash/timedatectl
@@ -30,6 +30,10 @@ _timedatectl() {
local OPTS='-h --help --version --adjust-system-clock --no-pager
--no-ask-password -H --host'

+ if __contains_word ">" ${COMP_WORDS[*]:0:COMP_CWORD}; then
+ return 0
+ fi
+
if __contains_word "$prev" $OPTS; then
case $prev in
--host|-H)
@@ -73,4 +77,4 @@ _timedatectl() {
return 0
}

-complete -F _timedatectl timedatectl
+complete -o default -o bashdefault -F _timedatectl timedatectl
diff --git shell-completion/bash/udevadm shell-completion/bash/udevadm
index b828b8d..e7cf29f 100644
--- shell-completion/bash/udevadm
+++ shell-completion/bash/udevadm
@@ -36,6 +36,10 @@ _udevadm() {

local verbs=(info trigger settle control monitor hwdb test-builtin test)

+ if __contains_word ">" ${COMP_WORDS[*]:0:COMP_CWORD}; then
+ return 0
+ fi
+
for ((i=0; i < COMP_CWORD; i++)); do
if __contains_word "${COMP_WORDS[i]}" "${verbs[@]}" &&
! __contains_word "${COMP_WORDS[i-1]}" ${OPTS[ARG]}; then
@@ -94,4 +98,4 @@ _udevadm() {
return 0
}

-complete -F _udevadm udevadm
+complete -o default -o bashdefault -F _udevadm udevadm
--
1.7.9.2
Ran Benita
2014-06-13 16:09:50 UTC
Permalink
Dr. Werner Fink
2014-06-16 12:59:34 UTC
Permalink
On Fri, Jun 13, 2014 at 07:09:50PM +0300, Ran Benita wrote:
Zbigniew Jędrzejewski-Szmek
2014-06-20 03:28:32 UTC
Permalink
Tom Gundersen
2014-06-13 15:51:34 UTC
Permalink
Hm, this would not help at all for modules loaded on-demand (which are
most of them). What is the problem being solved here?
---
units/systemd-sysctl.service.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git units/systemd-sysctl.service.in units/systemd-sysctl.service.in
index 5baf22c..c4bc0da 100644
--- units/systemd-sysctl.service.in
+++ units/systemd-sysctl.service.in
@@ -10,7 +10,7 @@ Description=Apply Kernel Variables
Documentation=man:systemd-sysctl.service(8) man:sysctl.d(5)
DefaultDependencies=no
Conflicts=shutdown.target
-After=systemd-readahead-collect.service systemd-readahead-replay.service
+After=systemd-readahead-collect.service systemd-readahead-replay.service systemd-modules-load.service
Before=sysinit.target shutdown.target
ConditionPathIsReadWrite=/proc/sys/
ConditionDirectoryNotEmpty=|/lib/sysctl.d
--
1.7.9.2
_______________________________________________
systemd-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Dr. Werner Fink
2014-06-16 12:52:30 UTC
Permalink
Post by Tom Gundersen
Hm, this would not help at all for modules loaded on-demand (which are
most of them). What is the problem being solved here?
Indeed this does not help for the loaded on-demand modules as this requires
an other approach. Nevertheless for the modules loaded by the service
systemd-modules-load.service it helps to load the the kernel parameters
afterwards as with many modules there will be *new* entiers below /proc/sys/
Post by Tom Gundersen
---
units/systemd-sysctl.service.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git units/systemd-sysctl.service.in units/systemd-sysctl.service.in
index 5baf22c..c4bc0da 100644
--- units/systemd-sysctl.service.in
+++ units/systemd-sysctl.service.in
@@ -10,7 +10,7 @@ Description=Apply Kernel Variables
Documentation=man:systemd-sysctl.service(8) man:sysctl.d(5)
DefaultDependencies=no
Conflicts=shutdown.target
-After=systemd-readahead-collect.service systemd-readahead-replay.service
+After=systemd-readahead-collect.service systemd-readahead-replay.service systemd-modules-load.service
Before=sysinit.target shutdown.target
ConditionPathIsReadWrite=/proc/sys/
ConditionDirectoryNotEmpty=|/lib/sysctl.d
--
1.7.9.2
Werner
--
"Having a smoking section in a restaurant is like having
a peeing section in a swimming pool." -- Edward Burr
Cristian Rodríguez
2014-06-16 14:49:00 UTC
Permalink
Post by Dr. Werner Fink
Post by Tom Gundersen
Hm, this would not help at all for modules loaded on-demand (which are
most of them). What is the problem being solved here?
Indeed this does not help for the loaded on-demand modules as this requires
an other approach. Nevertheless for the modules loaded by the service
systemd-modules-load.service it helps to load the the kernel parameters
afterwards as with many modules there will be *new* entiers below /proc/sys/
What exactly is the problem you are trying to solve with this ? It still
looks wrong .. The only way that could work is using an udev rule or an
/etc/modprobe.d/ snippet. AFAIK any other approach is doomed in one way
or another.
--
Cristian
"I don't know the key to success, but the key to failure is trying to
please everybody."
Frederic Crozat
2014-06-16 15:19:54 UTC
Permalink
Post by Cristian Rodríguez
Post by Dr. Werner Fink
Post by Tom Gundersen
Hm, this would not help at all for modules loaded on-demand (which are
most of them). What is the problem being solved here?
Indeed this does not help for the loaded on-demand modules as this requires
an other approach. Nevertheless for the modules loaded by the service
systemd-modules-load.service it helps to load the the kernel parameters
afterwards as with many modules there will be *new* entiers below /proc/sys/
What exactly is the problem you are trying to solve with this ? It still
looks wrong .. The only way that could work is using an udev rule or an
/etc/modprobe.d/ snippet. AFAIK any other approach is doomed in one way
or another.
See https://bugzilla.novell.com/show_bug.cgi?id=725412
--
Frederic Crozat
Project Manager Enterprise Desktop
SUSE
Tom Gundersen
2014-06-16 16:02:37 UTC
Permalink
Post by Frederic Crozat
Post by Cristian Rodríguez
Post by Dr. Werner Fink
Post by Tom Gundersen
Hm, this would not help at all for modules loaded on-demand (which are
most of them). What is the problem being solved here?
Indeed this does not help for the loaded on-demand modules as this requires
an other approach. Nevertheless for the modules loaded by the service
systemd-modules-load.service it helps to load the the kernel parameters
afterwards as with many modules there will be *new* entiers below /proc/sys/
What exactly is the problem you are trying to solve with this ? It still
looks wrong .. The only way that could work is using an udev rule or an
/etc/modprobe.d/ snippet. AFAIK any other approach is doomed in one way
or another.
See https://bugzilla.novell.com/show_bug.cgi?id=725412
Hm, that really does not look convincing. There is a fundamental
problem here (as Ludwig Nessel points out in the linked discussion:
"sysctl is broken by design unfortunately."), and the discussion nor
the patch do not get to the bottom of that.

Moreover, (essentially) this patch was already posted and rejected
last year. Lennart then wrote:

"Well, most modules are loaded asynchronously from udev, so I fear this
won't do much...

/etc/sysctl.d/ is really only for sysctl settings that exist all the
time, and -- as a special exception -- for network-device related
settings, which we set via a udev rule.

If people want to apply sysctls based on specific modules that are
loaded, or based on specific hw that shows up (i.e. hw that isn't a
network device) the only sane way is probably via a udev rule..."

Cheers,

Tom
Dr. Werner Fink
2014-06-18 09:54:49 UTC
Permalink
Post by Tom Gundersen
Post by Frederic Crozat
See https://bugzilla.novell.com/show_bug.cgi?id=725412
Hm, that really does not look convincing. There is a fundamental
"sysctl is broken by design unfortunately."), and the discussion nor
the patch do not get to the bottom of that.
Moreover, (essentially) this patch was already posted and rejected
"Well, most modules are loaded asynchronously from udev, so I fear this
won't do much...
/etc/sysctl.d/ is really only for sysctl settings that exist all the
time, and -- as a special exception -- for network-device related
settings, which we set via a udev rule.
If people want to apply sysctls based on specific modules that are
loaded, or based on specific hw that shows up (i.e. hw that isn't a
network device) the only sane way is probably via a udev rule..."
The only problem with udev rules is that many system administrators
do not know enough how to write and how to place such rules. What
about simple dependencies for such sysctl settings for specific modules.
Maybe by using a special name spavce below sysctl.d directories or a
special comment within the configure files below modules-load.d
directories. Or similar like the unit configuration of services.

Werner
--
"Having a smoking section in a restaurant is like having
a peeing section in a swimming pool." -- Edward Burr
Zbigniew Jędrzejewski-Szmek
2014-06-20 01:15:31 UTC
Permalink
Post by Dr. Werner Fink
Post by Tom Gundersen
Post by Frederic Crozat
See https://bugzilla.novell.com/show_bug.cgi?id=725412
Hm, that really does not look convincing. There is a fundamental
"sysctl is broken by design unfortunately."), and the discussion nor
the patch do not get to the bottom of that.
Moreover, (essentially) this patch was already posted and rejected
"Well, most modules are loaded asynchronously from udev, so I fear this
won't do much...
/etc/sysctl.d/ is really only for sysctl settings that exist all the
time, and -- as a special exception -- for network-device related
settings, which we set via a udev rule.
If people want to apply sysctls based on specific modules that are
loaded, or based on specific hw that shows up (i.e. hw that isn't a
network device) the only sane way is probably via a udev rule..."
The only problem with udev rules is that many system administrators
do not know enough how to write and how to place such rules. What
about simple dependencies for such sysctl settings for specific modules.
Maybe by using a special name spavce below sysctl.d directories or a
special comment within the configure files below modules-load.d
directories. Or similar like the unit configuration of services.
I pushed the patch from Cristian Rodríguez, esentially identical to
this. I also added a small example to sysctl.d(5), loading bridge
module statically and setting some settings... and another one using
udev rules. At least the situation is documented now.

Zbyszek
Zbigniew Jędrzejewski-Szmek
2014-06-20 02:15:15 UTC
Permalink
Post by Werner Fink
Otherwise the add_symlink() function tries to make directories for each
I now pushed a simpler patch which should fix the issue. Please
test.

Zbyszek
Loading...