Discussion:
[PATCH] Adding fsck integration to plymouth
(too old to reply)
Didier Roche
2015-01-28 13:19:30 UTC
Permalink
Hi,

Here is a suite of patches up to review to add fsckd integration to
plymouth. This work is mostly based on Lennart's suggestion on an email
thread few years ago
(http://lists.freedesktop.org/archives/systemd-devel/2011-April/002063.html)
where the proposal was to add a systemd-fsckd daemon.

The daemon is socket-activated, each systemd-fsck instances binds and
communicate its own fsck data to it. systemd-fsckd then agglomerates the
results and write to /dev/console as well as propagate those values to
plymouth.

The protocole to communicate with the plymouth theme is described in the
man page. I've modified ubuntu plymouth theme (which is a .script) to
display the progress. There is as well a cancel option in case some
people run on a rotational disks, hit the mount limit and need to give a
talk right away (not my use case, but have seen that in practice
recently) :)

The plymouth integration is optional, otherwise, only /dev/console (if
show-status is enabled) is written.

I'm opened to any question and suggested enhancements.
Cheers,
Didier
Didier Roche
2015-01-28 13:20:40 UTC
Permalink
Zbigniew Jędrzejewski-Szmek
2015-01-28 14:37:46 UTC
Permalink
From d0f49b4e0994b9a1dd2738da18c5a2a6708b444f Mon Sep 17 00:00:00 2001
Date: Mon, 26 Jan 2015 15:35:50 +0100
Subject: [PATCH 01/12] fsckd daemon for inter-fsckd communication
Add systemd-fsckd multiplexer which accept multiple systemd-fsck
accepts
instances to connect to it and send progress report. systemd-fsckd then
sends
computes and writes to /dev/console the number of devices currently being
checked and the minimum fsck progress. This will be used for interactive
progress report and cancelling in plymouth.
systemd-fsckd stops on idling when no systemd-fsck is connected.
idle
Make the necessary changes to systemd-fsck to connect to systemd-fsckd socket.
to connect to the
---
.gitignore | 1 +
Makefile.am | 11 ++
src/fsck/fsck.c | 82 ++++++---------
src/fsckd/Makefile | 1 +
src/fsckd/fsckd.c | 295 +++++++++++++++++++++++++++++++++++++++++++++++++++++
src/fsckd/fsckd.h | 34 ++++++
6 files changed, 373 insertions(+), 51 deletions(-)
create mode 120000 src/fsckd/Makefile
create mode 100644 src/fsckd/fsckd.c
create mode 100644 src/fsckd/fsckd.h
diff --git a/.gitignore b/.gitignore
index ab6d9d1..9400e75 100644
--- a/.gitignore
+++ b/.gitignore
@@ -74,6 +74,7 @@
/systemd-evcat
/systemd-firstboot
/systemd-fsck
+/systemd-fsckd
/systemd-fstab-generator
/systemd-getty-generator
/systemd-gnome-ask-password-agent
diff --git a/Makefile.am b/Makefile.am
index c463f23..f782e66 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -389,6 +389,7 @@ rootlibexec_PROGRAMS = \
systemd-remount-fs \
systemd-reply-password \
systemd-fsck \
+ systemd-fsckd \
systemd-machine-id-commit \
systemd-ac-power \
systemd-sysctl \
@@ -2355,6 +2356,16 @@ systemd_fsck_LDADD = \
libsystemd-shared.la
# ------------------------------------------------------------------------------
+systemd_fsckd_SOURCES = \
+ src/fsckd/fsckd.c \
+ $(NULL)
+
+systemd_fsckd_LDADD = \
+ libsystemd-internal.la \
+ libsystemd-shared.la \
+ $(NULL)
+
+# ------------------------------------------------------------------------------
systemd_machine_id_commit_SOURCES = \
src/machine-id-commit/machine-id-commit.c \
src/core/machine-id-setup.c \
diff --git a/src/fsck/fsck.c b/src/fsck/fsck.c
index 20b7940..4c4e150 100644
--- a/src/fsck/fsck.c
+++ b/src/fsck/fsck.c
@@ -26,6 +26,7 @@
#include <errno.h>
#include <unistd.h>
#include <fcntl.h>
+#include <linux/limits.h>
#include <sys/file.h>
#include "sd-bus.h"
@@ -39,6 +40,8 @@
#include "fileio.h"
#include "udev-util.h"
#include "path-util.h"
+#include "socket-util.h"
+#include "fsckd/fsckd.h"
static bool arg_skip = false;
static bool arg_force = false;
@@ -132,58 +135,42 @@ static void test_files(void) {
arg_show_progress = true;
}
-static double percent(int pass, unsigned long cur, unsigned long max) {
- /* Values stolen from e2fsck */
-
- static const int pass_table[] = {
- 0, 70, 90, 92, 95, 100
- };
-
- if (pass <= 0)
- return 0.0;
-
- if ((unsigned) pass >= ELEMENTSOF(pass_table) || max == 0)
- return 100.0;
-
- return (double) pass_table[pass-1] +
- ((double) pass_table[pass] - (double) pass_table[pass-1]) *
- (double) cur / (double) max;
-}
-
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);
+ 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.
- console = fopen("/dev/console", "we");
- if (!console)
- return -ENOMEM;
-
while (!feof(f)) {
- int pass, m;
+ int pass;
unsigned long cur, max;
_cleanup_free_ char *device = NULL;
- double p;
+ ssize_t n;
usec_t t;
+ FsckProgress progress;
if (fscanf(f, "%i %lu %lu %ms", &pass, &cur, &max, &device) != 4)
break;
- /* Only show one progress counter at max */
- if (!locked) {
- if (flock(fileno(console), LOCK_EX|LOCK_NB) < 0)
- continue;
-
- locked = true;
- }
-
/* Only update once every 50ms */
t = now(CLOCK_MONOTONIC);
if (last + 50 * USEC_PER_MSEC > t)
@@ -191,22 +178,15 @@ static int process_progress(int fd) {
last = t;
- p = percent(pass, cur, max);
- fprintf(console, "\r%s: fsck %3.1f%% complete...\r%n", device, p, &m);
- fflush(console);
-
- if (m > clear)
- clear = m;
- }
-
- if (clear > 0) {
- unsigned j;
+ /* send progress to fsckd */
+ strncpy(progress.device, device, PATH_MAX);
+ progress.cur = cur;
+ progress.max = max;
+ progress.pass = pass;
- fputc('\r', console);
- 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");
}
return 0;
diff --git a/src/fsckd/Makefile b/src/fsckd/Makefile
new file mode 120000
index 0000000..d0b0e8e
--- /dev/null
+++ b/src/fsckd/Makefile
@@ -0,0 +1 @@
+../Makefile
\ 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 -*-*/
+
+/***
+ This file is part of systemd.
+
+ Copyright 2015 Canonical
+
+
+ systemd is free software; you can redistribute it and/or modify it
+ under the terms of the GNU Lesser General Public License as published by
+ the Free Software Foundation; either version 2.1 of the License, or
+ (at your option) any later version.
+
+ systemd is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with systemd; If not, see <http://www.gnu.org/licenses/>.
+***/
+
+#include <getopt.h>
+#include <errno.h>
+#include <linux/limits.h>
+#include <math.h>
+#include <stdbool.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <sys/socket.h>
+#include <sys/types.h>
+#include <sys/un.h>
+#include <unistd.h>
+
+#include "build.h"
+#include "fsckd.h"
+#include "log.h"
+#include "list.h"
+#include "macro.h"
+#include "sd-daemon.h"
+#include "time-util.h"
+#include "util.h"
+
+#define IDLE_TIME_MINUTES 1
+
+typedef struct Clients {
+ int fd;
+ char device[PATH_MAX];
Maybe nicer to make this char*? PATH_MAX is a full page, overkill in most
circumstances.
+ unsigned long cur;
+ unsigned long max;
size_t?
+ int pass;
+ double percent;
+
+ LIST_FIELDS(struct Clients, clients);
+} Clients;
+
+static double compute_percent(int pass, unsigned long cur, unsigned long max) {
+ /* Values stolen from e2fsck */
+
+ static const int pass_table[] = {
+ 0, 70, 90, 92, 95, 100
+ };
+
+ if (pass <= 0)
+ return 0.0;
+
+ if ((unsigned) pass >= ELEMENTSOF(pass_table) || max == 0)
+ return 100.0;
+
+ return (double) pass_table[pass-1] +
+ ((double) pass_table[pass] - (double) pass_table[pass-1]) *
+ (double) cur / (double) max;
Just the (double) in front of cur should be enough.
+}
+
+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 ;))
+
+ console = fopen("/dev/console", "we");
+ if (!console) {
+ log_error("Can't connect to /dev/console");
+ return -ENOMEM;
+ }
Please don't make up the return value.
+
+ 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);
Capital "N".
+ current = alloca0(sizeof(Clients));
+ current->fd = new_client_fd;
+ if (!first)
+ LIST_INIT(clients, current);
+ else
+ LIST_PREPEND(clients, first, current);
+ first = current;
+ current = NULL;
+ }
+
+ LIST_FOREACH(clients, current, first) {
+ FsckProgress fsck_data;
+ int rc = 0;
Please use r for the return code.

Indentation is wrong.
+
+ 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);
+ current->fd = 0;
+ current->percent = 100;
+ } else if ((rc < 0) && (errno != EWOULDBLOCK)) {
Those parens are unnecessary.
+ 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.
+ current->percent = 100;
+ } else if (rc > 0) {
+ strncpy(current->device, fsck_data.device, PATH_MAX);
You should check that enough bytes have been read, and not read unitialized data.
+ current->cur = fsck_data.cur;
+ current->max = fsck_data.max;
+ current->pass = fsck_data.pass;
+ current->percent = compute_percent(current->pass, current->cur, current->max);
+
+ log_debug("Getting progress for %s: (%lu, %lu, %d) : %3.1f%%",
+ current->device, current->cur, current->max, current->pass, current->percent);
+ }
+
+ /* update global (eventually cached) values. */
+ if (current->fd > 0)
Wrong condition here too.
+ current_numdevices++;
+
+ /* right now, we only keep the minimum % of all fsckd processes. We could in the future trying to be
+ linear, but max changes and corresponds to the pass. We have all the informations into fsckd
+ already if we can treat that in a smarter way. */
+ current_percent = MIN(current_percent, current->percent);
+ }
+
+ 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);
oom check missing.
+
+ /* write to console */
+ fprintf(console, "\r%s\r%n", console_message, &m);
+ fflush(console);
+
+ if (m > clear)
+ clear = m;
+ }
+
+ /* idle out after IDLE_TIME_MINUTES minutes with no connected device */
+ t = now(CLOCK_MONOTONIC);
+ if (numdevices == 0) {
+ if (t > last_activity + IDLE_TIME_MINUTES * USEC_PER_MINUTE) {
+ log_debug("No fsck in progress for the last %d minutes, shutting down.", IDLE_TIME_MINUTES);
+ break;
+ }
+ } else
+ last_activity = t;
+ }
+
+ /* clear last line */
+ if (clear > 0) {
+ unsigned j;
+
+ fputc('\r', console);
+ for (j = 0; j < (unsigned) clear; j++)
+ fputc(' ', console);
+ fputc('\r', console);
+ fflush(console);
+ }
+
+ return 0;
+}
+
+static int create_socket(void) {
Can you base this on make_socket_fd() instead?
+ /* Create the socket manually for testing */
+ union {
+ struct sockaddr sa;
+ struct sockaddr_un un;
+ } sa;
+ int fd;
+
+ fd = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
+ if (fd < 0)
+ return log_error_errno(errno, "socket() failed: %m");
+
+ memset(&sa, 0, sizeof(sa));
+ sa.un.sun_family = AF_UNIX;
+ 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);
+
+ if (listen(fd, 5) < 0)
+ return log_error_errno(errno, "listening on %s failed: %m", FSCKD_SOCKET_PATH);
+
+ return fd;
+}
+
+static void help(void) {
+ printf("%s [OPTIONS...]\n\n"
+ "Capture fsck progress and forward one stream to plymouth\n\n"
+ " -h --help Show this help\n"
+ " --version Show package version\n",
+ program_invocation_short_name);
+}
+
+static int parse_argv(int argc, char *argv[]) {
+
+ enum {
+ ARG_VERSION = 0x100,
+ ARG_ROOT,
+ };
+
+ static const struct option options[] = {
+ { "help", no_argument, NULL, 'h' },
+ { "version", no_argument, NULL, ARG_VERSION },
+ {}
+ };
+
+ int c;
+
+ assert(argc >= 0);
+ assert(argv);
+
+ while ((c = getopt_long(argc, argv, "hv", options, NULL)) >= 0)
+ switch (c) {
+
+ help();
+ return 0;
+
+ puts(PACKAGE_STRING);
+ puts(SYSTEMD_FEATURES);
+ return 0;
+
+ return -EINVAL;
+
+ assert_not_reached("Unhandled option");
+ }
+
+ if (optind < argc) {
+ log_error("Extraneous arguments");
+ return -EINVAL;
+ }
+
+ return 1;
+}
+
+int main(int argc, char *argv[]) {
+ int r;
+ int fd, n;
+ int flags;
+
+ log_set_target(LOG_TARGET_AUTO);
+ log_parse_environment();
+ log_open();
+
+ r = parse_argv(argc, argv);
+ if (r <= 0)
+ return r < 0 ? EXIT_FAILURE : EXIT_SUCCESS;
+
+ n = sd_listen_fds(0);
+ if (n > 1) {
+ log_error("Too many file descriptors received.");
+ return EXIT_FAILURE;
+ } else if (n == 1) {
+ fd = SD_LISTEN_FDS_START + 0;
+ flags = fcntl(fd, F_GETFL, 0);
+ fcntl(fd, F_SETFL, flags | O_NONBLOCK);
+ } else {
+ fd = create_socket();
+ if (fd <= 0)
+ return EXIT_FAILURE;
+ }
+ return handle_requests(fd) < 0 ? EXIT_FAILURE : EXIT_SUCCESS;
+}
diff --git a/src/fsckd/fsckd.h b/src/fsckd/fsckd.h
new file mode 100644
index 0000000..e8cd014
--- /dev/null
+++ b/src/fsckd/fsckd.h
@@ -0,0 +1,34 @@
+/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
+
+/***
+ This file is part of systemd.
+
+ Copyright 2015 Canonical
+
+
+ systemd is free software; you can redistribute it and/or modify it
+ under the terms of the GNU Lesser General Public License as published by
+ the Free Software Foundation; either version 2.1 of the License, or
+ (at your option) any later version.
+
+ systemd is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with systemd; If not, see <http://www.gnu.org/licenses/>.
+***/
+
+#include <linux/limits.h>
+
+#define FSCKD_SOCKET_PATH "/run/systemd/fsckd"
+
+typedef struct FsckProgress {
+ unsigned long cur;
+ unsigned long max;
+ int pass;
+ char device[PATH_MAX];
+} FsckProgress;
--
2.1.4
_______________________________________________
systemd-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Zbyszek
Martin Pitt
2015-01-28 15:47:47 UTC
Permalink
Hey Zbyszek,
Post by Zbigniew Jędrzejewski-Szmek
+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 ;))
How is that not necessary? Just because the very next line initializes
it, and there is no exit path before? Because in the general case it
certainly is necessary, otherwise the _cleanup_ handler will try to
free/close/clean up a random pointer value and crash.

