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

[Zbigniew Jędrzejewski-Szmek]

On Thu, Jan 29, 2015 at 06:37:53PM +0100, Didier Roche wrote:
> 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.
You're right. Seems to be fine.

> 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?
fsckd_fd variable.

> >
> >>+                                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.
OK.

> >>+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 :)

Zbyszek