[systemd-devel] [PATCH 2/5] fsckd: Fix some error return values

[Lennart Poettering]

On Tue, 10.03.15 11:33, Didier Roche (didrocks at ubuntu.com) wrote:

> diff --git a/src/fsckd/fsckd.c b/src/fsckd/fsckd.c
> index 3fc923b..9393379 100644
> --- a/src/fsckd/fsckd.c
> +++ b/src/fsckd/fsckd.c
> @@ -220,7 +220,7 @@ static int manager_connect_plymouth(Manager *m) {
>                  goto fail;
>          }
>  
> -        return 1;
> +        return 0;

Oh, well, this doesn't matter too much I guess, since we don't check
for the precise value, but just < 0 or >= 0...

Returning 1 in this case is actually a bit of a pattern I am trying to
adopt across the codebase: return 0 if all is OK but we didn't do
anything. Return > 1 if all is OK and we actually did something. In
this case this distinction is of course entirely irrelevant, but it
was more of an automatism...

>  
>  fail:
>          manager_disconnect_plymouth(m);
> @@ -406,10 +406,8 @@ static int manager_new_connection_handler(sd_event_source *s, int fd, uint32_t r
>          log_debug("New fsck client connected to fd: %d", new_client_fd);
>  
>          c = new0(Client, 1);
> -        if (!c) {
> -                log_oom();
> -                return 0;
> -        }
> +        if (!c)
> +                return log_oom();

Well, I think logging and ignoring the OOM condition in this case is a
good idea. THink about the effect of returning an error up here: when
a handler callback returns an error sd-event will disable the event
source automatically. This means if fsckd hits an OOM once here, it
will become completely unresponsive, as it never processes incoming
connections anymore. However, if we simply log, close the connection,
and continue without propagating an error up, we are slightly more
robust: sure, the connection will fail, but subsequent connections
might not...

>          c->fd = new_client_fd;
>          new_client_fd = -1;
> @@ -417,7 +415,7 @@ static int manager_new_connection_handler(sd_event_source *s, int fd, uint32_t r
>          r = sd_event_add_io(m->event, &c->event_source, c->fd, EPOLLIN, client_progress_handler, c);
>          if (r < 0) {
>                  log_oom();
> -                return 0;
> +                return r;

Same here.

Lennart

-- 
Lennart Poettering, Red Hat