So IMHO it's a good defensive habit to always init _cleanup_* vars
with NULL. In the future someone might put some code with "return"
before the fopen() below, and then get a crash.

Or is there some gcc magic that I missed? (I thought only global
variables were guaranteed to be NULL, not stack vars).
Post by Zbigniew Jędrzejewski-Szmek
+
+ console = fopen("/dev/console", "we");
Thanks for clarifying,

Martin
--
Martin Pitt | http://www.piware.de
Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)
Dimitri John Ledkov
2015-01-28 16:06:30 UTC
Permalink
Post by Martin Pitt
Hey Zbyszek,
Post by Zbigniew Jędrzejewski-Szmek
+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 ;))
How is that not necessary? Just because the very next line initializes
it, and there is no exit path before? Because in the general case it
certainly is necessary, otherwise the _cleanup_ handler will try to
free/close/clean up a random pointer value and crash.
So IMHO it's a good defensive habit to always init _cleanup_* vars
with NULL. In the future someone might put some code with "return"
before the fopen() below, and then get a crash.
Or is there some gcc magic that I missed? (I thought only global
variables were guaranteed to be NULL, not stack vars).
Well, during systemd build there are some -Wmaybe-uninitialized
warnings.... in most cases it is code similar to the above case or
like checking r || check uninitialised variable. Shall we fix all of
them? In most cases it would be of limited gain, but at least it will
be clear to see when -Wmaybe-uninitalized warnings catches a real
thing.

(or is my compiler old and has false positives?)
Post by Martin Pitt
Post by Zbigniew Jędrzejewski-Szmek
+
+ console = fopen("/dev/console", "we");
Thanks for clarifying,
Martin
--
Martin Pitt | http://www.piware.de
Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)
_______________________________________________
systemd-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
--
Regards,

Dimitri.

Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ.
Zbigniew Jędrzejewski-Szmek
2015-01-28 16:51:21 UTC
Permalink
Post by Dimitri John Ledkov
Post by Martin Pitt
Hey Zbyszek,
Post by Zbigniew Jędrzejewski-Szmek
+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 ;))
How is that not necessary? Just because the very next line initializes
it, and there is no exit path before?
Yes.
Post by Dimitri John Ledkov
Post by Martin Pitt
Because in the general case it
certainly is necessary, otherwise the _cleanup_ handler will try to
free/close/clean up a random pointer value and crash.
Of course, but I think that in this case it is pretty obvious when
that the next line overwrites the NULL.
Post by Dimitri John Ledkov
Post by Martin Pitt
So IMHO it's a good defensive habit to always init _cleanup_* vars
with NULL. In the future someone might put some code with "return"
before the fopen() below, and then get a crash.
They can screw up in this and a thousand other ways. If there's a bunch
of other code in between, I agree that adding a defensive initialization
is good. In a simple case like this, I'm fine with skipping it.

Please note though, that my initial complaint was that there's an
inconsistency in the patch: one place had it, one didn't.
Post by Dimitri John Ledkov
Post by Martin Pitt
Or is there some gcc magic that I missed? (I thought only global
variables were guaranteed to be NULL, not stack vars).
No.
Post by Dimitri John Ledkov
Well, during systemd build there are some -Wmaybe-uninitialized
warnings.... in most cases it is code similar to the above case or
like checking r || check uninitialised variable. Shall we fix all of
them? In most cases it would be of limited gain, but at least it will
be clear to see when -Wmaybe-uninitalized warnings catches a real
thing.
gcc issues false positive warnings in two cases:
1. when the same conditional is used twice, and cannot change:

bool cond = ...;
int var;
if (cond)
var = ...;
...
if (cond)
use(var);

2. when there's a loop which must be executed at least one:
int var;
while(true) {
var = ...;
break;
}
use(var);

Adding workarounds is frowned upon, unless they are very simple. We
try not to have warnings in the -O0 compilation mode. There's just too
many false warnings at higher optimization levels to fix all.
Post by Dimitri John Ledkov
(or is my compiler old and has false positives?)
Hard to say, not knowing the version :)
gcc5 issues approx. 100000 new warnings.

Zbyszek
Dimitri John Ledkov
2015-01-28 16:58:14 UTC
Permalink
On 28 January 2015 at 16:51, Zbigniew Jędrzejewski-Szmek
Post by Zbigniew Jędrzejewski-Szmek
Post by Dimitri John Ledkov
Post by Martin Pitt
Hey Zbyszek,
(or is my compiler old and has false positives?)
Hard to say, not knowing the version :)
gcc5 issues approx. 100000 new warnings.
lovely, looking forward to patching out s/-Werror/-Wno-error/ all over
distributions again.

at least it's c11.
--
Regards,

Dimitri.

Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ.
Lennart Poettering
2015-01-28 20:19:46 UTC
Permalink
Post by Dimitri John Ledkov
Post by Martin Pitt
Hey Zbyszek,
Post by Zbigniew Jędrzejewski-Szmek
+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 ;))
How is that not necessary? Just because the very next line initializes
it, and there is no exit path before? Because in the general case it
certainly is necessary, otherwise the _cleanup_ handler will try to
free/close/clean up a random pointer value and crash.
So IMHO it's a good defensive habit to always init _cleanup_* vars
with NULL. In the future someone might put some code with "return"
before the fopen() below, and then get a crash.
Or is there some gcc magic that I missed? (I thought only global
variables were guaranteed to be NULL, not stack vars).
Well, during systemd build there are some -Wmaybe-uninitialized
warnings.... in most cases it is code similar to the above case or
like checking r || check uninitialised variable. Shall we fix all of
them? In most cases it would be of limited gain, but at least it will
be clear to see when -Wmaybe-uninitalized warnings catches a real
thing.
(or is my compiler old and has false positives?)
This is probably caused by LTO. The gcc warnings when using LTO are
not useful. While hacking it's a really good idea to disable LTO, the
warnings get more useful. It also saves you a lot of time to do so,
since LTO is awfully slow.

Lennart
--
Lennart Poettering, Red Hat
Lennart Poettering
2015-01-28 20:18:02 UTC
Permalink
Post by Martin Pitt
Hey Zbyszek,
Post by Zbigniew Jędrzejewski-Szmek
+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 ;))
How is that not necessary? Just because the very next line initializes
it, and there is no exit path before? Because in the general case it
certainly is necessary, otherwise the _cleanup_ handler will try to
free/close/clean up a random pointer value and crash.
So IMHO it's a good defensive habit to always init _cleanup_* vars
with NULL. In the future someone might put some code with "return"
before the fopen() below, and then get a crash.
Yeah, nowadays I tend to always initialize _cleanup_ vars at the time
i declare them. It's better to be safe than sorry...

Lennart
--
Lennart Poettering, Red Hat
Didier Roche
2015-01-29 17:37:53 UTC
Permalink
Hey Zbigniew,

Thanks for the quick feedbacks! Integrated your changes in the incoming
From d0f49b4e0994b9a1dd2738da18c5a2a6708b444f Mon Sep 17 00:00:00 2001
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
Zbigniew Jędrzejewski-Szmek
2015-01-31 00:34:33 UTC
Permalink
Post by Didier Roche
Hey Zbigniew,
Thanks for the quick feedbacks! Integrated your changes in the
From d0f49b4e0994b9a1dd2738da18c5a2a6708b444f Mon Sep 17 00:00:00 2001
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.
Post by Didier Roche
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.
Post by Didier Roche
+ 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.
Post by Didier Roche
+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
Didier Roche
2015-02-02 10:22:20 UTC
Permalink
Post by Zbigniew Jędrzejewski-Szmek
Post by Didier Roche
\ 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.
Right, done with -1 as init. Thanks!

Didier
Lennart Poettering
2015-01-28 20:16:14 UTC
Permalink
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)
%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)
%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
Didier Roche
2015-01-29 17:41:41 UTC
Permalink
Le 28/01/2015 21:16, Lennart Poettering a écrit :

