Le 10/03/2015 11:55, Lennart Poettering a écrit :
On Tue, 10.03.15 08:33, Martin Pitt (martin.p...@ubuntu.com) wrote:
Lennart Poettering [2015-03-09 19:11 +0100]:
Anyway, please look into fixing this, I am kinda relying on patches
for this, as I don't use this myself. Fedora isn't set up for it, nor
do I use a file system that still requires fsck at boot...
Yep, we'll fix those. But for the record, this can be tested easily
anywhere by replacing /usr/sbin/fsck with test/mocks/fsck (I'm on
btrfs myself..)
Hmm, is there also a ply mocker? That would be good!
There is none AFAIK, I'm using plymouth-x11 which is working quite well.
The only difficulty that I realized is that Control+C doesn't work for
cancelling into that X window. Consequently, everytime I changed to "c"
or any other key to test.
Well, again: fsck should *not* 'forward'. It should not be bothered at
all with processing stdout of the actual fsck tool at all. Instead it
should open a AF_UNIX/SOCK_STREAM connection to fsckd, use shutdown()
to close the read side, and then make the resulting fd the fd passed
to fsck's -C parameter. That way, the fsck tool will pass the progress
data *directly* to systemd-fsckd, and systemd-fsck will never see it
at all!
The suggestion is now implemented in the attached patch. Please note
that they rely on 0001-fsckd-Don-t-use-strjoina-on-gettext-call.patch
(just posted a new version) and
0002-fsckd-check-if-plymouth-is-running-before-attempting.patch (didn't
change that one).
It should handle what you had your mind, still enabling cancellation
through SIGTERM while removing the need for intermediates sockets.
Please note that I needed to open in systemd-fsck the socket in the
fork() children part to be able to identify the right fsck pid. I thus
used then an empty cmdline[] slot for this. Another way would be to
build the command line in the fork() children call entirely of course.
Not sure what you would prefer.
As always, I'm opened to any suggestion and hope that will satisfy your
original vision more :)
Cheers,
Didier
>From cdfcfb8490cd07c7b9129e6ce7c382c962c2c4dd Mon Sep 17 00:00:00 2001
From: Didier Roche <didro...@ubuntu.com>
Date: Wed, 11 Mar 2015 12:58:39 +0100
Subject: [PATCH 3/3] fsckd: directly connect fsck output to systemd-fsckd
* Remove the intermediate structure to handle progress report and cancellation
communication from systemd-fsck to systemd-fsckd.
* Directly connect fsck -C<fd> to systemd-fsckd. systemd-fsck doesn't get any
fd connecting from fsck anymore.
* Handle cancellation in systemd-fsckd by directly SIGTERM the fsck process
(which in turn, will make systemd-fsck terminate)
* In case of premature termination of fsck, only ignore the unidentified error
if SIGTERM was sent.
---
src/fsck/fsck.c | 111 ++++++++++++---------------------------
src/fsckd/fsckd.c | 154 ++++++++++++++++++++++++++++++++++--------------------
src/fsckd/fsckd.h | 13 -----
3 files changed, 130 insertions(+), 148 deletions(-)
diff --git a/src/fsck/fsck.c b/src/fsck/fsck.c
index 6e46633..b3dbd29 100644
--- a/src/fsck/fsck.c
+++ b/src/fsck/fsck.c
@@ -27,6 +27,7 @@
#include <fcntl.h>
#include <sys/file.h>
#include <sys/stat.h>
+#include <sys/socket.h>
#include "sd-bus.h"
#include "libudev.h"
@@ -130,81 +131,38 @@ static void test_files(void) {
}
-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;
+static int make_progress_fd(void) {
static const union sockaddr_union sa = {
.un.sun_family = AF_UNIX,
.un.sun_path = FSCKD_SOCKET_PATH,
};
+ int fsck_fd;
- fsckd_fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0);
- if (fsckd_fd < 0)
- return log_warning_errno(errno, "Cannot open fsckd socket, we won't report fsck progress: %m");
- if (connect(fsckd_fd, &sa.sa, offsetof(struct sockaddr_un, sun_path) + strlen(sa.un.sun_path)) < 0)
- return log_warning_errno(errno, "Cannot connect to fsckd socket, we won't report fsck progress: %m");
+ fsck_fd = socket(AF_UNIX, SOCK_STREAM, 0);
+ if (fsck_fd < 0)
+ return log_warning_errno(errno, "Cannot crate fsck socket, we won't report fsck progress: %m");
- f = fdopen(fd, "r");
- if (!f)
- return log_warning_errno(errno, "Cannot connect to fsck, we won't report fsck progress: %m");
-
- while (!feof(f)) {
- int pass;
- size_t buflen;
- size_t cur, max;
- 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;
-
- /* Only update once every 50ms */
- t = now(CLOCK_MONOTONIC);
- if (last + 50 * USEC_PER_MSEC > t)
- continue;
-
- last = t;
-
- /* send progress to fsckd */
- progress.devnum = device_num;
- progress.cur = cur;
- progress.max = max;
- progress.pass = pass;
-
- 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);
- }
- }
- }
+ if (shutdown(fsck_fd, SHUT_RD) < 0)
+ return log_warning_errno(errno, "Cannot make fsck socket write-only, we won't report fsck progress: %m");
- return 0;
+ if (connect(fsck_fd, &sa.sa, offsetof(struct sockaddr_un, sun_path) + strlen(sa.un.sun_path)) < 0)
+ return log_warning_errno(errno, "Cannot connect to fsckd socket, we won't report fsck progress: %m");
+
+ return fsck_fd;
}
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;
const char *device, *type;
bool root_directory;
- _cleanup_close_pair_ int progress_pipe[2] = { -1, -1 };
+ _cleanup_close_ int fsck_fd = -1;
char dash_c[sizeof("-C")-1 + DECIMAL_STR_MAX(int) + 1];
+ int fd_arg_index = -1;
struct stat st;
if (argc > 2) {
@@ -302,11 +260,6 @@ int main(int argc, char *argv[]) {
log_warning_errno(r, "fsck.%s cannot be used for %s: %m", type, device);
}
- if (pipe(progress_pipe) < 0) {
- r = log_error_errno(errno, "pipe(): %m");
- goto finish;
- }
-
cmdline[i++] = "/sbin/fsck";
cmdline[i++] = arg_repair;
cmdline[i++] = "-T";
@@ -324,9 +277,8 @@ int main(int argc, char *argv[]) {
if (arg_force)
cmdline[i++] = "-f";
- xsprintf(dash_c, "-C%i", progress_pipe[1]);
- cmdline[i++] = dash_c;
-
+ /* mark index for -C arg */
+ fd_arg_index = i++;
cmdline[i++] = device;
cmdline[i++] = NULL;
@@ -336,15 +288,22 @@ int main(int argc, char *argv[]) {
goto finish;
} else if (pid == 0) {
/* Child */
- progress_pipe[0] = safe_close(progress_pipe[0]);
+
+ /* create and connect systemd-fsckd socket to fsck fd.
+ only log a warning if progress couldn't be reported
+ but don't stop. */
+ fsck_fd = make_progress_fd();
+ if (fsck_fd >= 0) {
+ xsprintf(dash_c, "-C%i", fsck_fd);
+ cmdline[fd_arg_index] = dash_c;
+ } else
+ /* put in a harmless and redundant option */
+ cmdline[fd_arg_index] = "-T";
+
execv(cmdline[0], (char**) cmdline);
_exit(8); /* Operational error */
}
-
- progress_pipe[1] = safe_close(progress_pipe[1]);
-
- progress_rc = process_progress(progress_pipe[0], pid, st.st_rdev);
- progress_pipe[0] = -1;
+ safe_close(fsck_fd);
r = wait_for_terminate(pid, &status);
if (r < 0) {
@@ -352,14 +311,13 @@ int main(int argc, char *argv[]) {
goto finish;
}
- if (status.si_code != CLD_EXITED || (status.si_status & ~1) || progress_rc != 0) {
+ if (status.si_code != CLD_EXITED || (status.si_status & ~1)) {
- /* cancel will kill fsck (but process_progress returns 0) */
- if ((progress_rc != 0 && status.si_code == CLD_KILLED) || status.si_code == CLD_DUMPED)
+ if (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 if (progress_rc != 0)
+ else
log_error("fsck failed due to unknown reason.");
r = -EINVAL;
@@ -370,10 +328,9 @@ int main(int argc, char *argv[]) {
else if (status.si_code == CLD_EXITED && (status.si_status & 6))
/* Some other problem */
start_target(SPECIAL_EMERGENCY_TARGET);
- else {
+ else if (status.si_status == 15) {
r = 0;
- if (progress_rc != 0)
- log_warning("Ignoring error.");
+ log_warning("Ignoring error.");
}
} else
diff --git a/src/fsckd/fsckd.c b/src/fsckd/fsckd.c
index 6b35fc2..31add9b 100644
--- a/src/fsckd/fsckd.c
+++ b/src/fsckd/fsckd.c
@@ -53,8 +53,12 @@ struct Manager;
typedef struct Client {
struct Manager *manager;
- int fd;
- dev_t devnum;
+ char *device_name;
+ /* device id refers to "fd <fd>" until it gets a name as "device_name" */
+ char *device_id;
+
+ pid_t fsck_pid;
+ FILE *fsck_f;
size_t cur;
size_t max;
@@ -62,8 +66,8 @@ typedef struct Client {
double percent;
- size_t buflen;
bool cancelled;
+ bool bad_input;
sd_event_source *event_source;
@@ -148,23 +152,16 @@ static double compute_percent(int pass, size_t cur, size_t max) {
}
static int client_request_cancel(Client *c) {
- FsckdMessage cancel_msg = {
- .cancel = 1,
- };
-
- ssize_t n;
-
assert(c);
if (c->cancelled)
return 0;
- n = send(c->fd, &cancel_msg, sizeof(FsckdMessage), 0);
- if (n < 0)
- return log_warning_errno(errno, "Cannot send cancel to fsck on (%u:%u): %m", major(c->devnum), minor(c->devnum));
- if ((size_t) n < sizeof(FsckdMessage)) {
- log_warning("Short send when sending cancel to fsck on (%u:%u).", major(c->devnum), minor(c->devnum));
- return -EIO;
+ log_info("Request to cancel fsck for %s from fsckd", c->device_id);
+ if (kill(c->fsck_pid, SIGTERM) < 0) {
+ /* ignore the error and consider that cancel was sent if fsck just exited */
+ if (errno != ESRCH)
+ return log_error_errno(errno, "Cannot send cancel to fsck for %s: %m", c->device_id);
}
c->cancelled = true;
@@ -180,8 +177,11 @@ static void client_free(Client *c) {
}
sd_event_source_unref(c->event_source);
-
- safe_close(c->fd);
+ fclose(c->fsck_f);
+ if (c->device_name)
+ free(c->device_name);
+ if (c->device_id)
+ free(c->device_id);
free(c);
}
@@ -362,76 +362,106 @@ static int manager_update_global_progress(Manager *m) {
static int client_progress_handler(sd_event_source *s, int fd, uint32_t revents, void *userdata) {
Client *client = userdata;
- FsckProgress fsck_data;
- size_t buflen;
+ char line[LINE_MAX];
Manager *m;
- int r;
assert(client);
-
m = client->manager;
/* check first if we need to cancel this client */
if (m->cancel_requested)
client_request_cancel(client);
- /* ensure we have enough data to read */
- r = ioctl(fd, FIONREAD, &buflen);
- if (r == 0 && buflen != 0 && (size_t) buflen < sizeof(FsckProgress)) {
- if (client->buflen != buflen)
- client->buflen = buflen;
- /* we got twice the same size from a bad behaving client, kick it off the list */
- else {
- log_warning("Closing bad behaving fsck client connection at fd %d", client->fd);
- client_free(client);
- manager_update_global_progress(m);
+ while (fgets(line, sizeof(line), client->fsck_f) != NULL) {
+ int pass;
+ size_t cur, max;
+ _cleanup_free_ char *device = NULL, *old_device_id = NULL;
+
+ if (sscanf(line, "%i %lu %lu %ms", &pass, &cur, &max, &device) == 4) {
+ if (!client->device_name) {
+ client->device_name = strdup(device);
+ if (!client->device_name) {
+ log_oom();
+ continue;
+ }
+ old_device_id = client->device_id;
+ client->device_id = strdup(device);
+ if (!client->device_id) {
+ log_oom();
+ client->device_id = old_device_id;
+ old_device_id = NULL;
+ continue;
+ }
+ }
+ client->pass = pass;
+ client->cur = cur;
+ client->max = max;
+ client->bad_input = false;
+ client->percent = compute_percent(client->pass, client->cur, client->max);
+ log_debug("Getting progress for %s (%lu, %lu, %d) : %3.1f%%", client->device_id,
+ client->cur, client->max, client->pass, client->percent);
+ } else {
+ if (errno == ENOMEM) {
+ log_oom();
+ continue;
+ }
+
+ /* if previous input was already garbage, kick it off from progress report */
+ if (client->bad_input) {
+ log_warning("Closing connection on incorrect input of fsck connection for %s", client->device_id);
+ client_free(client);
+ manager_update_global_progress(m);
+ return 0;
+ }
+ client->bad_input = true;
}
- return 0;
+
}
- /* read actual data */
- r = recv(fd, &fsck_data, sizeof(FsckProgress), 0);
- if (r == 0) {
- log_debug("Fsck client connected to fd %d disconnected", client->fd);
+ if (feof(client->fsck_f)) {
+ log_debug("Fsck client %s disconnected", client->device_id);
client_free(client);
- } else if (r > 0 && r != sizeof(FsckProgress))
- log_warning("Unexpected data structure sent to fsckd socket from fd: %d. Ignoring", client->fd);
- else if (r > 0 && r == sizeof(FsckProgress)) {
- client->devnum = fsck_data.devnum;
- client->cur = fsck_data.cur;
- client->max = fsck_data.max;
- client->pass = fsck_data.pass;
- client->percent = compute_percent(client->pass, client->cur, client->max);
- log_debug("Getting progress for %u:%u (%lu, %lu, %d) : %3.1f%%",
- major(client->devnum), minor(client->devnum),
- client->cur, client->max, client->pass, client->percent);
- } else
- log_error_errno(r, "Unknown error while trying to read fsck data: %m");
+ }
manager_update_global_progress(m);
-
return 0;
}
static int manager_new_connection_handler(sd_event_source *s, int fd, uint32_t revents, void *userdata) {
_cleanup_(client_freep) Client *c = NULL;
- _cleanup_close_ int new_client_fd = -1;
+ _cleanup_close_ int new_fsck_fd = -1;
+ _cleanup_fclose_ FILE *new_fsck_f = NULL;
+ struct ucred ucred = {};
Manager *m = userdata;
int r;
assert(m);
/* Initialize and list new clients */
- new_client_fd = accept4(m->connection_fd, NULL, NULL, SOCK_CLOEXEC);
- if (new_client_fd < 0)
- return log_error_errno(errno, "Couldn't accept a new connection: %m");
+ new_fsck_fd = accept4(m->connection_fd, NULL, NULL, SOCK_CLOEXEC|SOCK_NONBLOCK);
+ if (new_fsck_fd < 0) {
+ log_error_errno(errno, "Couldn't accept a new connection: %m");
+ return 0;
+ }
if (m->n_clients >= CLIENTS_MAX) {
log_error("Too many clients, refusing connection.");
return 0;
}
- log_debug("New fsck client connected to fd: %d", new_client_fd);
+
+ new_fsck_f = fdopen(new_fsck_fd, "r");
+ if (!new_fsck_f) {
+ log_error_errno(errno, "Couldn't fdopen new connection for fd %d: %m", new_fsck_fd);
+ return 0;
+ }
+ new_fsck_fd = -1;
+
+ r = getpeercred(fileno(new_fsck_f), &ucred);
+ if (r < 0) {
+ log_error_errno(r, "Couldn't get credentials for fsck: %m");
+ return 0;
+ }
c = new0(Client, 1);
if (!c) {
@@ -439,10 +469,16 @@ static int manager_new_connection_handler(sd_event_source *s, int fd, uint32_t r
return 0;
}
- c->fd = new_client_fd;
- new_client_fd = -1;
+ c->fsck_pid = ucred.pid;
+ c->fsck_f = new_fsck_f;
+ new_fsck_f = NULL;
+
+ if (asprintf(&(c->device_id), "fd %d", fileno(c->fsck_f)) < 0) {
+ log_oom();
+ return 0;
+ }
- r = sd_event_add_io(m->event, &c->event_source, c->fd, EPOLLIN, client_progress_handler, c);
+ r = sd_event_add_io(m->event, &c->event_source, fileno(c->fsck_f), EPOLLIN, client_progress_handler, c);
if (r < 0) {
log_oom();
return 0;
@@ -452,6 +488,8 @@ static int manager_new_connection_handler(sd_event_source *s, int fd, uint32_t r
m->n_clients++;
c->manager = m;
+ log_debug("New fsck client connected: %s", c->device_id);
+
/* only request the client to cancel now in case the request is dropped by the client (chance to recancel) */
if (m->cancel_requested)
client_request_cancel(c);
diff --git a/src/fsckd/fsckd.h b/src/fsckd/fsckd.h
index 8239273..27a0d6b 100644
--- a/src/fsckd/fsckd.h
+++ b/src/fsckd/fsckd.h
@@ -23,16 +23,3 @@
***/
#define FSCKD_SOCKET_PATH "/run/systemd/fsckd"
-
-#include "libudev.h"
-
-typedef struct FsckProgress {
- dev_t devnum;
- size_t cur;
- 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
