[systemd-devel] [PATCH 01/12] fsckd daemon for inter-fsckd communication

[Didier Roche]

Le 28/01/2015 21:16, Lennart Poettering a écrit :

Thanks for your feedbacks! Changes integrated, some remaining questions 
though:

> On Wed, 28.01.15 14:20, Didier Roche (didier.roche at canonical.com) wrote:
>
>> +        last_activity = now(CLOCK_MONOTONIC);
>> +
>> +        for (;;) {
>> +                int new_client_fd = 0;
>> +                Clients *current;
>> +                _cleanup_free_ char *console_message = NULL;
>> +                double current_percent = 100;
>> +                int current_numdevices = 0, m = 0;
>> +                usec_t t;
>> +
>> +                /* Initialize and list new clients */
>> +                new_client_fd = accept4(socket_fd, NULL, NULL, SOCK_NONBLOCK);
>> +                if (new_client_fd > 0) {
>> +                        log_debug("new fsck client connected to fd: %d", new_client_fd);
>> +                        current = alloca0(sizeof(Clients));
> It's not OK to invoke alloca() in loops. This cannot work. Please use
> normal heap memoy for this.

Oh, good to know, replaced with a regular malloc. I didn't handle the 
freeing of Clients as they are destructed on program close, should I 
handle this (and thus looping over and freeing the new char* - which 
part of the struct)?
> +                        current = NULL;
> +                }
> +
> +                LIST_FOREACH(clients, current, first) {
> +                       FsckProgress fsck_data;
> +                       int rc = 0;
> +
> +                        if (current->fd > 0)
> +                                rc = recv(current->fd, &fsck_data, sizeof(FsckProgress), 0);
> +
> +                        if ((current->fd != 0) && (rc == 0)) {
> +                                log_debug("fsck client connected to
> fd %d disconnected", current->fd);
> Please print propery exit codes.

That was my bad, it's r rather then rc, so not a return code but number 
of bytes read, or I missing something?
>> +
>> +                        /* write to console */
>> +                        fprintf(console, "\r%s\r%n", console_message, &m);
>> +                        fflush(console);
> Hmm, what's the reason for first writing this to an alocated buffer,
> and then to the console? You can write this directly to the console, no?
>
>
Once I'm adding plymouth connection in 03/12, I'm reusing the same 
console_message (I can rename it if necessary).

Thanks for the review!
Cheers,
Didier