Thanks for your feedbacks! Changes integrated, some remaining questions
Post by Lennart Poettering
+ 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)?
Post by Lennart Poettering
+ 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?
Post by Lennart Poettering
+
+ /* 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
Lennart Poettering
2015-02-02 21:00:13 UTC
Permalink
Post by Didier Roche
Thanks for your feedbacks! Changes integrated, some remaining questions
Post by Lennart Poettering
+ 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)?
Yes, please, always release all memory you allocate before
exiting. This makes life much much easier when valgrinding
things. There are only very few cases where it is OK to leave memory
unfreed on shutdown.
Post by Didier Roche
Post by Lennart Poettering
+ 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?
Sorry, I meant "proper error codes". I.e. print "errno". In systemd we
have this idiom of:

log_debug_errno(errno, "foo bar waldo: %m");

i.e. the log_debug_errno() macro takes the error number as first arg,
and then resolves it to a human readable string with "%m".

Lennart
--
Lennart Poettering, Red Hat
Didier Roche
2015-01-28 13:21:06 UTC
Permalink
Didier Roche
2015-01-28 13:21:35 UTC
Permalink
Didier Roche
2015-01-28 13:22:04 UTC
Permalink
Zbigniew Jędrzejewski-Szmek
2015-01-28 14:44:45 UTC
Permalink
From 7afe9270e3210668053089caaff8a1dd790a48f5 Mon Sep 17 00:00:00 2001
Date: Mon, 26 Jan 2015 17:07:32 +0100
Subject: [PATCH 04/12] Add some plymouth functionality to connect and send
messages
Connect to plymouth (if running).
Automatic reconnect if plymouth was disconnected (or respawned)
when trying to send a subsequent message.
---
Makefile.am | 2 ++
configure.ac | 12 ++++++++
src/shared/plymouth.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++
src/shared/plymouth.h | 8 +++++
4 files changed, 105 insertions(+)
diff --git a/Makefile.am b/Makefile.am
index 18be607..a9d61f1 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -929,10 +929,12 @@ libsystemd_shared_la_CFLAGS = \
$(AM_CFLAGS) \
$(CAP_CFLAGS) \
$(SECCOMP_CFLAGS) \
+ $(PLYMOUTH_CFLAGS) \
-pthread
libsystemd_shared_la_LIBADD = \
$(CAP_LIBS) \
+ $(PLYMOUTH_LIBS) \
-lm
# ------------------------------------------------------------------------------
diff --git a/configure.ac b/configure.ac
index 12e4ab2..62f1eef 100644
--- a/configure.ac
+++ b/configure.ac
@@ -857,6 +857,18 @@ fi
AM_CONDITIONAL(HAVE_MICROHTTPD, [test "$have_microhttpd" = "yes"])
# ------------------------------------------------------------------------------
+have_plymouth=no
+AC_ARG_ENABLE(plymouth, AS_HELP_STRING([--disable-plymouth], [disable plymouth integration]))
+if test "x$enable_plymouth" != "xno"; then
+ PKG_CHECK_MODULES([PLYMOUTH], [ply-boot-client >= 0.8.0],
+ [AC_DEFINE(HAVE_PLYMOUTH, 1, [Define if plymouth is available]) have_plymouth=yes], have_plymouth=no)
+ if test "x$have_plymouth" = xno -a "x$enable_plymouth" = xyes; then
+ AC_MSG_ERROR([*** plymouth integration requested but libraries not found])
+ fi
+fi
+AM_CONDITIONAL(HAVE_PLYMOUTH, [test "$have_plymouth" = "yes"])
+
+# ------------------------------------------------------------------------------
have_gnutls=no
AC_ARG_ENABLE(gnutls, AS_HELP_STRING([--disable-gnutls], [disable gnutls support]))
if test "x$enable_gnutls" != "xno"; then
diff --git a/src/shared/plymouth.c b/src/shared/plymouth.c
index c4865dd..f7155c4 100644
--- a/src/shared/plymouth.c
+++ b/src/shared/plymouth.c
@@ -19,13 +19,96 @@
along with systemd; If not, see <http://www.gnu.org/licenses/>.
***/
+#ifdef HAVE_PLYMOUTH
+#include <ply-boot-client.h>
+#endif
+
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
+#include "log.h"
#include "plymouth.h"
+#ifdef HAVE_PLYMOUTH
+void plymouth_disconnected (void *user_data, ply_boot_client_t *client);
+void plymouth_update_failed(void *user_data, ply_boot_client_t *client);
+
+static ply_boot_client_t *plymouth_client = NULL;
+static ply_event_loop_t *plymouth_event_loop = NULL;
+#endif
+
bool plymouth_running(void) {
return access("/run/plymouth/pid", F_OK) >= 0;
}
+
+#ifdef HAVE_PLYMOUTH
+bool plymouth_connect(void) {
Is there a particular reason why this cannot return a normal int code?
+
+ /* Keep existing connection */
+ if (plymouth_client)
+ return true;
+
+ /* Create the event loop */
+ if (!plymouth_event_loop)
Please make this {
+ plymouth_event_loop = ply_event_loop_new();
+
+ if (!plymouth_event_loop) {
+ log_error("Couldn't create event loop for plymouth");
+ return false;
+ }
} a subclause of the if. It looks weird otherwise.
+
+ plymouth_client = ply_boot_client_new();
+
+ if (!ply_boot_client_connect(plymouth_client, plymouth_disconnected, NULL)) {
+ log_error("Couldn't connect to plymouth");
+ ply_boot_client_free(plymouth_client);
+ plymouth_client = NULL;
+ plymouth_event_loop = NULL;
+ return false;
+ }
+
+ /* attach event loop after being connected to plymouth or the disconnect handler won't be registered
+ and flush all events that may exists from an older connection if we are reconnected */
+ ply_boot_client_attach_to_event_loop(plymouth_client, plymouth_event_loop);
+ ply_boot_client_flush(plymouth_client);
+
+ return true;
+}
+
+void plymouth_disconnect(void) {
+ if (!plymouth_client)
+ return;
+ ply_boot_client_disconnect(plymouth_client);
+ ply_boot_client_flush(plymouth_client);
+}
+
+void plymouth_update(const char *message) {
+ if (!plymouth_running() || !plymouth_connect())
+ return;
+
+ ply_boot_client_update_daemon(plymouth_client, message, NULL, NULL, NULL);
+ ply_boot_client_flush(plymouth_client);
+}
+
+void plymouth_delete_message(void) {
+ if (!plymouth_running() || !plymouth_client)
+ return;
+
+ ply_boot_client_tell_daemon_to_display_message (plymouth_client, "", NULL, plymouth_update_failed, NULL);
+ ply_boot_client_flush(plymouth_client);
+}
+
+void plymouth_update_failed(void *user_data, ply_boot_client_t *client) {
+ log_error("Couldn't send message to plymouth");
+}
+
+void plymouth_disconnected (void *user_data, ply_boot_client_t *client) {
Those functions which are not defined in plymouth.h should be static.
+ log_warning("Plymouth disconnected");
+ plymouth_delete_message();
+ ply_boot_client_disconnect(plymouth_client);
+ ply_boot_client_free(plymouth_client);
+ plymouth_client = NULL;
+}
+#endif
diff --git a/src/shared/plymouth.h b/src/shared/plymouth.h
index 39c8c37..15cecb7 100644
--- a/src/shared/plymouth.h
+++ b/src/shared/plymouth.h
@@ -24,3 +24,11 @@
#include <stdbool.h>
bool plymouth_running(void);
+
+#ifdef HAVE_PLYMOUTH
+bool plymouth_connect(void);
+void plymouth_disconnect(void);
+void plymouth_update(const char *message);
+
+void plymouth_delete_message(void);
+#endif
Zbyszek
Didier Roche
2015-01-29 17:42:42 UTC
Permalink
Post by Zbigniew Jędrzejewski-Szmek
From 7afe9270e3210668053089caaff8a1dd790a48f5 Mon Sep 17 00:00:00 2001
Date: Mon, 26 Jan 2015 17:07:32 +0100
Subject: [PATCH 04/12] Add some plymouth functionality to connect and send
messages
diff --git a/src/shared/plymouth.c b/src/shared/plymouth.c
index c4865dd..f7155c4 100644
--- a/src/shared/plymouth.c
+++ b/src/shared/plymouth.c
@@ -19,13 +19,96 @@
along with systemd; If not, see <http://www.gnu.org/licenses/>.
***/
+#ifdef HAVE_PLYMOUTH
+#include <ply-boot-client.h>
+#endif
+
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
+#include "log.h"
#include "plymouth.h"
+#ifdef HAVE_PLYMOUTH
+void plymouth_disconnected (void *user_data, ply_boot_client_t *client);
+void plymouth_update_failed(void *user_data, ply_boot_client_t *client);
+
+static ply_boot_client_t *plymouth_client = NULL;
+static ply_event_loop_t *plymouth_event_loop = NULL;
+#endif
+
bool plymouth_running(void) {
return access("/run/plymouth/pid", F_OK) >= 0;
}
+
+#ifdef HAVE_PLYMOUTH
+bool plymouth_connect(void) {
Is there a particular reason why this cannot return a normal int code?
No reason, I changed to 0 for connected, -1 for can't connect.

Other changes done, thanks!
Cheers,
Didier
Zbigniew Jędrzejewski-Szmek
2015-01-31 00:44:49 UTC
Permalink
Post by Didier Roche
Post by Zbigniew Jędrzejewski-Szmek
bool plymouth_running(void) {
return access("/run/plymouth/pid", F_OK) >= 0;
}
+
+#ifdef HAVE_PLYMOUTH
+bool plymouth_connect(void) {
Is there a particular reason why this cannot return a normal int code?
No reason, I changed to 0 for connected, -1 for can't connect.
No, please return a real return code (-ENOMEM on the allocation failure
in this case). -1 is EPERM.

Zbyszek
Didier Roche
2015-02-02 10:22:15 UTC
Permalink
Post by Zbigniew Jędrzejewski-Szmek
Post by Didier Roche
Post by Zbigniew Jędrzejewski-Szmek
bool plymouth_running(void) {
return access("/run/plymouth/pid", F_OK) >= 0;
}
+
+#ifdef HAVE_PLYMOUTH
+bool plymouth_connect(void) {
Is there a particular reason why this cannot return a normal int code?
No reason, I changed to 0 for connected, -1 for can't connect.
No, please return a real return code (-ENOMEM on the allocation failure
in this case). -1 is EPERM.
Done, (even if I think that particular function is going to change with
the libplymouth dep removal.
Post by Zbigniew Jędrzejewski-Szmek
Zbyszek
Lennart Poettering
2015-01-28 20:22:26 UTC
Permalink
# ------------------------------------------------------------------------------
+have_plymouth=no
+AC_ARG_ENABLE(plymouth, AS_HELP_STRING([--disable-plymouth], [disable plymouth integration]))
+if test "x$enable_plymouth" != "xno"; then
+ PKG_CHECK_MODULES([PLYMOUTH], [ply-boot-client >= 0.8.0],
+ [AC_DEFINE(HAVE_PLYMOUTH, 1, [Define if plymouth is available]) have_plymouth=yes], have_plymouth=no)
+ if test "x$have_plymouth" = xno -a "x$enable_plymouth" = xyes; then
+ AC_MSG_ERROR([*** plymouth integration requested but libraries not found])
+ fi
+fi
+AM_CONDITIONAL(HAVE_PLYMOUTH, [test "$have_plymouth" = "yes"])
Hmm, I am bit concerned about adding this dependency. So far we
managed to talk to plymouth without using its library, and I am really
not sure we should start doing so now. So far the messages to send
were so simply that it really wasn't worth the effort to use the full
library.

Lennart
--
Lennart Poettering, Red Hat
Didier Roche
2015-01-29 17:43:22 UTC
Permalink
Post by Lennart Poettering
# ------------------------------------------------------------------------------
+have_plymouth=no
+AC_ARG_ENABLE(plymouth, AS_HELP_STRING([--disable-plymouth], [disable plymouth integration]))
+if test "x$enable_plymouth" != "xno"; then
+ PKG_CHECK_MODULES([PLYMOUTH], [ply-boot-client >= 0.8.0],
+ [AC_DEFINE(HAVE_PLYMOUTH, 1, [Define if plymouth is available]) have_plymouth=yes], have_plymouth=no)
+ if test "x$have_plymouth" = xno -a "x$enable_plymouth" = xyes; then
+ AC_MSG_ERROR([*** plymouth integration requested but libraries not found])
+ fi
+fi
+AM_CONDITIONAL(HAVE_PLYMOUTH, [test "$have_plymouth" = "yes"])
Hmm, I am bit concerned about adding this dependency. So far we
managed to talk to plymouth without using its library, and I am really
not sure we should start doing so now. So far the messages to send
were so simply that it really wasn't worth the effort to use the full
library.
This is doable for the first part, similar to what
src/tty-ask-password-agent/tty-ask-password-agent.c is doing (using the
socket directly to send update and message to it).
I'm quite unsure "watch and get key events" part as looking at
libplymouth code, this seems quite more complex as a protocol to
achieve. If you feel that needs to be done anyway, I can look deeper at
this if you really feel we should reimplement libplymouth protocol
rathen than having an optional dep on it.

Didier
Zbigniew Jędrzejewski-Szmek
2015-01-31 00:39:43 UTC
Permalink
Post by Didier Roche
Post by Lennart Poettering
# ------------------------------------------------------------------------------
+have_plymouth=no
+AC_ARG_ENABLE(plymouth, AS_HELP_STRING([--disable-plymouth], [disable plymouth integration]))
+if test "x$enable_plymouth" != "xno"; then
+ PKG_CHECK_MODULES([PLYMOUTH], [ply-boot-client >= 0.8.0],
+ [AC_DEFINE(HAVE_PLYMOUTH, 1, [Define if plymouth is available]) have_plymouth=yes], have_plymouth=no)
+ if test "x$have_plymouth" = xno -a "x$enable_plymouth" = xyes; then
+ AC_MSG_ERROR([*** plymouth integration requested but libraries not found])
+ fi
+fi
+AM_CONDITIONAL(HAVE_PLYMOUTH, [test "$have_plymouth" = "yes"])
Hmm, I am bit concerned about adding this dependency. So far we
managed to talk to plymouth without using its library, and I am really
not sure we should start doing so now. So far the messages to send
were so simply that it really wasn't worth the effort to use the full
library.
This is doable for the first part, similar to what
src/tty-ask-password-agent/tty-ask-password-agent.c is doing (using
the socket directly to send update and message to it).
I'm quite unsure "watch and get key events" part as looking at
libplymouth code, this seems quite more complex as a protocol to
achieve. If you feel that needs to be done anyway, I can look deeper
at this if you really feel we should reimplement libplymouth
protocol rathen than having an optional dep on it.
plymouth-core-libs are 200kb in Fedora. I wouldn't sweat it too much.

Zbyszek
Didier Roche
2015-02-02 10:22:07 UTC
Permalink
Post by Zbigniew Jędrzejewski-Szmek
Post by Didier Roche
Post by Lennart Poettering
# ------------------------------------------------------------------------------
+have_plymouth=no
+AC_ARG_ENABLE(plymouth, AS_HELP_STRING([--disable-plymouth], [disable plymouth integration]))
+if test "x$enable_plymouth" != "xno"; then
+ PKG_CHECK_MODULES([PLYMOUTH], [ply-boot-client >= 0.8.0],
+ [AC_DEFINE(HAVE_PLYMOUTH, 1, [Define if plymouth is available]) have_plymouth=yes], have_plymouth=no)
+ if test "x$have_plymouth" = xno -a "x$enable_plymouth" = xyes; then
+ AC_MSG_ERROR([*** plymouth integration requested but libraries not found])
+ fi
+fi
+AM_CONDITIONAL(HAVE_PLYMOUTH, [test "$have_plymouth" = "yes"])
Hmm, I am bit concerned about adding this dependency. So far we
managed to talk to plymouth without using its library, and I am really
not sure we should start doing so now. So far the messages to send
were so simply that it really wasn't worth the effort to use the full
library.
This is doable for the first part, similar to what
src/tty-ask-password-agent/tty-ask-password-agent.c is doing (using
the socket directly to send update and message to it).
I'm quite unsure "watch and get key events" part as looking at
libplymouth code, this seems quite more complex as a protocol to
achieve. If you feel that needs to be done anyway, I can look deeper
at this if you really feel we should reimplement libplymouth
protocol rathen than having an optional dep on it.
plymouth-core-libs are 200kb in Fedora. I wouldn't sweat it too much.
We discussed it during the hackfest, and got thanks to Lennart's strace
suggestion the protocol figured out, will work on that before resending
the patch list then (I hope, this week if I get time to look at this).

Didier
Post by Zbigniew Jędrzejewski-Szmek
Zbyszek
Didier Roche
2015-01-28 13:22:30 UTC
Permalink
Zbigniew Jędrzejewski-Szmek
2015-01-28 14:47:58 UTC
Permalink
From c60d4f41e279dd5ed7134d97d95549aac1f38e69 Mon Sep 17 00:00:00 2001
Date: Mon, 26 Jan 2015 16:29:30 +0100
Subject: [PATCH 05/12] Connect and send to plymouth progress report
Try to connect and send to plymouth (if running) some check report progress,
using libplymouth.
fsckd:<num_devices>:<progress>:<string>
* num_devices corresponds to the current number of devices being checked (int)
* progress corresponds to the current minimum percentage of all devices being
checking (float, from 0 to 100)
checked
* string is a translated message ready to be displayed by the plymouth theme
displaying the information above. It can be overriden by plymouth themes
supporting i18n.
---
src/fsckd/fsckd.c | 23 ++++++++++++++++++++++-
src/shared/plymouth.c | 15 +++++++++++++++
src/shared/plymouth.h | 2 ++
3 files changed, 39 insertions(+), 1 deletion(-)
diff --git a/src/fsckd/fsckd.c b/src/fsckd/fsckd.c
index e27cd6d..b516193 100644
--- a/src/fsckd/fsckd.c
+++ b/src/fsckd/fsckd.c
@@ -39,6 +39,9 @@
#include "log.h"
#include "list.h"
#include "macro.h"
+#ifdef HAVE_PLYMOUTH
+#include "plymouth.h"
+#endif
#include "sd-daemon.h"
#include "time-util.h"
#include "util.h"
@@ -95,6 +98,9 @@ static int handle_requests(int socket_fd) {
int new_client_fd = 0;
Clients *current;
_cleanup_free_ char *console_message = NULL;
+#ifdef HAVE_PLYMOUTH
+ _cleanup_free_ char *fsck_message = NULL;
+#endif
double current_percent = 100;
int current_numdevices = 0, m = 0;
usec_t t;
@@ -155,6 +161,9 @@ static int handle_requests(int socket_fd) {
asprintf(&console_message, "Checking in progress on %d disks (%3.1f%% complete)",
numdevices, percent);
+#ifdef HAVE_PLYMOUTH
+ asprintf(&fsck_message, "fsckd:%d:%3.1f:%s", numdevices, percent, console_message);
+#endif
oom check.
/* write to console */
if (show_progress) {
@@ -162,6 +171,11 @@ static int handle_requests(int socket_fd) {
fflush(console);
}
+#ifdef HAVE_PLYMOUTH
+ /* send to plymouth */
+ plymouth_update(fsck_message);
+#endif
+
if (m > clear)
clear = m;
}
@@ -187,6 +201,9 @@ static int handle_requests(int socket_fd) {
fputc('\r', console);
fflush(console);
}
+#ifdef HAVE_PLYMOUTH
+ plymouth_disconnect();
+#endif
return 0;
}
@@ -218,7 +235,11 @@ static int create_socket(void) {
static void help(void) {
printf("%s [OPTIONS...]\n\n"
- "Capture fsck progress and forward one stream to plymouth\n\n"
+#ifdef HAVE_PLYMOUTH
+ "Capture fsck progress, log to console and forward one stream to plymouth\n\n"
+#else
+ "Capture fsck progress and log to console\n\n"
+#endif
" -h --help Show this help\n"
" --version Show package version\n",
program_invocation_short_name);
diff --git a/src/shared/plymouth.c b/src/shared/plymouth.c
index f7155c4..b43d355 100644
--- a/src/shared/plymouth.c
+++ b/src/shared/plymouth.c
@@ -92,6 +92,21 @@ void plymouth_update(const char *message) {
ply_boot_client_flush(plymouth_client);
}
+static void plymouth_key_pressed(void *callback, const char *pressed_key, ply_boot_client_t *client) {
+ ((keypress_callback)callback)();
+}
+
+bool plymouth_watch_key(const char *keys, const char *message, keypress_callback callback) {
+ if (!plymouth_running() || !plymouth_connect())
+ return false;
+
+ ply_boot_client_tell_daemon_to_display_message (plymouth_client, message, NULL,
+ plymouth_update_failed, NULL);
+ ply_boot_client_ask_daemon_to_watch_for_keystroke (plymouth_client, keys, plymouth_key_pressed,
+ NULL, callback);
+ return true;
+}
+
void plymouth_delete_message(void) {
if (!plymouth_running() || !plymouth_client)
return;
diff --git a/src/shared/plymouth.h b/src/shared/plymouth.h
index 15cecb7..f5ea00c 100644
--- a/src/shared/plymouth.h
+++ b/src/shared/plymouth.h
@@ -30,5 +30,7 @@ bool plymouth_connect(void);
void plymouth_disconnect(void);
void plymouth_update(const char *message);
+typedef void (*keypress_callback)(void);
void plymouth_delete_message(void);
+bool plymouth_watch_key(const char *keys, const char *message, keypress_callback callback);
#endif
Zbyszek
Lennart Poettering
2015-01-28 20:23:44 UTC
Permalink
+#ifdef HAVE_PLYMOUTH
+ asprintf(&fsck_message, "fsckd:%d:%3.1f:%s", numdevices, percent, console_message);
+#endif
OOM check.

The communcaition here with plymouth still looks simple enough to just
do that directly in our code, the way we did it so far...

Lennart
--
Lennart Poettering, Red Hat
Didier Roche
2015-01-28 13:22:54 UTC
Permalink
Zbigniew Jędrzejewski-Szmek
2015-01-28 14:53:17 UTC
Permalink
From 104cf82ba28941e907f277a713f834ceb3d909f0 Mon Sep 17 00:00:00 2001
Date: Mon, 26 Jan 2015 16:40:52 +0100
Subject: [PATCH 06/12] Support cancellation of fsck in progress
Grab in fsckd plymouth watch key for C or c, and propagate this cancel request
to systemd-fsck which will terminate fsck.
Could we bind to ^c or if this is not possible, "three c's in three
seconds" instead? I'm worried that before you could press anything to little
effect in plymouth, and now a single key will have significant consequences.
Send a message to signal to user what key we are grabbing for fsck cancel.
Message is: fsckd-cancel-msg:<string>
Where string is a translated string ready to be displayed by the plymouth theme
indicating that c or C can be used to cancel current checks. It can be
overriden (matching only fsckd-cancel-msg prefix) for themes supporting i18n.
---
src/fsck/fsck.c | 29 +++++++++++++++++++++--------
src/fsckd/fsckd.c | 43 +++++++++++++++++++++++++++++++++++++++++++
src/fsckd/fsckd.h | 5 +++++
3 files changed, 69 insertions(+), 8 deletions(-)
diff --git a/src/fsck/fsck.c b/src/fsck/fsck.c
index f5dd546..0b42e3b 100644
--- a/src/fsck/fsck.c
+++ b/src/fsck/fsck.c
@@ -47,6 +47,8 @@
static bool arg_skip = false;
static bool arg_force = false;
static const char *arg_repair = "-a";
+static pid_t fsck_pid;
+static bool cancel_requested = false;
static void start_target(const char *target) {
_cleanup_bus_error_free_ sd_bus_error error = SD_BUS_ERROR_NULL;
@@ -165,6 +167,7 @@ static int process_progress(int fd) {
ssize_t n;
usec_t t;
FsckProgress progress;
+ FsckdMessage fsckd_message;
if (fscanf(f, "%i %lu %lu %ms", &pass, &cur, &max, &device) != 4)
break;
@@ -185,6 +188,16 @@ static int process_progress(int fd) {
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");
+
+ /* get fsckd requests */
+ n = recv(fsckd_fd, &fsckd_message, sizeof(FsckdMessage), 0);
+ if (n > 0) {
+ if (fsckd_message.cancel) {
+ log_warning("Request to cancel fsck from fsckd");
+ cancel_requested = true;
+ kill(fsck_pid, SIGTERM);
+ }
+ }
}
return 0;
@@ -193,7 +206,6 @@ static int process_progress(int fd) {
int main(int argc, char *argv[]) {
const char *cmdline[9];
int i = 0, r = EXIT_FAILURE, q;
- pid_t pid;
siginfo_t status;
_cleanup_udev_unref_ struct udev *udev = NULL;
_cleanup_udev_device_unref_ struct udev_device *udev_device = NULL;
@@ -321,11 +333,11 @@ int main(int argc, char *argv[]) {
cmdline[i++] = device;
cmdline[i++] = NULL;
- pid = fork();
- if (pid < 0) {
+ fsck_pid = fork();
+ if (fsck_pid < 0) {
log_error_errno(errno, "fork(): %m");
goto finish;
- } else if (pid == 0) {
+ } else if (fsck_pid == 0) {
/* Child */
if (progress_pipe[0] >= 0)
safe_close(progress_pipe[0]);
@@ -340,7 +352,7 @@ int main(int argc, char *argv[]) {
progress_pipe[0] = -1;
}
- q = wait_for_terminate(pid, &status);
+ q = wait_for_terminate(fsck_pid, &status);
if (q < 0) {
log_error_errno(q, "waitid(): %m");
goto finish;
@@ -348,11 +360,11 @@ int main(int argc, char *argv[]) {
if (status.si_code != CLD_EXITED || (status.si_status & ~1)) {
- if (status.si_code == CLD_KILLED || status.si_code == CLD_DUMPED)
+ if ((!cancel_requested && status.si_code == CLD_KILLED) || status.si_code == CLD_DUMPED)
log_error("fsck terminated by signal %s.", signal_to_string(status.si_status));
else if (status.si_code == CLD_EXITED)
log_error("fsck failed with error code %i.", status.si_status);
- else
+ else if (!cancel_requested)
log_error("fsck failed due to unknown reason.");
if (status.si_code == CLD_EXITED && (status.si_status & 2) && root_directory)
@@ -363,7 +375,8 @@ int main(int argc, char *argv[]) {
start_target(SPECIAL_EMERGENCY_TARGET);
else {
r = EXIT_SUCCESS;
- log_warning("Ignoring error.");
+ if (!cancel_requested)
+ log_warning("Ignoring error.");
}
} else
diff --git a/src/fsckd/fsckd.c b/src/fsckd/fsckd.c
index b516193..5760916 100644
--- a/src/fsckd/fsckd.c
+++ b/src/fsckd/fsckd.c
@@ -57,10 +57,15 @@ typedef struct Clients {
unsigned long max;
int pass;
double percent;
+ bool cancelled;
LIST_FIELDS(struct Clients, clients);
} Clients;
+#ifdef HAVE_PLYMOUTH
+static bool cancelled = false;
+#endif
+
static double compute_percent(int pass, unsigned long cur, unsigned long max) {
/* Values stolen from e2fsck */
@@ -79,12 +84,25 @@ static double compute_percent(int pass, unsigned long cur, unsigned long max) {
(double) cur / (double) max;
}
+#ifdef HAVE_PLYMOUTH
+static void cancel_requested(void) {
+ log_debug("Request to cancel fsck checking received");
+ cancelled = true;
+}
+#endif
+
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;
+#ifdef HAVE_PLYMOUTH
+ const char *plymouth_cancel_message;
+ bool cancel_message_plymouth_sent = false;
+
+ plymouth_cancel_message = strappenda("fsckd-cancel-msg:", "Press C to cancel all checks in progress");
+#endif
console = fopen("/dev/console", "we");
if (!console) {
@@ -111,6 +129,7 @@ static int handle_requests(int socket_fd) {
log_debug("new fsck client connected to fd: %d", new_client_fd);
current = alloca0(sizeof(Clients));
current->fd = new_client_fd;
+ current->cancelled = false;
if (!first)
LIST_INIT(clients, current);
else
@@ -143,6 +162,20 @@ static int handle_requests(int socket_fd) {
log_debug("Getting progress for %s: (%lu, %lu, %d) : %3.1f%%",
current->device, current->cur, current->max, current->pass, current->percent);
+
+#ifdef HAVE_PLYMOUTH
+ /* send cancel message if cancel key was pressed (and the socket wasn't closed) */
+ if (cancelled && !current->cancelled) {
+ FsckdMessage cancel_msg;
+ ssize_t n;
+ cancel_msg.cancel = true;
+ n = send(current->fd, &cancel_msg, sizeof(FsckdMessage), 0);
+ if (n < 0 || (size_t) n < sizeof(FsckdMessage))
+ log_warning_errno(n, "Cannot send cancel to fsck on %s: %m", current->device);
+ else
+ current->cancelled = true;
+ }
+#endif
}
/* update global (eventually cached) values. */
@@ -173,6 +206,10 @@ static int handle_requests(int socket_fd) {
#ifdef HAVE_PLYMOUTH
/* send to plymouth */
+ if (!cancel_message_plymouth_sent) {
+ cancel_message_plymouth_sent = \
+ plymouth_watch_key("Cc", plymouth_cancel_message, cancel_requested);
+ }
plymouth_update(fsck_message);
#endif
@@ -183,6 +220,12 @@ static int handle_requests(int socket_fd) {
/* idle out after IDLE_TIME_MINUTES minutes with no connected device */
t = now(CLOCK_MONOTONIC);
if (numdevices == 0) {
+#ifdef HAVE_PLYMOUTH
+ if (cancel_message_plymouth_sent) {
+ plymouth_delete_message();
+ cancel_message_plymouth_sent = false;
+ }
+#endif
if (t > last_activity + IDLE_TIME_MINUTES * USEC_PER_MINUTE) {
log_debug("No fsck in progress for the last %d minutes, shutting down.", IDLE_TIME_MINUTES);
break;
diff --git a/src/fsckd/fsckd.h b/src/fsckd/fsckd.h
index e8cd014..6a029ec 100644
--- a/src/fsckd/fsckd.h
+++ b/src/fsckd/fsckd.h
@@ -23,6 +23,7 @@
***/
#include <linux/limits.h>
+#include <stdbool.h>
#define FSCKD_SOCKET_PATH "/run/systemd/fsckd"
@@ -32,3 +33,7 @@ typedef struct FsckProgress {
int pass;
char device[PATH_MAX];
} FsckProgress;
+
+typedef struct FsckdMessage {
+ bool cancel;
+} FsckdMessage;
Zbyszek
Dimitri John Ledkov
2015-01-28 15:21:27 UTC
Permalink
On 28 January 2015 at 14:53, Zbigniew Jędrzejewski-Szmek
Post by Zbigniew Jędrzejewski-Szmek
From 104cf82ba28941e907f277a713f834ceb3d909f0 Mon Sep 17 00:00:00 2001
Date: Mon, 26 Jan 2015 16:40:52 +0100
Subject: [PATCH 06/12] Support cancellation of fsck in progress
Grab in fsckd plymouth watch key for C or c, and propagate this cancel request
to systemd-fsck which will terminate fsck.
Could we bind to ^c or if this is not possible, "three c's in three
seconds" instead? I'm worried that before you could press anything to little
effect in plymouth, and now a single key will have significant consequences.
Hm? an interactive message with key-binding is usually shown and then
plymouth reacts to such a key prompt.
This is how it has always worked on plymouth prompts since forever...
thus this would not be a surprise to most plymouth users (~ 5+ years
by now?!)
Doing it otherwise, will, on the contrary, impede user experience.
Post by Zbigniew Jędrzejewski-Szmek
Send a message to signal to user what key we are grabbing for fsck cancel.
Message is: fsckd-cancel-msg:<string>
Where string is a translated string ready to be displayed by the plymouth theme
indicating that c or C can be used to cancel current checks. It can be
overriden (matching only fsckd-cancel-msg prefix) for themes supporting i18n.
---
src/fsck/fsck.c | 29 +++++++++++++++++++++--------
src/fsckd/fsckd.c | 43 +++++++++++++++++++++++++++++++++++++++++++
src/fsckd/fsckd.h | 5 +++++
3 files changed, 69 insertions(+), 8 deletions(-)
diff --git a/src/fsck/fsck.c b/src/fsck/fsck.c
index f5dd546..0b42e3b 100644
--- a/src/fsck/fsck.c
+++ b/src/fsck/fsck.c
@@ -47,6 +47,8 @@
static bool arg_skip = false;
static bool arg_force = false;
static const char *arg_repair = "-a";
+static pid_t fsck_pid;
+static bool cancel_requested = false;
static void start_target(const char *target) {
_cleanup_bus_error_free_ sd_bus_error error = SD_BUS_ERROR_NULL;
@@ -165,6 +167,7 @@ static int process_progress(int fd) {
ssize_t n;
usec_t t;
FsckProgress progress;
+ FsckdMessage fsckd_message;
if (fscanf(f, "%i %lu %lu %ms", &pass, &cur, &max, &device) != 4)
break;
@@ -185,6 +188,16 @@ static int process_progress(int fd) {
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");
+
+ /* get fsckd requests */
+ n = recv(fsckd_fd, &fsckd_message, sizeof(FsckdMessage), 0);
+ if (n > 0) {
+ if (fsckd_message.cancel) {
+ log_warning("Request to cancel fsck from fsckd");
+ cancel_requested = true;
+ kill(fsck_pid, SIGTERM);
+ }
+ }
}
return 0;
@@ -193,7 +206,6 @@ static int process_progress(int fd) {
int main(int argc, char *argv[]) {
const char *cmdline[9];
int i = 0, r = EXIT_FAILURE, q;
- pid_t pid;
siginfo_t status;
_cleanup_udev_unref_ struct udev *udev = NULL;
_cleanup_udev_device_unref_ struct udev_device *udev_device = NULL;
@@ -321,11 +333,11 @@ int main(int argc, char *argv[]) {
cmdline[i++] = device;
cmdline[i++] = NULL;
- pid = fork();
- if (pid < 0) {
+ fsck_pid = fork();
+ if (fsck_pid < 0) {
log_error_errno(errno, "fork(): %m");
goto finish;
- } else if (pid == 0) {
+ } else if (fsck_pid == 0) {
/* Child */
if (progress_pipe[0] >= 0)
safe_close(progress_pipe[0]);
@@ -340,7 +352,7 @@ int main(int argc, char *argv[]) {
progress_pipe[0] = -1;
}
- q = wait_for_terminate(pid, &status);
+ q = wait_for_terminate(fsck_pid, &status);
if (q < 0) {
log_error_errno(q, "waitid(): %m");
goto finish;
@@ -348,11 +360,11 @@ int main(int argc, char *argv[]) {
if (status.si_code != CLD_EXITED || (status.si_status & ~1)) {
- if (status.si_code == CLD_KILLED || status.si_code == CLD_DUMPED)
+ if ((!cancel_requested && status.si_code == CLD_KILLED) || status.si_code == CLD_DUMPED)
log_error("fsck terminated by signal %s.", signal_to_string(status.si_status));
else if (status.si_code == CLD_EXITED)
log_error("fsck failed with error code %i.", status.si_status);
- else
+ else if (!cancel_requested)
log_error("fsck failed due to unknown reason.");
if (status.si_code == CLD_EXITED && (status.si_status & 2) && root_directory)
@@ -363,7 +375,8 @@ int main(int argc, char *argv[]) {
start_target(SPECIAL_EMERGENCY_TARGET);
else {
r = EXIT_SUCCESS;
- log_warning("Ignoring error.");
+ if (!cancel_requested)
+ log_warning("Ignoring error.");
}
} else
diff --git a/src/fsckd/fsckd.c b/src/fsckd/fsckd.c
index b516193..5760916 100644
--- a/src/fsckd/fsckd.c
+++ b/src/fsckd/fsckd.c
@@ -57,10 +57,15 @@ typedef struct Clients {
unsigned long max;
int pass;
double percent;
+ bool cancelled;
LIST_FIELDS(struct Clients, clients);
} Clients;
+#ifdef HAVE_PLYMOUTH
+static bool cancelled = false;
+#endif
+
static double compute_percent(int pass, unsigned long cur, unsigned long max) {
/* Values stolen from e2fsck */
@@ -79,12 +84,25 @@ static double compute_percent(int pass, unsigned long cur, unsigned long max) {
(double) cur / (double) max;
}
+#ifdef HAVE_PLYMOUTH
+static void cancel_requested(void) {
+ log_debug("Request to cancel fsck checking received");
+ cancelled = true;
+}
+#endif
+
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;
+#ifdef HAVE_PLYMOUTH
+ const char *plymouth_cancel_message;
+ bool cancel_message_plymouth_sent = false;
+
+ plymouth_cancel_message = strappenda("fsckd-cancel-msg:", "Press C to cancel all checks in progress");
+#endif
console = fopen("/dev/console", "we");
if (!console) {
@@ -111,6 +129,7 @@ static int handle_requests(int socket_fd) {
log_debug("new fsck client connected to fd: %d", new_client_fd);
current = alloca0(sizeof(Clients));
current->fd = new_client_fd;
+ current->cancelled = false;
if (!first)
LIST_INIT(clients, current);
else
@@ -143,6 +162,20 @@ static int handle_requests(int socket_fd) {
log_debug("Getting progress for %s: (%lu, %lu, %d) : %3.1f%%",
current->device, current->cur, current->max, current->pass, current->percent);
+
+#ifdef HAVE_PLYMOUTH
+ /* send cancel message if cancel key was pressed (and the socket wasn't closed) */
+ if (cancelled && !current->cancelled) {
+ FsckdMessage cancel_msg;
+ ssize_t n;
+ cancel_msg.cancel = true;
+ n = send(current->fd, &cancel_msg, sizeof(FsckdMessage), 0);
+ if (n < 0 || (size_t) n < sizeof(FsckdMessage))
+ log_warning_errno(n, "Cannot send cancel to fsck on %s: %m", current->device);
+ else
+ current->cancelled = true;
+ }
+#endif
}
/* update global (eventually cached) values. */
@@ -173,6 +206,10 @@ static int handle_requests(int socket_fd) {
#ifdef HAVE_PLYMOUTH
/* send to plymouth */
+ if (!cancel_message_plymouth_sent) {
+ cancel_message_plymouth_sent = \
+ plymouth_watch_key("Cc", plymouth_cancel_message, cancel_requested);
+ }
plymouth_update(fsck_message);
#endif
@@ -183,6 +220,12 @@ static int handle_requests(int socket_fd) {
/* idle out after IDLE_TIME_MINUTES minutes with no connected device */
t = now(CLOCK_MONOTONIC);
if (numdevices == 0) {
+#ifdef HAVE_PLYMOUTH
+ if (cancel_message_plymouth_sent) {
+ plymouth_delete_message();
+ cancel_message_plymouth_sent = false;
+ }
+#endif
if (t > last_activity + IDLE_TIME_MINUTES * USEC_PER_MINUTE) {
log_debug("No fsck in progress for the last %d minutes, shutting down.", IDLE_TIME_MINUTES);
break;
diff --git a/src/fsckd/fsckd.h b/src/fsckd/fsckd.h
index e8cd014..6a029ec 100644
--- a/src/fsckd/fsckd.h
+++ b/src/fsckd/fsckd.h
@@ -23,6 +23,7 @@
***/
#include <linux/limits.h>
+#include <stdbool.h>
#define FSCKD_SOCKET_PATH "/run/systemd/fsckd"
@@ -32,3 +33,7 @@ typedef struct FsckProgress {
int pass;
char device[PATH_MAX];
} FsckProgress;
+
+typedef struct FsckdMessage {
+ bool cancel;
+} FsckdMessage;
Zbyszek
_______________________________________________
systemd-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
--
Regards,

Dimitri.

Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ.
Martin Pitt
2015-01-28 15:30:19 UTC
Permalink
Post by Dimitri John Ledkov
Hm? an interactive message with key-binding is usually shown and then
plymouth reacts to such a key prompt.
This is how it has always worked on plymouth prompts since forever...
thus this would not be a surprise to most plymouth users (~ 5+ years
by now?!)
Doing it otherwise, will, on the contrary, impede user experience.
I don't see anything wrong with always making Control-C work for the
Unix wizards, and then the more "human friendly" annouced key bindings
which can be translated, and thus also work on keyboards where you
don't have a "c" in the first place.

Martin
--
Martin Pitt | http://www.piware.de
Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)
Zbigniew Jędrzejewski-Szmek
2015-01-28 15:31:02 UTC
Permalink
Post by Dimitri John Ledkov
On 28 January 2015 at 14:53, Zbigniew Jędrzejewski-Szmek
Post by Zbigniew Jędrzejewski-Szmek
From 104cf82ba28941e907f277a713f834ceb3d909f0 Mon Sep 17 00:00:00 2001
Date: Mon, 26 Jan 2015 16:40:52 +0100
Subject: [PATCH 06/12] Support cancellation of fsck in progress
Grab in fsckd plymouth watch key for C or c, and propagate this cancel request
to systemd-fsck which will terminate fsck.
Could we bind to ^c or if this is not possible, "three c's in three
seconds" instead? I'm worried that before you could press anything to little
effect in plymouth, and now a single key will have significant consequences.
Hm? an interactive message with key-binding is usually shown and then
plymouth reacts to such a key prompt.
This is how it has always worked on plymouth prompts since forever...
thus this would not be a surprise to most plymouth users (~ 5+ years
by now?!)
Doing it otherwise, will, on the contrary, impede user experience.
If you say so. I have never interacted with plymouth except to press ESC.
I'll have to give this a try.

Zbyszek
Dimitri John Ledkov
2015-01-28 15:34:34 UTC
Permalink
On 28 January 2015 at 15:31, Zbigniew Jędrzejewski-Szmek
Post by Zbigniew Jędrzejewski-Szmek
Post by Dimitri John Ledkov
On 28 January 2015 at 14:53, Zbigniew Jędrzejewski-Szmek
Post by Zbigniew Jędrzejewski-Szmek
From 104cf82ba28941e907f277a713f834ceb3d909f0 Mon Sep 17 00:00:00 2001
Date: Mon, 26 Jan 2015 16:40:52 +0100
Subject: [PATCH 06/12] Support cancellation of fsck in progress
Grab in fsckd plymouth watch key for C or c, and propagate this cancel request
to systemd-fsck which will terminate fsck.
Could we bind to ^c or if this is not possible, "three c's in three
seconds" instead? I'm worried that before you could press anything to little
effect in plymouth, and now a single key will have significant consequences.
Hm? an interactive message with key-binding is usually shown and then
plymouth reacts to such a key prompt.
This is how it has always worked on plymouth prompts since forever...
thus this would not be a surprise to most plymouth users (~ 5+ years
by now?!)
Doing it otherwise, will, on the contrary, impede user experience.
If you say so. I have never interacted with plymouth except to press ESC.
I'll have to give this a try.
Actually it's not ESC button, but rather any escape sequence will drop
into messages mode. That is shift, ctrl, alt, Fn, all of them.
Thus I don't think ^c binding is actually available to plymouth clients.
But my plymouth knowledge is rusty.
--
Regards,

Dimitri.

Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ.
Didier Roche
2015-01-29 17:44:23 UTC
Permalink
Post by Zbigniew Jędrzejewski-Szmek
From 104cf82ba28941e907f277a713f834ceb3d909f0 Mon Sep 17 00:00:00 2001
Date: Mon, 26 Jan 2015 16:40:52 +0100
Subject: [PATCH 06/12] Support cancellation of fsck in progress
Grab in fsckd plymouth watch key for C or c, and propagate this cancel request
to systemd-fsck which will terminate fsck.
Could we bind to ^c or if this is not possible, "three c's in three
seconds" instead? I'm worried that before you could press anything to little
effect in plymouth, and now a single key will have significant consequences.
I tried to have a look at libplymouth, and if I'm correct, it's not
possible to listen and get events for compose keys, so no way to get
something like Control+C. As Dimitri told, it's been quite some years we
are doing that in ubuntu, and that's the reason why we show a message to
ensure the user is aware about that key (and that's why this patch is
doing). Is it good for you this way?

Cheers,
Didier
Zbigniew Jędrzejewski-Szmek
2015-01-31 00:04:37 UTC
Permalink
Post by Didier Roche
Post by Zbigniew Jędrzejewski-Szmek
From 104cf82ba28941e907f277a713f834ceb3d909f0 Mon Sep 17 00:00:00 2001
Date: Mon, 26 Jan 2015 16:40:52 +0100
Subject: [PATCH 06/12] Support cancellation of fsck in progress
Grab in fsckd plymouth watch key for C or c, and propagate this cancel request
to systemd-fsck which will terminate fsck.
Could we bind to ^c or if this is not possible, "three c's in three
seconds" instead? I'm worried that before you could press anything to little
effect in plymouth, and now a single key will have significant consequences.
I tried to have a look at libplymouth, and if I'm correct, it's not
possible to listen and get events for compose keys, so no way to get
something like Control+C. As Dimitri told, it's been quite some
years we are doing that in ubuntu, and that's the reason why we show
a message to ensure the user is aware about that key (and that's why
this patch is doing). Is it good for you this way?
I think so. We can always improve the interface later on if it's
confusing for users.

(If plymouth forwards the key to us, we could detect "triple c within
two seconds" ourselves. But if you think that a single key is fine,
than that's fine for me.)

Zbyszek
Lennart Poettering
2015-02-02 21:02:41 UTC
Permalink
Post by Zbigniew Jędrzejewski-Szmek
From 104cf82ba28941e907f277a713f834ceb3d909f0 Mon Sep 17 00:00:00 2001
Date: Mon, 26 Jan 2015 16:40:52 +0100
Subject: [PATCH 06/12] Support cancellation of fsck in progress
Grab in fsckd plymouth watch key for C or c, and propagate this cancel request
to systemd-fsck which will terminate fsck.
Could we bind to ^c or if this is not possible, "three c's in three
seconds" instead? I'm worried that before you could press anything to little
effect in plymouth, and now a single key will have significant consequences.
I tried to have a look at libplymouth, and if I'm correct, it's not possible
to listen and get events for compose keys, so no way to get something like
Control+C. As Dimitri told, it's been quite some years we are doing that in
ubuntu, and that's the reason why we show a message to ensure the user is
aware about that key (and that's why this patch is doing). Is it good for
you this way?
Hmm, I thought plymouth was listening on the tty (and not the input
subsystem), which means it should deliver ctrl-C as ascii character
code 3, because UNIX...

Lennart
--
Lennart Poettering, Red Hat
Lennart Poettering
2015-01-28 20:25:31 UTC
Permalink
src/fsck/fsck.c | 29 +++++++++++++++++++++--------
src/fsckd/fsckd.c | 43 +++++++++++++++++++++++++++++++++++++++++++
src/fsckd/fsckd.h | 5 +++++
3 files changed, 69 insertions(+), 8 deletions(-)
diff --git a/src/fsck/fsck.c b/src/fsck/fsck.c
index f5dd546..0b42e3b 100644
--- a/src/fsck/fsck.c
+++ b/src/fsck/fsck.c
@@ -47,6 +47,8 @@
static bool arg_skip = false;
static bool arg_force = false;
static const char *arg_repair = "-a";
+static pid_t fsck_pid;
+static bool cancel_requested = false;
Please do not introduce new global variables unnecessarily. We try to
keep variables locally, and keep the global state at a minimum. An
exception is mostly the command line args, which by their nature are
global anyway...

Lennart
--
Lennart Poettering, Red Hat
Didier Roche
2015-01-29 17:44:47 UTC
Permalink
Post by Lennart Poettering
src/fsck/fsck.c | 29 +++++++++++++++++++++--------
src/fsckd/fsckd.c | 43 +++++++++++++++++++++++++++++++++++++++++++
src/fsckd/fsckd.h | 5 +++++
3 files changed, 69 insertions(+), 8 deletions(-)
diff --git a/src/fsck/fsck.c b/src/fsck/fsck.c
index f5dd546..0b42e3b 100644
--- a/src/fsck/fsck.c
+++ b/src/fsck/fsck.c
@@ -47,6 +47,8 @@
static bool arg_skip = false;
static bool arg_force = false;
static const char *arg_repair = "-a";
+static pid_t fsck_pid;
+static bool cancel_requested = false;
Please do not introduce new global variables unnecessarily. We try to
keep variables locally, and keep the global state at a minimum. An
exception is mostly the command line args, which by their nature are
global anyway...
Sure, I put fsck_pid as a function parameter. For cancel_requested, I
noticed that process_progress() was already return an int that was not
read, so assigned it to a variable and check for fsck KILL status and
not process_progress returning 0.

I didn't add checks (as there were none as of today) on
process_progress() status, but can do.

Cheers,
Didier
Didier Roche
2015-01-28 13:23:21 UTC
Permalink
Didier Roche
2015-01-28 13:23:44 UTC
Permalink
Didier Roche
2015-01-28 13:24:07 UTC
Permalink
Didier Roche
2015-01-28 13:24:32 UTC
Permalink
Zbigniew Jędrzejewski-Szmek
2015-01-28 15:00:18 UTC
Permalink
From 000f1b6ff4f5f80a2a13309590d255de6d6526ec Mon Sep 17 00:00:00 2001
Date: Mon, 26 Jan 2015 17:30:00 +0100
Subject: [PATCH 10/12] Add fsckd service and socket, retarget systemd-fsck
systemd-fsckd can be socket-activated by systemd-fsck process. Reflect that
in the different unit files.
---
Makefile.am | 3 +++
units/.gitignore | 1 +
units/systemd-fsck-root.service.in | 4 +++-
units/systemd-fsckd.service.in | 16 ++++++++++++++++
units/systemd-fsckd.socket | 15 +++++++++++++++
6 files changed, 41 insertions(+), 3 deletions(-)
create mode 100644 units/systemd-fsckd.service.in
create mode 100644 units/systemd-fsckd.socket
diff --git a/Makefile.am b/Makefile.am
index a9d61f1..1eeba4f 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -492,6 +492,7 @@ dist_systemunit_DATA = \
units/slices.target \
units/system.slice \
units/x-.slice \
+ units/systemd-fsckd.socket \
units/systemd-initctl.socket \
units/systemd-shutdownd.socket \
units/syslog.socket \
@@ -543,6 +544,7 @@ nodist_systemunit_DATA = \
units/systemd-kexec.service \
units/systemd-fsck-root.service \
+ units/systemd-fsckd.service \
units/systemd-machine-id-commit.service \
units/systemd-udevd.service \
units/systemd-udev-trigger.service \
@@ -596,6 +598,7 @@ EXTRA_DIST += \
units/user/systemd-exit.service.in \
units/systemd-fsck-root.service.in \
+ units/systemd-fsckd.service.in \
units/systemd-machine-id-commit.service.in \
units/debug-shell.service.in \
diff --git a/units/.gitignore b/units/.gitignore
index 6fdb629..26ae965 100644
--- a/units/.gitignore
+++ b/units/.gitignore
@@ -28,6 +28,7 @@
/systemd-firstboot.service
/systemd-fsck-root.service
+/systemd-fsckd.service
/systemd-machine-id-commit.service
/systemd-halt.service
/systemd-hibernate.service
diff --git a/units/systemd-fsck-root.service.in b/units/systemd-fsck-root.service.in
index 6d76578..bd3bdbc 100644
--- a/units/systemd-fsck-root.service.in
+++ b/units/systemd-fsck-root.service.in
@@ -9,12 +9,14 @@
Description=File System Check on Root Device
Documentation=man:systemd-fsck-root.service(8)
DefaultDependencies=no
+Wants=systemd-fsckd.socket
Before=local-fs.target shutdown.target
+After=systemd-fsckd.socket
ConditionPathIsReadWrite=!/
[Service]
Type=oneshot
RemainAfterExit=yes
-StandardOutput=journal+console
+StandardOutput=journal
Just remove the line completely to use the admin default (here and in
other cases...).
TimeoutSec=0
index 857e625..3ceb5f2 100644
@@ -10,12 +10,13 @@ Description=File System Check on %f
DefaultDependencies=no
BindsTo=%i.device
-After=%i.device systemd-fsck-root.service local-fs-pre.target
+Wants=systemd-fsckd.socket
+After=%i.device systemd-fsck-root.service local-fs-pre.target systemd-fsckd.socket
Before=shutdown.target
[Service]
Type=oneshot
RemainAfterExit=yes
-StandardOutput=journal+console
+StandardOutput=journal
TimeoutSec=0
diff --git a/units/systemd-fsckd.service.in b/units/systemd-fsckd.service.in
new file mode 100644
index 0000000..27c325f
--- /dev/null
+++ b/units/systemd-fsckd.service.in
@@ -0,0 +1,16 @@
+# This file is part of systemd.
+#
+# systemd is free software; you can redistribute it and/or modify it
+# under the terms of the GNU Lesser General Public License as published by
+# the Free Software Foundation; either version 2.1 of the License, or
+# (at your option) any later version.
+
+[Unit]
+Description=File System Check Daemon to report status
+DefaultDependencies=no
+Requires=systemd-fsckd.socket
+Before=shutdown.target
+
+[Service]
+StandardOutput=journal+console
diff --git a/units/systemd-fsckd.socket b/units/systemd-fsckd.socket
new file mode 100644
index 0000000..96a034a
--- /dev/null
+++ b/units/systemd-fsckd.socket
@@ -0,0 +1,15 @@
+# This file is part of systemd.
+#
+# systemd is free software; you can redistribute it and/or modify it
+# under the terms of the GNU Lesser General Public License as published by
+# the Free Software Foundation; either version 2.1 of the License, or
+# (at your option) any later version.
+
+[Unit]
+Description=fsck to fsckd communication Socket
+DefaultDependencies=no
+Before=sockets.target
Is this necessary? It is pulled in by the fsck units already, I don't think we
need to start it separately.
+
+[Socket]
+ListenStream=/run/systemd/fsckd
Zbyszek
Didier Roche
2015-01-29 17:45:04 UTC
Permalink
Post by Zbigniew Jędrzejewski-Szmek
From 000f1b6ff4f5f80a2a13309590d255de6d6526ec Mon Sep 17 00:00:00 2001
Date: Mon, 26 Jan 2015 17:30:00 +0100
Subject: [PATCH 10/12] Add fsckd service and socket, retarget systemd-fsck
systemd-fsckd can be socket-activated by systemd-fsck process. Reflect that
in the different unit files.
---
Makefile.am | 3 +++
units/.gitignore | 1 +
units/systemd-fsck-root.service.in | 4 +++-
units/systemd-fsckd.service.in | 16 ++++++++++++++++
units/systemd-fsckd.socket | 15 +++++++++++++++
6 files changed, 41 insertions(+), 3 deletions(-)
create mode 100644 units/systemd-fsckd.service.in
create mode 100644 units/systemd-fsckd.socket
diff --git a/Makefile.am b/Makefile.am
index a9d61f1..1eeba4f 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -492,6 +492,7 @@ dist_systemunit_DATA = \
units/slices.target \
units/system.slice \
units/x-.slice \
+ units/systemd-fsckd.socket \
units/systemd-initctl.socket \
units/systemd-shutdownd.socket \
units/syslog.socket \
@@ -543,6 +544,7 @@ nodist_systemunit_DATA = \
units/systemd-kexec.service \
units/systemd-fsck-root.service \
+ units/systemd-fsckd.service \
units/systemd-machine-id-commit.service \
units/systemd-udevd.service \
units/systemd-udev-trigger.service \
@@ -596,6 +598,7 @@ EXTRA_DIST += \
units/user/systemd-exit.service.in \
units/systemd-fsck-root.service.in \
+ units/systemd-fsckd.service.in \
units/systemd-machine-id-commit.service.in \
units/debug-shell.service.in \
diff --git a/units/.gitignore b/units/.gitignore
index 6fdb629..26ae965 100644
--- a/units/.gitignore
+++ b/units/.gitignore
@@ -28,6 +28,7 @@
/systemd-firstboot.service
/systemd-fsck-root.service
+/systemd-fsckd.service
/systemd-machine-id-commit.service
/systemd-halt.service
/systemd-hibernate.service
diff --git a/units/systemd-fsck-root.service.in b/units/systemd-fsck-root.service.in
index 6d76578..bd3bdbc 100644
--- a/units/systemd-fsck-root.service.in
+++ b/units/systemd-fsck-root.service.in
@@ -9,12 +9,14 @@
Description=File System Check on Root Device
Documentation=man:systemd-fsck-root.service(8)
DefaultDependencies=no
+Wants=systemd-fsckd.socket
Before=local-fs.target shutdown.target
+After=systemd-fsckd.socket
ConditionPathIsReadWrite=!/
[Service]
Type=oneshot
RemainAfterExit=yes
-StandardOutput=journal+console
+StandardOutput=journal
Just remove the line completely to use the admin default (here and in
other cases...).
TimeoutSec=0
index 857e625..3ceb5f2 100644
@@ -10,12 +10,13 @@ Description=File System Check on %f
DefaultDependencies=no
BindsTo=%i.device
-After=%i.device systemd-fsck-root.service local-fs-pre.target
+Wants=systemd-fsckd.socket
+After=%i.device systemd-fsck-root.service local-fs-pre.target systemd-fsckd.socket
Before=shutdown.target
[Service]
Type=oneshot
RemainAfterExit=yes
-StandardOutput=journal+console
+StandardOutput=journal
TimeoutSec=0
diff --git a/units/systemd-fsckd.service.in b/units/systemd-fsckd.service.in
new file mode 100644
index 0000000..27c325f
--- /dev/null
+++ b/units/systemd-fsckd.service.in
@@ -0,0 +1,16 @@
+# This file is part of systemd.
+#
+# systemd is free software; you can redistribute it and/or modify it
+# under the terms of the GNU Lesser General Public License as published by
+# the Free Software Foundation; either version 2.1 of the License, or
+# (at your option) any later version.
+
+[Unit]
+Description=File System Check Daemon to report status
+DefaultDependencies=no
+Requires=systemd-fsckd.socket
+Before=shutdown.target
+
+[Service]
+StandardOutput=journal+console
diff --git a/units/systemd-fsckd.socket b/units/systemd-fsckd.socket
new file mode 100644
index 0000000..96a034a
--- /dev/null
+++ b/units/systemd-fsckd.socket
@@ -0,0 +1,15 @@
+# This file is part of systemd.
+#
+# systemd is free software; you can redistribute it and/or modify it
+# under the terms of the GNU Lesser General Public License as published by
+# the Free Software Foundation; either version 2.1 of the License, or
+# (at your option) any later version.
+
+[Unit]
+Description=fsck to fsckd communication Socket
+DefaultDependencies=no
+Before=sockets.target
Is this necessary? It is pulled in by the fsck units already, I don't think we
need to start it separately.
This was a leftover, sorry about that. Fixed now.

Didier
Didier Roche
2015-01-28 13:24:55 UTC
Permalink
Zbigniew Jędrzejewski-Szmek
2015-01-28 15:06:50 UTC
Permalink
From 6b13d8fb248bf4176f1ad7e1d4736683462bf196 Mon Sep 17 00:00:00 2001
Date: Mon, 26 Jan 2015 17:34:59 +0100
Subject: [PATCH 11/12] Add man page and references to it.
Add man page explaining the plymouth theme protocol, usage of the daemon
as well as the socket activation part.
Adapt existing fsck man page.
---
Makefile-man.am | 12 +++
man/systemd-fsckd.service.xml | 170 +++++++++++++++++++++++++++++++++++++++++
units/systemd-fsckd.service.in | 1 +
units/systemd-fsckd.socket | 2 +-
5 files changed, 188 insertions(+), 3 deletions(-)
create mode 100644 man/systemd-fsckd.service.xml
diff --git a/Makefile-man.am b/Makefile-man.am
index 105853e..f2e13e8 100644
--- a/Makefile-man.am
+++ b/Makefile-man.am
@@ -67,6 +67,7 @@ MANPAGES += \
man/systemd-escape.1 \
man/systemd-firstboot.1 \
+ man/systemd-fsckd.service.8 \
man/systemd-fstab-generator.8 \
man/systemd-getty-generator.8 \
man/systemd-gpt-auto-generator.8 \
@@ -210,6 +211,8 @@ MANPAGES_ALIAS += \
man/systemd-firstboot.service.1 \
man/systemd-fsck-root.service.8 \
man/systemd-fsck.8 \
+ man/systemd-fsckd.8 \
+ man/systemd-fsckd.socket.8 \
man/systemd-hibernate-resume.8 \
man/systemd-hibernate.service.8 \
man/systemd-hybrid-sleep.service.8 \
@@ -323,6 +326,8 @@ man/systemd-ask-password-wall.service.8: man/systemd-ask-password-console.servic
man/systemd-firstboot.service.1: man/systemd-firstboot.1
+man/systemd-fsckd.8: man/systemd-fsckd.service.8
+man/systemd-fsckd.socket.8: man/systemd-fsckd.service.8
man/systemd-hibernate.service.8: man/systemd-suspend.service.8
man/systemd-hybrid-sleep.service.8: man/systemd-suspend.service.8
@@ -606,6 +611,12 @@ man/systemd-fsck-root.service.html: man/systemd-***@.service.html
$(html-alias)
+man/systemd-fsckd.html: man/systemd-fsckd.service.html
+ $(html-alias)
+
+man/systemd-fsckd.socket.html: man/systemd-fsckd.service.html
+ $(html-alias)
+
$(html-alias)
@@ -1732,6 +1743,7 @@ EXTRA_DIST += \
man/systemd-escape.xml \
man/systemd-firstboot.xml \
+ man/systemd-fsckd.service.xml \
man/systemd-fstab-generator.xml \
man/systemd-getty-generator.xml \
man/systemd-gpt-auto-generator.xml \
index ee66f37..d366712 100644
@@ -87,8 +87,9 @@
check, number of mounts, unclean unmount, etc.</para>
<para><filename>systemd-fsck</filename> will forward
- file system checking progress to the console. If a
- file system check fails for a service without
+ file system checking progress to
+ <filename>systemd-fsckd.service</filename>
+ socket. If a file system check fails for a service without
<option>nofail</option>, emergency mode is activated,
by isolating to
<filename>emergency.target</filename>.</para>
@@ -142,6 +143,7 @@
<para>
<citerefentry><refentrytitle>systemd</refentrytitle><manvolnum>1</manvolnum></citerefentry>,
<citerefentry><refentrytitle>fsck</refentrytitle><manvolnum>8</manvolnum></citerefentry>,
+ <citerefentry><refentrytitle>systemd-fsckd.service</refentrytitle><manvolnum>8</manvolnum></citerefentry>,
<citerefentry><refentrytitle>systemd-quotacheck.service</refentrytitle><manvolnum>8</manvolnum></citerefentry>,
<citerefentry><refentrytitle>fsck.btrfs</refentrytitle><manvolnum>8</manvolnum></citerefentry>,
<citerefentry><refentrytitle>fsck.cramfs</refentrytitle><manvolnum>8</manvolnum></citerefentry>,
diff --git a/man/systemd-fsckd.service.xml b/man/systemd-fsckd.service.xml
new file mode 100644
index 0000000..befcc45
--- /dev/null
+++ b/man/systemd-fsckd.service.xml
@@ -0,0 +1,170 @@
+<?xml version="1.0"?>
+<!--*-nxml-*-->
+<!DOCTYPE refentry PUBLIC "-//OASIS//DTD DocBook XML V4.2//EN" "http://www.oasis-open.org/docbook/xml/4.2/docbookx.dtd">
+<!--
+ This file is part of systemd.
+
+ Copyright 2015 Canonical
+
+ systemd is free software; you can redistribute it and/or modify it
+ under the terms of the GNU Lesser General Public License as published by
+ the Free Software Foundation; either version 2.1 of the License, or
+ (at your option) any later version.
+
+ systemd is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with systemd; If not, see <http://www.gnu.org/licenses/>.
+-->
+<refentry id="systemd-fsckd.service" xmlns:xi="http://www.w3.org/2001/XInclude">
Please use 2ch indentation for new man pages.
+
+ <refentryinfo>
+ <title>systemd-fsckd.service</title>
+ <productname>systemd</productname>
+
+ <authorgroup>
+ <author>
+ <contrib>Developer</contrib>
+ <firstname>Didier</firstname>
+ <surname>Roche</surname>
+ </author>
+ </authorgroup>
+ </refentryinfo>
+
+ <refmeta>
+ <refentrytitle>systemd-fsckd.service</refentrytitle>
+ <manvolnum>8</manvolnum>
+ </refmeta>
+
+ <refnamediv>
+ <refname>systemd-fsckd.service</refname>
+ <refname>systemd-fsckd.socket</refname>
+ <refname>systemd-fsckd</refname>
+ <refpurpose>File system check progress reporting</refpurpose>
+ </refnamediv>
+
+ <refsynopsisdiv>
+ <para><filename>systemd-fsckd.service</filename></para>
+ <para><filename>systemd-fsckd.socket</filename></para>
+ <para><filename>/usr/lib/systemd/systemd-fsckd</filename></para>
+ </refsynopsisdiv>
+
+ <refsect1>
+ <title>Description</title>
+
+ <para><filename>systemd-fsckd.service</filename> is a
+ service responsible for fetching file system check
receiving, not fetching
+ progress, and communicating some consolidated data
+ to console and plymouth (if running). It also handles
+ possible check cancellations.</para>
+ <para><filename>systemd-fsck-root.service</filename> or
+ progress from fsck and send their individual progress to
+ <filename>systemd-fsckd</filename>, through socket activation
+ by <filename>systemd-fsckd.socket</filename>.</para>
I think we don't need this kind of detail in the man page. It might change anyway.
+
+ <para><filename>systemd-fsckd</filename> accepts
+ <filename>systemd-fsck</filename> UNIX domain
+ sockets communication, fetches the lowest progress value of
+ all fsck running in parallel with the number of devices
+ being currently checked. It writes the result to
+ <filename>/dev/console</filename> if show status is enabled,
+ and communicates to the user translated strings to plymouth
+ ready to be used by scripted themes, not supporting i18n.
+ The compilted plymouth themes can use the raw data (see below)
+ to display their own custom messages.</para>
+
+ <para>The first time it connects to plymouth, a request
+ to grab c or C keypresses is sent, as well as a text message.
+ When the cancel key is pressed, all running fscks are terminated.
+ It will also any new fsck for the lifetime of
This sentence is missing some verb.
+ <filename>systemd-fsckd</filename>.</para>
+ </refsect1>
+
+ <refsect1>
+ <title>Protocol with plymouth</title>
+
+ <para><filename>systemd-fsckd</filename> passes the following
+ following messages to the theme via libplymouth:</para>
dup
+ <variablelist>
+ <varlistentry>
+ <listitem><para>the current number of devices
+ being checked (int)</para></listitem>
+ </varlistentry>
+ <varlistentry>
+ <listitem><para>the current minimum percentage of
+ all devices being checking (float, from 0 to 100)</para></listitem>
+ </varlistentry>
+ <varlistentry>
+ <listitem><para>a translated message ready to be displayed
+ by the plymouth theme displaying the data above. It can be overriden
+ by themes supporting i18n.</para></listitem>
+ </varlistentry>
+ </variablelist>
+ </para>
+
+ <variablelist>
+ <varlistentry>
+ <listitem><para>a translated string ready to be displayed
+ by the plymouth theme indicating that c or C can be used to cancel
+ current checks. It can be overriden (matching only
+ <literal>fsckd-cancel-msg</literal> prefix)
+ by themes supporting i18n.</para></listitem>
+ </varlistentry>
+ </variablelist>
+ </para>
This is very detailed too, but it is OK, we don't really have a good place for this
kind of documentation.
+ </refsect1>
+
+ <refsect1>
+ <title>Options</title>
+
+ <para>The following options are understood:</para>
+
+ <variablelist>
+ <xi:include href="standard-options.xml" xpointer="help" />
+ <xi:include href="standard-options.xml" xpointer="version" />
+ </variablelist>
+
+ </refsect1>
+
+ <refsect1>
+ <title>Exit status</title>
+
+ <para>On success, 0 is returned, a non-zero failure
+ code otherwise. Note that the daemon stays idle for
+ a while to accept new <filename>systemd-fsck</filename>
+ connections before exiting.</para>
+ </refsect1>
+
+ <refsect1>
+ <title>See Also</title>
+ <para>
+ <citerefentry><refentrytitle>systemd</refentrytitle><manvolnum>1</manvolnum></citerefentry>,
+ <citerefentry><refentrytitle>systemd-fsck</refentrytitle><manvolnum>8</manvolnum></citerefentry>,
+ <citerefentry><refentrytitle>fsck</refentrytitle><manvolnum>8</manvolnum></citerefentry>,
+ <citerefentry><refentrytitle>systemd-quotacheck.service</refentrytitle><manvolnum>8</manvolnum></citerefentry>,
+ <citerefentry><refentrytitle>fsck.btrfs</refentrytitle><manvolnum>8</manvolnum></citerefentry>,
+ <citerefentry><refentrytitle>fsck.cramfs</refentrytitle><manvolnum>8</manvolnum></citerefentry>,
+ <citerefentry><refentrytitle>fsck.ext4</refentrytitle><manvolnum>8</manvolnum></citerefentry>,
+ <citerefentry><refentrytitle>fsck.fat</refentrytitle><manvolnum>8</manvolnum></citerefentry>,
+ <citerefentry><refentrytitle>fsck.hfsplus</refentrytitle><manvolnum>8</manvolnum></citerefentry>,
+ <citerefentry><refentrytitle>fsck.minix</refentrytitle><manvolnum>8</manvolnum></citerefentry>,
+ <citerefentry><refentrytitle>fsck.ntfs</refentrytitle><manvolnum>8</manvolnum></citerefentry>,
+ <citerefentry><refentrytitle>fsck.xfs</refentrytitle><manvolnum>8</manvolnum></citerefentry>
+ </para>
+ </refsect1>
+
+</refentry>
diff --git a/units/systemd-fsckd.service.in b/units/systemd-fsckd.service.in
index 27c325f..9c7ed51 100644
--- a/units/systemd-fsckd.service.in
+++ b/units/systemd-fsckd.service.in
@@ -7,6 +7,7 @@
[Unit]
Description=File System Check Daemon to report status
+Documentation=man:systemd-fsckd.service(8)
DefaultDependencies=no
Requires=systemd-fsckd.socket
Before=shutdown.target
diff --git a/units/systemd-fsckd.socket b/units/systemd-fsckd.socket
index 96a034a..6f4badb 100644
--- a/units/systemd-fsckd.socket
+++ b/units/systemd-fsckd.socket
@@ -7,7 +7,7 @@
[Unit]
Description=fsck to fsckd communication Socket
DefaultDependencies=no
Before=sockets.target
Zbyszek
Didier Roche
2015-01-29 17:46:40 UTC
Permalink
Post by Zbigniew Jędrzejewski-Szmek
From 6b13d8fb248bf4176f1ad7e1d4736683462bf196 Mon Sep 17 00:00:00 2001
Date: Mon, 26 Jan 2015 17:34:59 +0100
Subject: [PATCH 11/12] Add man page and references to it.
--- /dev/null
+++ b/man/systemd-fsckd.service.xml
@@ -0,0 +1,170 @@
+<?xml version="1.0"?>
+<!--*-nxml-*-->
+<!DOCTYPE refentry PUBLIC "-//OASIS//DTD DocBook XML V4.2//EN" "http://www.oasis-open.org/docbook/xml/4.2/docbookx.dtd">
+<!--
+ This file is part of systemd.
+
+ Copyright 2015 Canonical
+
+ systemd is free software; you can redistribute it and/or modify it
+ under the terms of the GNU Lesser General Public License as published by
+ the Free Software Foundation; either version 2.1 of the License, or
+ (at your option) any later version.
+
+ systemd is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with systemd; If not, see <http://www.gnu.org/licenses/>.
+-->
+<refentry id="systemd-fsckd.service" xmlns:xi="http://www.w3.org/2001/XInclude">
Please use 2ch indentation for new man pages.
Converted to 2 characters for indentation.
Post by Zbigniew Jędrzejewski-Szmek
+ progress, and communicating some consolidated data
+ to console and plymouth (if running). It also handles
+ possible check cancellations.</para>
+ <para><filename>systemd-fsck-root.service</filename> or
+ progress from fsck and send their individual progress to
+ <filename>systemd-fsckd</filename>, through socket activation
+ by <filename>systemd-fsckd.socket</filename>.</para>
I think we don't need this kind of detail in the man page. It might change anyway.
Removed then!
Post by Zbigniew Jędrzejewski-Szmek
+ <variablelist>
+ <varlistentry>
+ <listitem><para>the current number of devices
+ being checked (int)</para></listitem>
+ </varlistentry>
+ <varlistentry>
+ <listitem><para>the current minimum percentage of
+ all devices being checking (float, from 0 to 100)</para></listitem>
+ </varlistentry>
+ <varlistentry>
+ <listitem><para>a translated message ready to be displayed
+ by the plymouth theme displaying the data above. It can be overriden
+ by themes supporting i18n.</para></listitem>
+ </varlistentry>
+ </variablelist>
+ </para>
+
+ <variablelist>
+ <varlistentry>
+ <listitem><para>a translated string ready to be displayed
+ by the plymouth theme indicating that c or C can be used to cancel
+ current checks. It can be overriden (matching only
+ <literal>fsckd-cancel-msg</literal> prefix)
+ by themes supporting i18n.</para></listitem>
+ </varlistentry>
+ </variablelist>
+ </para>
This is very detailed too, but it is OK, we don't really have a good place for this
kind of documentation.
Yeah, some API for plymouth theme authors. I didn't find a better place
(or the systemd wiki?)

Thanks again for the detailed rereading :)
Cheers,
Didier
Zbigniew Jędrzejewski-Szmek
2015-01-31 00:36:35 UTC
Permalink
Post by Didier Roche
Post by Zbigniew Jędrzejewski-Szmek
This is very detailed too, but it is OK, we don't really have a good place for this
kind of documentation.
Yeah, some API for plymouth theme authors. I didn't find a better
place (or the systemd wiki?)
The wiki tends to get outdated... This text is not too long and it'll
be easier to find and maintain here.

Zbyszek
Didier Roche
2015-01-28 13:25:17 UTC
Permalink
Zbigniew Jędrzejewski-Szmek
2015-01-28 15:08:26 UTC
Permalink
From aefe0667ed62d5d7e17193c0f5ae302ed57e4727 Mon Sep 17 00:00:00 2001
Date: Mon, 26 Jan 2015 17:46:36 +0100
Subject: [PATCH 12/12] Add mock fsck process
---
test/mocks/fsck | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
create mode 100755 test/mocks/fsck
diff --git a/test/mocks/fsck b/test/mocks/fsck
new file mode 100755
index 0000000..77b50d7
--- /dev/null
+++ b/test/mocks/fsck
@@ -0,0 +1,27 @@
+#!/bin/bash
I think you can change this to /bin/sh, I don't see any bashisms.
+fd=0
+
+OPTIND=1
+while getopts "C:aTlM" opt; do
+ case "$opt" in
+ C)
+ fd=$OPTARG
+ ;;
+ \?);;
+ esac
+done
+
+shift "$((OPTIND-1))"
+device=$1
+
+echo "Running fake fsck on $device"
+
+declare -a maxpass=(30 5 2 30 60)
+
+for pass in {1..5}; do
+ maxprogress=${maxpass[$((pass-1))]}
+ for (( current=0; current<=${maxprogress}; current++)); do
+ echo "$pass $current $maxprogress $device">&$fd
+ sleep 0.1
+ done
+done
--
2.1.4
_______________________________________________
systemd-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Martin Pitt
2015-01-28 16:13:04 UTC
Permalink
Post by Zbigniew Jędrzejewski-Szmek
+#!/bin/bash
I think you can change this to /bin/sh, I don't see any bashisms.
+declare -a maxpass=(30 5 2 30 60)
I do :-) POSIX shell doesn't have arrays AFAIK, and declare -a
definitively doesn't work in e. g. dash.
Post by Zbigniew Jędrzejewski-Szmek
+for pass in {1..5}; do
+ maxprogress=${maxpass[$((pass-1))]}
That said, if using POSIX shell is a concern (probably not for this
little test dummy), this could certainly be rewritten to something
like

pass=0
for maxprogress in 30 5 2 30 60; do
pass=$((pass+1))

which isn't really more complicated and avoids arrays.

Martin
--
Martin Pitt | http://www.piware.de
Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)
Didier Roche
2015-01-29 17:49:00 UTC
Permalink
Post by Martin Pitt
Post by Zbigniew Jędrzejewski-Szmek
+#!/bin/bash
I think you can change this to /bin/sh, I don't see any bashisms.
+declare -a maxpass=(30 5 2 30 60)
I do :-) POSIX shell doesn't have arrays AFAIK, and declare -a
definitively doesn't work in e. g. dash.
Post by Zbigniew Jędrzejewski-Szmek
+for pass in {1..5}; do
+ maxprogress=${maxpass[$((pass-1))]}
That said, if using POSIX shell is a concern (probably not for this
little test dummy), this could certainly be rewritten to something
like
pass=0
for maxprogress in 30 5 2 30 60; do
pass=$((pass+1))
which isn't really more complicated and avoids arrays.
Yeah, there are some bashims. If getting POSIX shell is important for
the mock, I'm happy to do the necessary changes.
Didier
Didier Roche
2015-02-04 16:05:50 UTC
Permalink
Hey,

I rewrote a version of this patch including the feedback on the list. As
per IRC discussion, (and after giving up the busy loop for a rewrite
with epool), I did rebase it again on sd_event.

I'm only proposing there up for review the 2 first patches (without
plymouth communication, cancel support, i18n, man pages and the service
and socket) so that I don't have to rebase all other 10 patches on a
moving ground.

I'm opened to any feedback and fixes to do :)
Cheers,
Didier
Lennart Poettering
2015-02-04 16:10:02 UTC
Permalink
Hey,
I rewrote a version of this patch including the feedback on the list. As per
IRC discussion, (and after giving up the busy loop for a rewrite with
epool), I did rebase it again on sd_event.
I'm only proposing there up for review the 2 first patches (without plymouth
communication, cancel support, i18n, man pages and the service and socket)
so that I don't have to rebase all other 10 patches on a moving
ground.
Tom just added support for installing timer events with a NULL
callback, that trigger event loop exit. I kinda prefer that solution
over a new call sd_event_loop() with timeout.

sd_event_add_time(event, NULL, CLOCK_MONOTONIC, now(CLOCK_MONOTONIC) + 5 * USEC_PER_SEC, NULL, NULL);

That line should be enough to mak an even loop exit after 5s...

Lennart
--
Lennart Poettering, Red Hat
Didier Roche
2015-02-04 16:40:25 UTC
Permalink
Post by Lennart Poettering
Hey,
I rewrote a version of this patch including the feedback on the list. As per
IRC discussion, (and after giving up the busy loop for a rewrite with
epool), I did rebase it again on sd_event.
I'm only proposing there up for review the 2 first patches (without plymouth
communication, cancel support, i18n, man pages and the service and socket)
so that I don't have to rebase all other 10 patches on a moving
ground.
Tom just added support for installing timer events with a NULL
callback, that trigger event loop exit. I kinda prefer that solution
over a new call sd_event_loop() with timeout.
sd_event_add_time(event, NULL, CLOCK_MONOTONIC, now(CLOCK_MONOTONIC) + 5 * USEC_PER_SEC, NULL, NULL);
So, it means that I need to reset it after any received activity, is
that ok? (as this will be really frequent as each clients in parallel
can send a message each 50ms). The goal is to have a global "inactivity"
timeout.

I didn't see a way to cancel this event source though?
Didier
Post by Lennart Poettering
That line should be enough to mak an even loop exit after 5s...
Lennart
Lennart Poettering
2015-02-04 17:20:55 UTC
Permalink
Post by Lennart Poettering
Hey,
I rewrote a version of this patch including the feedback on the list. As per
IRC discussion, (and after giving up the busy loop for a rewrite with
epool), I did rebase it again on sd_event.
I'm only proposing there up for review the 2 first patches (without plymouth
communication, cancel support, i18n, man pages and the service and socket)
so that I don't have to rebase all other 10 patches on a moving
ground.
Tom just added support for installing timer events with a NULL
callback, that trigger event loop exit. I kinda prefer that solution
over a new call sd_event_loop() with timeout.
sd_event_add_time(event, NULL, CLOCK_MONOTONIC, now(CLOCK_MONOTONIC) + 5 * USEC_PER_SEC, NULL, NULL);
So, it means that I need to reset it after any received activity, is that
ok? (as this will be really frequent as each clients in parallel can send a
message each 50ms). The goal is to have a global "inactivity" timeout.
I didn't see a way to cancel this event source though?
Oh, I see, you actually want a real idle logic, not just a normal
timer.

So far, for daemons like timedated, localed and so on, we are using an
idle logic that is implemented in bus_event_loop_with_idle() in
src/libsystemd/sd-bus/bus-util.c. It does considerably more than what
you need (since it contains all the magic to racefully do exit-on-idle
for bus services so that no bus messages are lost).

I think the best would be to take inspiration from that code, isolate
there basic minimum out of it, without all the dbus logic, and then
stick that in your C file.

We can generalize such exit-on-idle logic one day, somewhere between
sd-bus and sd-event, but that requires considerabe design work, so
that we find a generic solution that works for you and also covers
this dbus case without hacks. For now it's hence better if you just
take inspiration from bus_event_loop_with_idle(), drop all the
bus-specific bits, and stick it in your .c code...

Lennart
--
Lennart Poettering, Red Hat
Didier Roche
2015-02-05 08:59:28 UTC
Permalink
Post by Lennart Poettering
Post by Lennart Poettering
Hey,
I rewrote a version of this patch including the feedback on the list. As per
IRC discussion, (and after giving up the busy loop for a rewrite with
epool), I did rebase it again on sd_event.
I'm only proposing there up for review the 2 first patches (without plymouth
communication, cancel support, i18n, man pages and the service and socket)
so that I don't have to rebase all other 10 patches on a moving
ground.
Tom just added support for installing timer events with a NULL
callback, that trigger event loop exit. I kinda prefer that solution
over a new call sd_event_loop() with timeout.
sd_event_add_time(event, NULL, CLOCK_MONOTONIC, now(CLOCK_MONOTONIC) + 5 * USEC_PER_SEC, NULL, NULL);
So, it means that I need to reset it after any received activity, is that
ok? (as this will be really frequent as each clients in parallel can send a
message each 50ms). The goal is to have a global "inactivity" timeout.
I didn't see a way to cancel this event source though?
Oh, I see, you actually want a real idle logic, not just a normal
timer.
So far, for daemons like timedated, localed and so on, we are using an
idle logic that is implemented in bus_event_loop_with_idle() in
src/libsystemd/sd-bus/bus-util.c. It does considerably more than what
you need (since it contains all the magic to racefully do exit-on-idle
for bus services so that no bus messages are lost).
I think the best would be to take inspiration from that code, isolate
there basic minimum out of it, without all the dbus logic, and then
stick that in your C file.
We can generalize such exit-on-idle logic one day, somewhere between
sd-bus and sd-event, but that requires considerabe design work, so
that we find a generic solution that works for you and also covers
this dbus case without hacks. For now it's hence better if you just
take inspiration from bus_event_loop_with_idle(), drop all the
bus-specific bits, and stick it in your .c code...
Making sense.

Done and fixed. Thanks a lot
Cheers,
Didier

Didier Roche
2015-02-04 16:06:45 UTC
Permalink
Zbigniew Jędrzejewski-Szmek
2015-02-04 16:24:58 UTC
Permalink
+typedef struct Clients {
+ struct Manager *manager;
+ int fd;
+ dev_t devnum;
+ size_t cur;
+ size_t max;
+ int pass;
+ double percent;
+ size_t buflen;
+
+ LIST_FIELDS(struct Clients, clients);
+} Clients;
Would be better to call this Client.
+typedef struct Manager {
+ sd_event *event;
+ Clients *clients;
But still 'Client *clients' here.
+ if ((fabs(current_percent - m->percent) > 0.01) || (current_numdevices != m->numdevices)) {
Again, too much parens.
+ client = malloc(sizeof(Clients));
new0(Client, 1)

(Unless you initialize all fields, it's better to zero out the structure.)
+ if (!client)
+ return log_oom();
+ client->fd = new_client_fd;
+ client->manager = m;
+ LIST_PREPEND(clients, m->clients, client);
+ r = sd_event_add_io(m->event, NULL, client->fd, EPOLLIN, progress_handler, client);
+ if (r < 0) {
+ remove_client(&(m->clients), client);
+ return r;
+ }
I think you can do this in opposite order, and then the clean-up will
not be necessary.
+ m = new0(Manager, 1);
+ if (!m)
+ return -ENOMEM;
+
+ r = sd_event_default(&m->event);
+ if (r < 0)
+ return r;
+
+ m->clients = NULL;
+ m->connection_fd = fd;
+
+ m->console = fopen("/dev/console", "we");
+ if (!m->console)
+ return log_error_errno(errno, "Can't connect to /dev/console: %m");
+ m->percent = 100;
+ m->numdevices = 0;
+ m->clear = 0;
Since structure is initialized to zero, there's no need to set stuff to 0 again.
+typedef struct FsckProgress {
+ dev_t devnum;
+ unsigned long cur;
+ unsigned long max;
+ int pass;
+} FsckProgress;
I think I asked about that before, but not 100% sure: shouldn't those be size_t?
If they are disk offsets, than long is not enough.

Zbyszek
Didier Roche
2015-02-04 16:50:29 UTC
Permalink
Le 04/02/2015 17:24, Zbigniew Jędrzejewski-Szmek a écrit :

Thanks again for the quick review! Fixed if not commented (sorry, some
issues were back due to the refactoring).
Post by Zbigniew Jędrzejewski-Szmek
+typedef struct Clients {
+ struct Manager *manager;
+ int fd;
+ dev_t devnum;
+ size_t cur;
+ size_t max;
+ int pass;
+ double percent;
+ size_t buflen;
+
+ LIST_FIELDS(struct Clients, clients);
+} Clients;
Would be better to call this Client.
+typedef struct Manager {
+ sd_event *event;
+ Clients *clients;
But still 'Client *clients' here.
Right, I can't decide (due to the list) what's the best one, what do you
think?
Post by Zbigniew Jędrzejewski-Szmek
+ if (!client)
+ return log_oom();
+ client->fd = new_client_fd;
+ client->manager = m;
+ LIST_PREPEND(clients, m->clients, client);
+ r = sd_event_add_io(m->event, NULL, client->fd, EPOLLIN, progress_handler, client);
+ if (r < 0) {
+ remove_client(&(m->clients), client);
+ return r;
+ }
I think you can do this in opposite order, and then the clean-up will
not be necessary.
I need to delete the client item and close the new fd (which is part of
remove_client()), and I can't assign the add_io event before the client
instantiated. So, it will be another free() + safe_close() rather then
remove_client(). Does it makes sense still to add this?
Post by Zbigniew Jędrzejewski-Szmek
+typedef struct FsckProgress {
+ dev_t devnum;
+ unsigned long cur;
+ unsigned long max;
+ int pass;
+} FsckProgress;
I think I asked about that before, but not 100% sure: shouldn't those be size_t?
If they are disk offsets, than long is not enough.
Sorry, I only changed it in fsckd.c and fsck.c, done in the passing
struct now.

Cheers,
Didier
Zbigniew Jędrzejewski-Szmek
2015-02-04 17:04:43 UTC
Permalink
Post by Didier Roche
Thanks again for the quick review! Fixed if not commented (sorry,
some issues were back due to the refactoring).
Post by Zbigniew Jędrzejewski-Szmek
+typedef struct Clients {
+ struct Manager *manager;
+ int fd;
+ dev_t devnum;
+ size_t cur;
+ size_t max;
+ int pass;
+ double percent;
+ size_t buflen;
+
+ LIST_FIELDS(struct Clients, clients);
+} Clients;
Would be better to call this Client.
+typedef struct Manager {
+ sd_event *event;
+ Clients *clients;
But still 'Client *clients' here.
Right, I can't decide (due to the list) what's the best one, what do
you think?
We use singular in other cases like this.
Post by Didier Roche
Post by Zbigniew Jędrzejewski-Szmek
+ if (!client)
+ return log_oom();
+ client->fd = new_client_fd;
+ client->manager = m;
+ LIST_PREPEND(clients, m->clients, client);
+ r = sd_event_add_io(m->event, NULL, client->fd, EPOLLIN, progress_handler, client);
+ if (r < 0) {
+ remove_client(&(m->clients), client);
+ return r;
+ }
I think you can do this in opposite order, and then the clean-up will
not be necessary.
I need to delete the client item and close the new fd (which is part
of remove_client()), and I can't assign the add_io event before the
client instantiated. So, it will be another free() + safe_close()
rather then remove_client(). Does it makes sense still to add this?
I guess no.

Zbyszek
Continue reading on narkive:
Loading...