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

[Didier Roche]

Le 10/03/2015 11:41, Lennart Poettering a écrit :
> 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.

Thanks for the explanations, I dropped that patch. Note that I needed to 
do a slight change to the paradigm in the replacement of patch 04_*

Didier