On Wed, 28.01.15 14:20, Didier Roche (didier.ro...@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
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
