[systemd-devel] [PATCH] vconsole-setup: check error of child process

[lucas.de.marchi at gmail.com]

From: Lucas De Marchi <lucas.demarchi at intel.com>

If we don't check the error of the child process, systemd-vconsole-setup
would exit with 0 even if it could not really setup the console.

For a simple test, move loadkeys elsewhere and execute
systemd-vconsole-setup:

	[root at localhost ~]# strace -f -e execve /usr/lib/systemd/systemd-vconsole-setup
	execve("/usr/lib/systemd/systemd-vconsole-setup", ["/usr/lib/systemd/systemd-vconsol"...], [/* 15 vars */]) = 0
	Process 171 attached
	[pid   171] execve("/usr/bin/loadkeys", ["/usr/bin/loadkeys", "-q", "-C", "/dev/tty0", "br-abnt2"], [/* 15 vars */]) = -1 ENOENT (No such file or directory)
	[pid   171] +++ exited with 1 +++
	--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=171, si_uid=0, si_status=1, si_utime=0, si_stime=0} ---
	+++ exited with 0 +++

Note that loadkeys returned 1 while systemd-vconsole-setup return 0.

Since the font and keyboard setup are already serialized, refactor the
code a little bit so the functions do the wait by themselves. One change
in behavior in this patch is that we don't return early, but we do try
to setup the keyboard even if the font load failed.
---
 src/vconsole/vconsole-setup.c | 68 +++++++++++++++----------------------------
 1 file changed, 24 insertions(+), 44 deletions(-)

diff --git a/src/vconsole/vconsole-setup.c b/src/vconsole/vconsole-setup.c
index bf681d9..fe8573f 100644
--- a/src/vconsole/vconsole-setup.c
+++ b/src/vconsole/vconsole-setup.c
@@ -96,16 +96,14 @@ static int enable_utf8(int fd) {
         return r;
 }
 
-static int keymap_load(const char *vc, const char *map, const char *map_toggle, bool utf8, pid_t *_pid) {
+static bool keyboard_load_and_wait(const char *vc, const char *map, const char *map_toggle, bool utf8) {
         const char *args[8];
         int i = 0;
         pid_t pid;
 
-        if (isempty(map)) {
-                /* An empty map means kernel map */
-                *_pid = 0;
-                return 0;
-        }
+        /* An empty map means kernel map */
+        if (isempty(map))
+                return true;
 
         args[i++] = KBD_LOADKEYS;
         args[i++] = "-q";
@@ -119,27 +117,25 @@ static int keymap_load(const char *vc, const char *map, const char *map_toggle,
         args[i++] = NULL;
 
         pid = fork();
-        if (pid < 0)
-                return log_error_errno(errno, "Failed to fork: %m");
-        else if (pid == 0) {
+        if (pid < 0) {
+                log_error_errno(errno, "Failed to fork: %m");
+                return false;
+        } else if (pid == 0) {
                 execv(args[0], (char **) args);
                 _exit(EXIT_FAILURE);
         }
 
-        *_pid = pid;
-        return 0;
+        return wait_for_terminate_and_warn(KBD_LOADKEYS, pid, true) == 0;
 }
 
-static int font_load(const char *vc, const char *font, const char *map, const char *unimap, pid_t *_pid) {
+static bool font_load_and_wait(const char *vc, const char *font, const char *map, const char *unimap) {
         const char *args[9];
         int i = 0;
         pid_t pid;
 
-        if (isempty(font)) {
-                /* An empty font means kernel font */
-                *_pid = 0;
-                return 0;
-        }
+        /* An empty font means kernel font */
+        if (isempty(font))
+                return true;
 
         args[i++] = KBD_SETFONT;
         args[i++] = "-C";
@@ -156,15 +152,15 @@ static int font_load(const char *vc, const char *font, const char *map, const ch
         args[i++] = NULL;
 
         pid = fork();
-        if (pid < 0)
-                return log_error_errno(errno, "Failed to fork: %m");
-        else if (pid == 0) {
+        if (pid < 0) {
+                log_error_errno(errno, "Failed to fork: %m");
+                return false;
+        } else if (pid == 0) {
                 execv(args[0], (char **) args);
                 _exit(EXIT_FAILURE);
         }
 
-        *_pid = pid;
-        return 0;
+        return wait_for_terminate_and_warn(KBD_SETFONT, pid, true) == 0;
 }
 
 /*
@@ -238,8 +234,7 @@ int main(int argc, char **argv) {
                 *vc_font = NULL, *vc_font_map = NULL, *vc_font_unimap = NULL;
         _cleanup_close_ int fd = -1;
         bool utf8;
-        pid_t font_pid = 0, keymap_pid = 0;
-        bool font_copy = false;
+        bool font_copy = false, font_ok, keyboard_ok;
         int r = EXIT_FAILURE;
 
         log_set_target(LOG_TARGET_AUTO);
@@ -298,27 +293,12 @@ int main(int argc, char **argv) {
         else
                 disable_utf8(fd);
 
-        r = font_load(vc, vc_font, vc_font_map, vc_font_unimap, &font_pid);
-        if (r < 0) {
-                log_error_errno(r, "Failed to start " KBD_SETFONT ": %m");
-                return EXIT_FAILURE;
-        }
-
-        if (font_pid > 0)
-                wait_for_terminate_and_warn(KBD_SETFONT, font_pid, true);
-
-        r = keymap_load(vc, vc_keymap, vc_keymap_toggle, utf8, &keymap_pid);
-        if (r < 0) {
-                log_error_errno(r, "Failed to start " KBD_LOADKEYS ": %m");
-                return EXIT_FAILURE;
-        }
-
-        if (keymap_pid > 0)
-                wait_for_terminate_and_warn(KBD_LOADKEYS, keymap_pid, true);
+        font_ok = font_load_and_wait(vc, vc_font, vc_font_map, vc_font_unimap);
+        keyboard_ok = keyboard_load_and_wait(vc, vc_keymap, vc_keymap_toggle, utf8);
 
-        /* Only copy the font when we started setfont successfully */
-        if (font_copy && font_pid > 0)
+        /* Only copy the font when we executed setfont successfully */
+        if (font_copy && font_ok)
                 font_copy_to_all_vcs(fd);
 
-        return EXIT_SUCCESS;
+        return font_ok && keyboard_ok ? EXIT_SUCCESS : EXIT_FAILURE;
 }
-- 
2.3.2