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

[Didier Roche]

Le 28/01/2015 15:37, Zbigniew Jędrzejewski-Szmek a écrit :
> On Wed, Jan 28, 2015 at 02:20:40PM +0100, Didier Roche wrote:
Hey Zbigniew,

Thanks for the quick feedbacks! Integrated your changes in the incoming 
patches. Just answered to some comments here:

>>  From d0f49b4e0994b9a1dd2738da18c5a2a6708b444f Mon Sep 17 00:00:00 2001
>> From: Didier Roche <didrocks at ubuntu.com>
>> Date: Mon, 26 Jan 2015 15:35:50 +0100
>> Subject: [PATCH 01/12] fsckd daemon for inter-fsckd communication
>>
>>
> +        fsckd_fd = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
> +        if (fsckd_fd < 0) {
> +                log_warning_errno(errno, "Cannot open fsckd socket, we won't report fsck progress: %m");
> +                return -errno;
> +        }
> +        if (connect(fsckd_fd, &sa.sa, offsetof(struct sockaddr_un, sun_path) + strlen(sa.un.sun_path)) < 0) {
> +                log_warning_errno(errno, "Cannot connect to fsckd socket, we won't report fsck progress: %m");
> +                return -errno;
> +        }
>   
>           f = fdopen(fd, "r");
>           if (!f) {
> -                safe_close(fd);
> +                log_warning_errno(errno, "Cannot connect to fsck, we won't report fsck progress: %m");
>                   return -errno;
>           }
> There's a double close now, since both f and fsckd_fd refer to the same fd.

Actually no, fd is the fd from the pipe between fsck -> systemd-fsck
and fsckd_fd is the fd from the socket between systemd-fsck -> 
systemd-fsckd.

Or am I missing something?

>
> \ No newline at end of file
> diff --git a/src/fsckd/fsckd.c b/src/fsckd/fsckd.c
> new file mode 100644
> index 0000000..3059c68
> --- /dev/null
> +++ b/src/fsckd/fsckd.c
> @@ -0,0 +1,295 @@
> +/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
> +
>> +}
>> +
>> +static int handle_requests(int socket_fd) {
>> +        Clients *first = NULL;
>> +        usec_t last_activity = 0;
>> +        int numdevices = 0, clear = 0;
>> +        double percent = 100;
>> +        _cleanup_fclose_ FILE *console = NULL;
> NULL not necessary (and you don't have it in similar circumstances above ;))
So, after the discussion on the mailing list and to be more future 
proof, I went to recheck, but I'm afraid that I didn't find any place 
where I have some _cleanup_* without initializing to NULL? Did I get it 
wrong?

>
>> +                                log_error_errno(errno, "Socket read error, disconnecting fd %d: %m", current->fd);
>> +                                current->fd = 0;
> 0 is a valid file descriptor number. You must use -1. This also means that some initialization
> to -1 is missing.
>

The clients are always initialized when we get a valid fd (current->fd = 
new_client_fd;), so no need to initiliaze to -1, I'm using now -1 though 
to invalidate the fd.

>> +static int create_socket(void) {
> Can you base this on make_socket_fd() instead?

Oh nice, didn't find it when I looked for a generic utility. Using it :)


Thanks again for this review!
Cheers,
Didier