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

[Lennart Poettering]

On Wed, 28.01.15 14:20, Didier Roche (didier.roche at canonical.com) wrote:

>  static int process_progress(int fd) {
> -        _cleanup_fclose_ FILE *console = NULL, *f = NULL;
> +        _cleanup_fclose_ FILE *f = NULL;
>          usec_t last = 0;
> -        bool locked = false;
> -        int clear = 0;
> +        _cleanup_close_ int fsckd_fd;
> +        static const union sockaddr_union sa = {
> +                .un.sun_family = AF_UNIX,
> +                .un.sun_path = FSCKD_SOCKET_PATH,
> +        };
> +
> +        fsckd_fd = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);

Please always specifiy SOCK_CLOEXEC. In fact, *all* our fs should be
opened in CLOEXEC mode these days, except for the very few case where
that's explicitly not desired.

> -                for (j = 0; j < (unsigned) clear; j++)
> -                        fputc(' ', console);
> -                fputc('\r', console);
> -                fflush(console);
> +                n = send(fsckd_fd, &progress, sizeof(FsckProgress), 0);
> +                if (n < 0 || (size_t) n < sizeof(FsckProgress))
> +                        log_warning_errno(n, "Cannot communicate
> -                fsck progress to fsckd: %m");

Hmm, this error check is incorrect. The first argument of
log_warning_errno() should carry an errno integer of some kind, but
the raw glibc send() does not return that, it returns a size...

> +
> +#define IDLE_TIME_MINUTES 1
> +
> +typedef struct Clients {
> +        int fd;
> +        char device[PATH_MAX];

Hmm, no, please don't. Please never use fixed size arrays for these
things unless the string is really fixed size. Please use char* here,
and allocate the data. 

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

> +                        current->fd = new_client_fd;
> +                        if (!first)
> +                                LIST_INIT(clients, current);
> +                        else
> +                                LIST_PREPEND(clients, first, current);
> +                        first = current;


LIST_PREPEND(clients, first, current) already does all of the five
lines above.

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

> +                                current->fd = 0;
> +                                current->percent = 100;
> +                        } else if ((rc < 0) && (errno !=
> EWOULDBLOCK)) {

EWOULDBLOCK is a windows name. Please use EAGAIN.

> +                if ((fabs(current_percent - percent) > 0.000001) || (current_numdevices != numdevices)) {
> +                        numdevices = current_numdevices;
> +                        percent = current_percent;
> +
> +                        asprintf(&console_message, "Checking in
> progress on %d disks (%3.1f%% complete)",
> +                                 numdevices, percent);

Missing oom check.

> +
> +                        /* 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?


> +static int create_socket(void) {
> +        /* Create the socket manually for testing */
> +        union {
> +                struct sockaddr sa;
> +                struct sockaddr_un un;
> +        } sa;

Please use sockaddr_union for this.

> +        int fd;
> +
> +        fd = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
> +        if (fd < 0)
> +                return log_error_errno(errno, "socket() failed:
> %m");

SOCK_CLOEXEC, please.

> +
> +        memset(&sa, 0, sizeof(sa));
> +        sa.un.sun_family = AF_UNIX;

Please initialize fields with constant values when you declare the
variable already.

> +        strncpy(sa.un.sun_path, FSCKD_SOCKET_PATH, sizeof(sa.un.sun_path));
> +        unlink(FSCKD_SOCKET_PATH);
> +        if (bind(fd, &sa.sa, sizeof(sa)) < 0)
> +                return log_error_errno(errno, "binding %s failed:
> %m", FSCKD_SOCKET_PATH);

This not the right way to listen on an AF_UNIX path. You need to
calculate the size of the sockaddr properly, as "offsetof(sa,
un.sun_path) + strlen(sa.un.sun_path)". Otherwise the kernel will
including all those trailing NULs in the socket address! 

Lennart

-- 
Lennart Poettering, Red Hat