[systemd-devel] [PATCH 3/9] Connect to plymouth and support cancellation of in, progress fsck
Zbigniew Jędrzejewski-Szmek
zbyszek at in.waw.pl
Tue Feb 17 15:30:52 PST 2015
On Tue, Feb 17, 2015 at 05:26:05PM +0100, Didier Roche wrote:
> Le 14/02/2015 17:38, Zbigniew Jędrzejewski-Szmek a écrit :
> >On Thu, Feb 05, 2015 at 06:09:24PM +0100, Didier Roche wrote:
> >> From ec3b3d2cd4b0097f9fafa6c3f0f400e06292e21c Mon Sep 17 00:00:00 2001
> >>From: Didier Roche <didrocks at ubuntu.com>
> >>Date: Thu, 5 Feb 2015 17:08:18 +0100
> >>Subject: [PATCH 3/9] Connect to plymouth and support cancellation of in
> >> progress fsck
> >>
> >>Try to connect and send to plymouth (if running) some checked report progress,
> >>using direct plymouth protocole.
> >>
> >>Update message is the following:
> >>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
> >> checked (float, from 0 to 100)
> >>* 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.
> >>
> >>Grab in fsckd plymouth watch key Control+C, and propagate this cancel request
> >>to systemd-fsck which will terminate fsck.
> >>
> >>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 Control+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 | 33 +++++++++----
> >> src/fsckd/fsckd.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >> src/fsckd/fsckd.h | 5 ++
> >> 3 files changed, 173 insertions(+), 10 deletions(-)
> >>
> >>diff --git a/src/fsck/fsck.c b/src/fsck/fsck.c
> >>index d5aaf9e..1f5a3de 100644
> >>--- a/src/fsck/fsck.c
> >>+++ b/src/fsck/fsck.c
> >>@@ -132,7 +132,7 @@ static void test_files(void) {
> >> }
> >>-static int process_progress(int fd, dev_t device_num) {
> >>+static int process_progress(int fd, pid_t fsck_pid, dev_t device_num) {
> >> _cleanup_fclose_ FILE *f = NULL;
> >> usec_t last = 0;
> >> _cleanup_close_ int fsckd_fd = -1;
> >>@@ -159,11 +159,13 @@ static int process_progress(int fd, dev_t device_num) {
> >> while (!feof(f)) {
> >> int pass;
> >>+ size_t buflen;
> >> size_t cur, max;
> >>- ssize_t n;
> >>+ ssize_t r;
> >> usec_t t;
> >> _cleanup_free_ char *device = NULL;
> >> FsckProgress progress;
> >>+ FsckdMessage fsckd_message;
> >> if (fscanf(f, "%i %lu %lu %ms", &pass, &cur, &max, &device) != 4)
> >> break;
> >>@@ -181,9 +183,19 @@ static int process_progress(int fd, dev_t device_num) {
> >> progress.max = max;
> >> progress.pass = pass;
> >>- n = send(fsckd_fd, &progress, sizeof(FsckProgress), 0);
> >>- if (n < 0 || (size_t) n < sizeof(FsckProgress))
> >>+ r = send(fsckd_fd, &progress, sizeof(FsckProgress), 0);
> >>+ if (r < 0 || (size_t) r < sizeof(FsckProgress))
> >> log_warning_errno(errno, "Cannot communicate fsck progress to fsckd: %m");
> >>+
> >>+ /* get fsckd requests, only read when we have coherent size data */
> >>+ r = ioctl(fsckd_fd, FIONREAD, &buflen);
> >>+ if (r == 0 && (size_t) buflen == sizeof(FsckdMessage)) {
> >Shoudlnt' this be >= ? If two messages are queued, buflen will be
> >bigger then one message, and we'll never read it.
>
> Indeed, changed it.
> >
> >>+ r = recv(fsckd_fd, &fsckd_message, sizeof(FsckdMessage), 0);
> >>+ if (r > 0 && fsckd_message.cancel) {
> >>+ log_warning("Request to cancel fsck from fsckd");
> >log_notice or log_info. Actually log_info I think, since this is a
> >legitimate user request.
> Done.
> >
> >>+
> >>+ n = send(current->fd, &cancel_msg, sizeof(FsckdMessage), 0);
> >>+ if (n < 0 || (size_t) n < sizeof(FsckdMessage))
> >>+ return log_warning_errno(n, "Cannot send cancel to fsck on (%u, %u): %m", major(current->devnum), minor(current->devnum));
> >line wrap please.
>
> Done. Btw, I was wondering if there was any kind of contributor rule
> like a style guideline in systemd? I probably just missed it.
CODING_STYLE.
There's a bit of a inconsitency about good line length limits, but
this one wraps two times in my window which I consider too much :)
> >>+
> >>+ if (!m->plymouth_cancel_sent) {
> >>+ /* indicate to plymouth that we listen to Ctrl+C */
> >>+ r = loop_write(m->plymouth_fd, PLYMOUTH_REQUEST_KEY, sizeof(PLYMOUTH_REQUEST_KEY), true);
> >>+ if (r < 0)
> >>+ return log_warning_errno(errno, "Can't send to plymouth cancel key: %m");
> >>+ m->plymouth_cancel_sent = true;
> >>+ plymouth_cancel_message = strappenda("fsckd-cancel-msg:", "Press Ctrl+C to cancel all checks in progress");
> >...all filesystem checks...
>
> done (will repost the i18n patch + po ones as they are impacted by
> that change)
> >
> >>+ r = send_message_plymouth_socket(m->plymouth_fd, plymouth_cancel_message, false);
> >>+ if (r < 0)
> >>+ log_warning_errno(r, "Can't send cancel user message to plymouth: %m");
> >>+ } else if (m->numdevices == 0) {
> >>+ m->plymouth_cancel_sent = false;
> >>+ r = send_message_plymouth_socket(m->plymouth_fd, "", false);
> >>+ if (r < 0)
> >>+ log_warning_errno(r, "Can't clear cancel user message to plymouth: %m");
> >Not *that* important, but those two log_warning_errnos are poorly worded.
> >If I was a user, I'd be a bit worried if I saw that somebody tried to
> >cancel me, even if they failed ;)
> Ahah, good point! Slightly reworded.
> > Also "clear ... to".
> isn't what I used above? "clear … to"
Well, yes, but still this sentence doesn't make sense.
> >>+
> >>+ r = send_message_plymouth_socket(m->plymouth_fd, message, true);
> >>+ if (r < 0)
> >>+ return log_warning_errno(errno, "Couldn't send %s to plymouth: %m", message);
> >\"%s\" would be better, otherwise this can be hard to parse.
> >Can the message contain non-utf-8 data?
> The po files state charset=UTF-8, but if one language tweak that, I
> guess that could happen. In practice, that didn't occur in ubuntu
> from what I know when sent to plymouth.
Hm, I guess it's good enough for now. This is essentially
trusted data.
> >> typedef struct FsckProgress {
> >>@@ -32,3 +33,7 @@ typedef struct FsckProgress {
> >> size_t max;
> >> int pass;
> >> } FsckProgress;
> >>+
> >>+typedef struct FsckdMessage {
> >>+ bool cancel;
> >>+} FsckdMessage;
> >bool as the mechanism for data passing is not that good. Depending on
> >the architecture, it can have different sizes, iirc. (I can imaging
> >that for whatever reason user ends up runnning 32bit fsckd with 64bit
> >plymouth. Not likely, but better not to have to think about this at
> >all). Maybe you could make it uint8_t, which would have the advantage
> >of being trivially endianness independent.
>
> Sure, doing that change.
>
> Not that the FsckdMessage is only sent to systemd-fsck, not
> plymouth, so it's only systemd inter-services communication. I do
> hope that a user won't install 2 versions of systemd compiled for
> different archs and one opens the socket with a 32 bits version, the
> other (64 bits) write to it :)
> Anyway, good advice, and changed to an uint8_t.
> >
> >Zbyszek
>
> From e6400709f603cde406b8d3b605ca0e1dfd10ba3c Mon Sep 17 00:00:00 2001
> From: Didier Roche <didrocks at ubuntu.com>
> Date: Thu, 5 Feb 2015 17:08:18 +0100
> Subject: [PATCH 3/9] Connect to plymouth and support cancellation of in
> progress fsck
>
> Try to connect and send to plymouth (if running) some checked report progress,
> using direct plymouth protocole.
>
> Update message is the following:
> 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
> checked (float, from 0 to 100)
> * 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.
>
> Grab in fsckd plymouth watch key Control+C, and propagate this cancel request
> to systemd-fsck which will terminate fsck.
>
> 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 Control+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 | 33 ++++++++----
> src/fsckd/fsckd.c | 146 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> src/fsckd/fsckd.h | 4 ++
> 3 files changed, 173 insertions(+), 10 deletions(-)
>
> diff --git a/src/fsck/fsck.c b/src/fsck/fsck.c
> index c7461d2..efd6b5b 100644
> --- a/src/fsck/fsck.c
> +++ b/src/fsck/fsck.c
> @@ -132,7 +132,7 @@ static void test_files(void) {
>
> }
>
> -static int process_progress(int fd, dev_t device_num) {
> +static int process_progress(int fd, pid_t fsck_pid, dev_t device_num) {
> _cleanup_fclose_ FILE *f = NULL;
> usec_t last = 0;
> _cleanup_close_ int fsckd_fd = -1;
> @@ -153,11 +153,13 @@ static int process_progress(int fd, dev_t device_num) {
>
> while (!feof(f)) {
> int pass;
> + size_t buflen;
> size_t cur, max;
> - ssize_t n;
> + ssize_t r;
> usec_t t;
> _cleanup_free_ char *device = NULL;
> FsckProgress progress;
> + FsckdMessage fsckd_message;
>
> if (fscanf(f, "%i %lu %lu %ms", &pass, &cur, &max, &device) != 4)
> break;
> @@ -175,9 +177,19 @@ static int process_progress(int fd, dev_t device_num) {
> progress.max = max;
> progress.pass = pass;
>
> - n = send(fsckd_fd, &progress, sizeof(FsckProgress), 0);
> - if (n < 0 || (size_t) n < sizeof(FsckProgress))
> + r = send(fsckd_fd, &progress, sizeof(FsckProgress), 0);
> + if (r < 0 || (size_t) r < sizeof(FsckProgress))
> log_warning_errno(errno, "Cannot communicate fsck progress to fsckd: %m");
> +
> + /* get fsckd requests, only read when we have coherent size data */
> + r = ioctl(fsckd_fd, FIONREAD, &buflen);
> + if (r == 0 && (size_t) buflen >= sizeof(FsckdMessage)) {
> + r = recv(fsckd_fd, &fsckd_message, sizeof(FsckdMessage), 0);
> + if (r > 0 && fsckd_message.cancel == 1) {
> + log_info("Request to cancel fsck from fsckd");
> + kill(fsck_pid, SIGTERM);
> + }
> + }
> }
>
> return 0;
> @@ -187,6 +199,7 @@ int main(int argc, char *argv[]) {
> const char *cmdline[9];
> int i = 0, r = EXIT_FAILURE, q;
> pid_t pid;
> + int progress_rc;
> siginfo_t status;
> _cleanup_udev_unref_ struct udev *udev = NULL;
> _cleanup_udev_device_unref_ struct udev_device *udev_device = NULL;
> @@ -329,7 +342,7 @@ int main(int argc, char *argv[]) {
> progress_pipe[1] = safe_close(progress_pipe[1]);
>
> if (progress_pipe[0] >= 0) {
> - process_progress(progress_pipe[0], st.st_rdev);
> + progress_rc = process_progress(progress_pipe[0], pid, st.st_rdev);
> progress_pipe[0] = -1;
> }
>
> @@ -339,13 +352,14 @@ int main(int argc, char *argv[]) {
> goto finish;
> }
>
> - if (status.si_code != CLD_EXITED || (status.si_status & ~1)) {
> + if (status.si_code != CLD_EXITED || (status.si_status & ~1) || progress_rc != 0) {
>
> - if (status.si_code == CLD_KILLED || status.si_code == CLD_DUMPED)
> + /* cancel will kill fsck (but process_progress returns 0) */
> + if ((progress_rc != 0 && 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 (progress_rc != 0)
> log_error("fsck failed due to unknown reason.");
>
> if (status.si_code == CLD_EXITED && (status.si_status & 2) && root_directory)
> @@ -356,7 +370,8 @@ int main(int argc, char *argv[]) {
> start_target(SPECIAL_EMERGENCY_TARGET);
> else {
> r = EXIT_SUCCESS;
> - log_warning("Ignoring error.");
> + if (progress_rc != 0)
> + log_warning("Ignoring error.");
> }
>
> } else
> diff --git a/src/fsckd/fsckd.c b/src/fsckd/fsckd.c
> index 6b2eeb0..30c4f28 100644
> --- a/src/fsckd/fsckd.c
> +++ b/src/fsckd/fsckd.c
> @@ -34,6 +34,7 @@
> #include <unistd.h>
>
> #include "build.h"
> +#include "def.h"
> #include "event-util.h"
> #include "fsckd.h"
> #include "log.h"
> @@ -44,6 +45,7 @@
> #include "util.h"
>
> #define IDLE_TIME_SECONDS 30
> +#define PLYMOUTH_REQUEST_KEY "K"
>
> struct Manager;
>
> @@ -56,6 +58,7 @@ typedef struct Client {
> int pass;
> double percent;
> size_t buflen;
> + bool cancelled;
>
> LIST_FIELDS(struct Client, clients);
> } Client;
> @@ -68,8 +71,13 @@ typedef struct Manager {
> FILE *console;
> double percent;
> int numdevices;
> + int plymouth_fd;
> + bool plymouth_cancel_sent;
> + bool cancel_requested;
> } Manager;
>
> +static int connect_plymouth(Manager *m);
> +static int update_global_progress(Manager *m);
> static void manager_free(Manager *m);
> DEFINE_TRIVIAL_CLEANUP_FUNC(Manager*, manager_free);
> #define _cleanup_manager_free_ _cleanup_(manager_freep)
> @@ -92,6 +100,19 @@ static double compute_percent(int pass, size_t cur, size_t max) {
> (double) cur / max;
> }
>
> +static int request_cancel_client(Client *current) {
> + FsckdMessage cancel_msg;
> + ssize_t n;
> + cancel_msg.cancel = 1;
> +
> + n = send(current->fd, &cancel_msg, sizeof(FsckdMessage), 0);
> + if (n < 0 || (size_t) n < sizeof(FsckdMessage))
> + return log_warning_errno(n, "Cannot send cancel to fsck on (%u, %u): %m",
> + major(current->devnum), minor(current->devnum));
> + else
> + current->cancelled = true;
> + return 0;
> +}
>
> static void remove_client(Client **first, Client *item) {
> LIST_REMOVE(clients, *first, item);
> @@ -99,10 +120,91 @@ static void remove_client(Client **first, Client *item) {
> free(item);
> }
>
> +static void on_plymouth_disconnect(Manager *m) {
> + safe_close(m->plymouth_fd);
> + m->plymouth_fd = -1;
> + m->plymouth_cancel_sent = false;
> +}
> +
> +static int plymouth_feedback_handler(sd_event_source *s, int fd, uint32_t revents, void *userdata) {
> + Manager *m = userdata;
> + Client *current;
> + char buffer[6];
> + int r;
> +
> + assert(m);
> +
> + r = read(m->plymouth_fd, buffer, sizeof(buffer));
> + if (r <= 0)
> + on_plymouth_disconnect(m);
> + else {
> + if (buffer[0] == '\15')
> + log_error("Message update to plymouth wasn't delivered successfully");
> +
> + /* the only answer support type we requested is a key interruption */
> + if (buffer[0] == '\2' && buffer[5] == 'c') {
> + m->cancel_requested = true;
> + /* cancel all connected clients */
> + LIST_FOREACH(clients, current, m->clients)
> + request_cancel_client(current);
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int send_message_plymouth_socket(int plymouth_fd, const char *message, bool update) {
> + _cleanup_free_ char *packet = NULL;
> + int r, n;
> + char mode = 'M';
> +
> + if (update)
> + mode = 'U';
> +
> + if (asprintf(&packet, "%c\002%c%s%n", mode, (int) (strlen(message) + 1), message, &n) < 0)
> + return log_oom();
> + r = loop_write(plymouth_fd, packet, n + 1, true);
> + return r;
> +}
> +
> +
> +static int send_message_plymouth(Manager *m, const char *message) {
> + int r;
> + const char *plymouth_cancel_message = NULL;
> +
> + r = connect_plymouth(m);
> + if (r < 0)
> + return r;
> +
> + if (!m->plymouth_cancel_sent) {
> + /* indicate to plymouth that we listen to Ctrl+C */
> + r = loop_write(m->plymouth_fd, PLYMOUTH_REQUEST_KEY, sizeof(PLYMOUTH_REQUEST_KEY), true);
> + if (r < 0)
> + return log_warning_errno(errno, "Can't send to plymouth cancel key: %m");
> + m->plymouth_cancel_sent = true;
> + plymouth_cancel_message = strappenda("fsckd-cancel-msg:", "Press Ctrl+C to cancel all filesystem checks in progress");
> + r = send_message_plymouth_socket(m->plymouth_fd, plymouth_cancel_message, false);
> + if (r < 0)
> + log_warning_errno(r, "Can't send filesystem cancel message to plymouth: %m");
> + } else if (m->numdevices == 0) {
> + m->plymouth_cancel_sent = false;
> + r = send_message_plymouth_socket(m->plymouth_fd, "", false);
> + if (r < 0)
> + log_warning_errno(r, "Can't clear filesystem cancel message to plymouth: %m");
> + }
> +
> + r = send_message_plymouth_socket(m->plymouth_fd, message, true);
> + if (r < 0)
> + return log_warning_errno(errno, "Couldn't send \"%s\" to plymouth: %m", message);
> +
> + return 0;
> +}
> +
> static int update_global_progress(Manager *m) {
> Client *current = NULL;
> _cleanup_free_ char *console_message = NULL;
> - int current_numdevices = 0, l = 0;
> + _cleanup_free_ char *fsck_message = NULL;
> + int current_numdevices = 0, l = 0, r;
> double current_percent = 100;
>
> /* get the overall percentage */
> @@ -123,6 +225,8 @@ static int update_global_progress(Manager *m) {
> if (asprintf(&console_message, "Checking in progress on %d disks (%3.1f%% complete)",
> m->numdevices, m->percent) < 0)
> return -ENOMEM;
> + if (asprintf(&fsck_message, "fsckd:%d:%3.1f:%s", m->numdevices, m->percent, console_message) < 0)
> + return -ENOMEM;
>
> /* write to console */
> if (m->console) {
> @@ -130,12 +234,41 @@ static int update_global_progress(Manager *m) {
> fflush(m->console);
> }
>
> + /* try to connect to plymouth and send message */
> + r = send_message_plymouth(m, fsck_message);
> + if (r < 0)
> + log_debug("Couldn't send message to plymouth");
> +
> if (l > m->clear)
> m->clear = l;
> }
> return 0;
> }
>
> +static int connect_plymouth(Manager *m) {
> + union sockaddr_union sa = PLYMOUTH_SOCKET;
> + int r;
> +
> + /* try to connect or reconnect if sending a message */
> + if (m->plymouth_fd <= 0) {
> + m->plymouth_fd = socket(AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0);
> + if (m->plymouth_fd < 0) {
> + return log_warning_errno(errno, "Connection to plymouth socket failed: %m");
> + }
> + if (connect(m->plymouth_fd, &sa.sa, offsetof(struct sockaddr_un, sun_path) + 1 + strlen(sa.un.sun_path+1)) < 0) {
> + on_plymouth_disconnect(m);
> + return log_warning_errno(errno, "Couldn't connect to plymouth: %m");
> + }
> + r = sd_event_add_io(m->event, NULL, m->plymouth_fd, EPOLLIN, plymouth_feedback_handler, m);
> + if (r < 0) {
> + on_plymouth_disconnect(m);
> + return log_warning_errno(r, "Can't listen to plymouth socket: %m");
> + }
> + }
> +
> + return 0;
> +}
> +
> static int progress_handler(sd_event_source *s, int fd, uint32_t revents, void *userdata) {
> Client *client = userdata;
> Manager *m = NULL;
> @@ -146,6 +279,12 @@ static int progress_handler(sd_event_source *s, int fd, uint32_t revents, void *
> assert(client);
> m = client->manager;
>
> + /* check first if we need to cancel this client */
> + if (m->cancel_requested) {
> + if (!client->cancelled)
> + request_cancel_client(client);
> + }
> +
> /* ensure we have enough data to read */
> r = ioctl(fd, FIONREAD, &buflen);
> if (r == 0 && buflen != 0 && (size_t) buflen < sizeof(FsckProgress)) {
> @@ -210,6 +349,9 @@ static int new_connection_handler(sd_event_source *s, int fd, uint32_t revents,
> remove_client(&(m->clients), client);
> return r;
> }
> + /* only request the client to cancel now in case the request is dropped by the client (chance to recancel) */
> + if (m->cancel_requested)
> + request_cancel_client(client);
> } else
> return log_error_errno(errno, "Couldn't accept a new connection: %m");
>
> @@ -233,6 +375,7 @@ static void manager_free(Manager *m) {
> }
>
> safe_close(m->connection_fd);
> + safe_close(m->plymouth_fd);
> if (m->console)
> fclose(m->console);
>
> @@ -266,6 +409,7 @@ static int manager_new(Manager **ret, int fd) {
> }
> m->percent = 100;
>
> + m->plymouth_fd = -1;
> *ret = m;
> m = NULL;
>
> diff --git a/src/fsckd/fsckd.h b/src/fsckd/fsckd.h
> index 6fe37a7..8239273 100644
> --- a/src/fsckd/fsckd.h
> +++ b/src/fsckd/fsckd.h
> @@ -32,3 +32,7 @@ typedef struct FsckProgress {
> size_t max;
> int pass;
> } FsckProgress;
> +
> +typedef struct FsckdMessage {
> + uint8_t cancel;
> +} FsckdMessage;
> --
> 2.1.4
>
More information about the systemd-devel
mailing list