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

[David Herrmann]

Hi

On Wed, Mar 11, 2015 at 5:12 PM, Lucas De Marchi
<lucas.de.marchi at gmail.com> wrote:
> On Wed, Mar 11, 2015 at 12:55 PM, David Herrmann <dh.herrmann at gmail.com> wrote:
>> Hi
>>
>> On Tue, Mar 10, 2015 at 8:56 PM,  <lucas.de.marchi at gmail.com> wrote:
>>> 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 +++
>>>
>>> Now systemd-vconsole-setup exits with EXIT_FAILURE if the child failed
>>> for whatever reason.
>>> ---
>>>  src/vconsole/vconsole-setup.c | 17 ++++++++++++-----
>>>  1 file changed, 12 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/vconsole/vconsole-setup.c b/src/vconsole/vconsole-setup.c
>>> index bf681d9..601ca4a 100644
>>> --- a/src/vconsole/vconsole-setup.c
>>> +++ b/src/vconsole/vconsole-setup.c
>>> @@ -304,8 +304,12 @@ int main(int argc, char **argv) {
>>>                  return EXIT_FAILURE;
>>>          }
>>>
>>> -        if (font_pid > 0)
>>> -                wait_for_terminate_and_warn(KBD_SETFONT, font_pid, true);
>>> +        if (font_pid > 0) {
>>> +                r = wait_for_terminate_and_warn(KBD_SETFONT, font_pid, true);
>>> +                if (r != 0)
>>> +                        return EXIT_FAILURE;
>>> +        }
>>> +
>>
>> Looks mostly good. However, I'd prefer if we continue on "r > 0" but
>> remember the error for later. This way, we still try loading the right
>> keymap even though the font might have failed.
>
> Is there any reason to continue on r > 0 but not on r < 0? I'm ok with
> changing the current behavior, but just like to know which one is
> better.

Yeah, not sure here. For most helpers here r<0 means serious error
(fork() failed, etc.), r>0 means just the spawned application failed.
I guess we should continue for all !=0. That is, don't return
prematurely at all.

Thanks
David