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 <didro...@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 <didro...@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 > _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel