[systemd-devel] [PATCH 3/9] Connect to plymouth and support cancellation of in, progress fsck
Didier Roche
didrocks at ubuntu.com
Tue Feb 17 08:26:05 PST 2015
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.
>
>> +
>> + 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"
>
>> + }
>> +
>> + 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.
>
>>
>> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Connect-to-plymouth-and-support-cancellation-of-in-p.patch
Type: text/x-diff
Size: 15056 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20150217/41a51827/attachment.patch>
More information about the systemd-devel
mailing list