Le 01/12/2014 18:37, Lennart Poettering a écrit :
On Mon, 24.11.14 12:35, Didier Roche (didro...@ubuntu.com) wrote:
+static int is_on_temporary_fs(int fd) {
+ struct statfs s;
+
+ if (fstatfs(fd, &s) < 0)
+ return -errno;
+
+ return F_TYPE_EQUAL(s.f_type, TMPFS_MAGIC) ||
+ F_TYPE_EQUAL(s.f_type, RAMFS_MAGIC);
+}
This should really move to util.[ch] I figure, and reuse
is_temporary_fs() that we already have there.
Thanks for your feedback.
I did introduce is_fd_on_temporary_fs() in utils.[ch] and reused
is_temporary_fs there.
+
+int machine_id_commit(const char *root) {
+ const char *etc_machine_id;
+ _cleanup_close_ int fd = -1;
+ _cleanup_close_ int initial_mntns_fd = -1;
+ struct stat st;
+ char id[34]; /* 32 + \n + \0 */
+ int r;
+
+ if (isempty(root))
+ etc_machine_id = "/etc/machine-id";
+ else {
+ etc_machine_id = strappenda(root, "/etc/machine-id");
+ path_kill_slashes((char*) etc_machine_id);
+ }
+
+ r = path_is_mount_point(etc_machine_id, false);
+ if (r <= 0) {
+ log_error("We expected that %s was an independent mount.",
etc_machine_id);
+ return r < 0 ? r : -ENOENT;
+ }
I think this should really work in idempotent style, i.e. so that you
exit cleanly if the thing is already a proper file.
Making sense. I downgraded the error messaging to debug and always
returns 0. I tried other various use case and I think the functions
(hence, the binary) is idempotent now.
+
+ /* read existing machine-id */
+ fd = open(etc_machine_id, O_RDONLY|O_CLOEXEC|O_NOCTTY);
+ if (fd < 0) {
+ log_error("Cannot open %s: %m", etc_machine_id);
+ return -errno;
+ }
+ if (fstat(fd, &st) < 0) {
+ log_error("fstat() failed: %m");
+ return -errno;
+ }
+ if (!S_ISREG(st.st_mode)) {
+ log_error("We expected %s to be a regular file.",
etc_machine_id);
+ return -EISDIR;
+ }
+ r = get_valid_machine_id(fd, id);
+ if (r < 0) {
+ log_error("We didn't find a valid machine-id.");
+ return r;
+ }
+
+ r = is_on_temporary_fs(fd);
+ if (r <= 0) {
+ log_error("We expected %s to be a temporary file system.",
etc_machine_id);
+ return r;
+ }
+
+ fd = safe_close(fd);
+
+ /* store current mount namespace */
+ r = namespace_open(0, NULL, &initial_mntns_fd, NULL, NULL);
+ if (r < 0) {
+ log_error("Can't fetch current mount namespace: %s",
strerror(r));
+ return r;
+ }
+
+ /* switch to a new mount namespace, isolate ourself and unmount
etc_machine_id in our new namespace */
+ if (unshare(CLONE_NEWNS) == -1) {
+ log_error("Not enough privilege to switch to another
namespace.");
+ return EPERM;
+ }
+
+ if (mount(NULL, "/", NULL, MS_SLAVE | MS_REC, NULL) < 0) {
+ log_error("Couldn't make-rslave / mountpoint in our private
namespace.");
+ return EPERM;
+ }
+
+ r = umount(etc_machine_id);
+ if (r < 0) {
+ log_error("Failed to unmount transient %s file in our private
namespace: %m", etc_machine_id);
+ return -errno;
+ }
+
+ /* update a persistent version of etc_machine_id */
+ fd = open(etc_machine_id, O_RDWR|O_CREAT|O_CLOEXEC|O_NOCTTY, 0444);
+ if (fd < 0) {
+ log_error("Cannot open for writing %s. This is mandatory to get a
persistent machine-id: %m",
+ etc_machine_id);
+ return -errno;
+ }
+ if (fstat(fd, &st) < 0) {
+ log_error("fstat() failed: %m");
+ return -errno;
+ }
+ if (!S_ISREG(st.st_mode)) {
+ log_error("We expected %s to be a regular file.",
etc_machine_id);
+ return -EISDIR;
+ }
+
+ r = write_machine_id(fd, id);
+ if (r < 0) {
+ log_error("Cannot write %s: %s", etc_machine_id, strerror(r));
+ return r;
+ }
Since you prepared the original patch, we improved the loggic logic of
systemd. It would be great if you would update the patch to make use
of it. In particular, this means avoiding strerror() for cases like
the above (because it is not thread-safe to use). Instead use the new
log_error_errno() that takes the error value as first parameter, and
then makes sure that %m actually evaluates to the error string for
that error. Also it will then return the error, so that you can
simplify the four lines above into:
if (r < 0)
return log_error_errno(r, "Cannot write %s: %m",
etc_machine_id);
Nice feature! I replaced it in some other places as well.
+ fd = safe_close(fd);
+
+ /* return to initial namespace and proceed a lazy tmpfs unmount */
+ r = namespace_enter(-1, initial_mntns_fd, -1, -1);
+ if (r < 0) {
+ log_warning("Failed to switch back to initial mount namespace.
We'll keep transient %s file until next reboot.", etc_machine_id);
+ } else {
No {} for single-line if-blocks please.
Also, please print the error cause, use log_Warning_errno() for this,
and use %m...
Done (here and on the next statement as well)!
+ r = umount2(etc_machine_id, MNT_DETACH);
+ if (r < 0)
+ log_warning("Failed to unmount transient %s file. We keep
that mount until next reboot: %m", etc_machine_id);
+ }
+
+ return 0;
+}
+
int machine_id_setup(const char *root) {
Otherwise looks great.
Thanks! Here is the new version with the above suggestion.
Cheers,
Didier
>From bddb0c408e0a746cf053c1404ba44b02deea75f3 Mon Sep 17 00:00:00 2001
From: Didier Roche <didro...@ubuntu.com>
Date: Mon, 24 Nov 2014 09:43:29 +0100
Subject: [PATCH 2/5] Add a machine_id_commit call to commit on disk a
transient machine-id
If /etc was read only at boot time with an empty /etc/machine-id, the latter
will be mounted as a tmpfs and get reset at each boot. If the system becomes rw
later, this functionality enables to commit in a race-free manner the
transient machine-id to disk.
---
src/core/machine-id-setup.c | 107 ++++++++++++++++++++++++++++++++++++++++++++
src/core/machine-id-setup.h | 1 +
src/shared/util.c | 9 ++++
src/shared/util.h | 2 +
4 files changed, 119 insertions(+)
diff --git a/src/core/machine-id-setup.c b/src/core/machine-id-setup.c
index f3763ed..25448ce 100644
--- a/src/core/machine-id-setup.c
+++ b/src/core/machine-id-setup.c
@@ -188,6 +188,113 @@ static int write_machine_id(int fd, char id[34]) {
return -errno;
}
+int machine_id_commit(const char *root) {
+ const char *etc_machine_id;
+ _cleanup_close_ int fd = -1;
+ _cleanup_close_ int initial_mntns_fd = -1;
+ struct stat st;
+ char id[34]; /* 32 + \n + \0 */
+ int r;
+
+ if (isempty(root))
+ etc_machine_id = "/etc/machine-id";
+ else {
+ etc_machine_id = strappenda(root, "/etc/machine-id");
+ path_kill_slashes((char*) etc_machine_id);
+ }
+
+ if (path_is_mount_point(etc_machine_id, false) <= 0) {
+ log_debug("We expected that %s was a mount point. Nothing to do.", etc_machine_id);
+ return 0;
+ }
+
+ /* read existing machine-id */
+ fd = open(etc_machine_id, O_RDONLY|O_CLOEXEC|O_NOCTTY);
+ if (fd < 0) {
+ log_error("Cannot open %s: %m", etc_machine_id);
+ return -errno;
+ }
+ if (fstat(fd, &st) < 0) {
+ log_error("fstat() failed: %m");
+ return -errno;
+ }
+ if (!S_ISREG(st.st_mode)) {
+ log_error("We expected %s to be a regular file.", etc_machine_id);
+ return -EISDIR;
+ }
+ r = get_valid_machine_id(fd, id);
+ if (r < 0) {
+ log_error("We didn't find a valid machine-id.");
+ return r;
+ }
+
+ r = is_fd_on_temporary_fs(fd);
+ if (r <= 0) {
+ log_error("We expected %s to be a temporary file system.", etc_machine_id);
+ return r;
+ }
+
+ fd = safe_close(fd);
+
+ /* store current mount namespace */
+ r = namespace_open(0, NULL, &initial_mntns_fd, NULL, NULL);
+ if (r < 0)
+ return log_error_errno(r, "Can't fetch current mount namespace: %m");
+
+ /* switch to a new mount namespace, isolate ourself and unmount etc_machine_id in our new namespace */
+ if (unshare(CLONE_NEWNS) == -1) {
+ log_error("Not enough privilege to switch to another namespace.");
+ return EPERM;
+ }
+
+ if (mount(NULL, "/", NULL, MS_SLAVE | MS_REC, NULL) < 0) {
+ log_error("Couldn't make-rslave / mountpoint in our private namespace.");
+ return EPERM;
+ }
+
+ r = umount(etc_machine_id);
+ if (r < 0) {
+ log_error("Failed to unmount transient %s file in our private namespace: %m", etc_machine_id);
+ return -errno;
+ }
+
+ /* update a persistent version of etc_machine_id */
+ fd = open(etc_machine_id, O_RDWR|O_CREAT|O_CLOEXEC|O_NOCTTY, 0444);
+ if (fd < 0) {
+ log_error("Cannot open for writing %s. This is mandatory to get a persistent machine-id: %m",
+ etc_machine_id);
+ return -errno;
+ }
+ if (fstat(fd, &st) < 0) {
+ log_error("fstat() failed: %m");
+ return -errno;
+ }
+ if (!S_ISREG(st.st_mode)) {
+ log_error("We expected %s to be a regular file.", etc_machine_id);
+ return -EISDIR;
+ }
+
+ r = write_machine_id(fd, id);
+ if (r < 0)
+ return log_error_errno(r, "Cannot write %s: %m", etc_machine_id);
+
+ fd = safe_close(fd);
+
+ /* return to initial namespace and proceed a lazy tmpfs unmount */
+ r = namespace_enter(-1, initial_mntns_fd, -1, -1);
+ if (r < 0)
+ log_warning_errno(r, "Failed to switch back to initial mount namespace: %m.\nWe'll keep transient %s file until next reboot.",
+ etc_machine_id);
+ else {
+ r = umount2(etc_machine_id, MNT_DETACH);
+ if (r < 0)
+ log_warning_errno(r, "Failed to unmount transient %s file: %m.\nWe keep that mount until next reboot.",
+ etc_machine_id);
+ }
+
+ return 0;
+}
+
int machine_id_setup(const char *root) {
const char *etc_machine_id, *run_machine_id;
_cleanup_close_ int fd = -1;
diff --git a/src/core/machine-id-setup.h b/src/core/machine-id-setup.h
index b0583ee..f7707c3 100644
--- a/src/core/machine-id-setup.h
+++ b/src/core/machine-id-setup.h
@@ -21,4 +21,5 @@
along with systemd; If not, see <http://www.gnu.org/licenses/>.
***/
+int machine_id_commit(const char *root);
int machine_id_setup(const char *root);
diff --git a/src/shared/util.c b/src/shared/util.c
index 2165170..4fcbab9 100644
--- a/src/shared/util.c
+++ b/src/shared/util.c
@@ -3052,6 +3052,15 @@ _pure_ static int is_temporary_fs(struct statfs *s) {
F_TYPE_EQUAL(s->f_type, RAMFS_MAGIC);
}
+int is_fd_on_temporary_fs(int fd) {
+ struct statfs s;
+
+ if (fstatfs(fd, &s) < 0)
+ return -errno;
+
+ return is_temporary_fs(&s);
+}
+
int rm_rf_children(int fd, bool only_dirs, bool honour_sticky, struct stat *root_dev) {
struct statfs s;
diff --git a/src/shared/util.h b/src/shared/util.h
index b53a45d..0b7c77b 100644
--- a/src/shared/util.h
+++ b/src/shared/util.h
@@ -454,6 +454,8 @@ int get_ctty(pid_t, dev_t *_devnr, char **r);
int chmod_and_chown(const char *path, mode_t mode, uid_t uid, gid_t gid);
int fchmod_and_fchown(int fd, mode_t mode, uid_t uid, gid_t gid);
+int is_fd_on_temporary_fs(int fd);
+
int rm_rf_children(int fd, bool only_dirs, bool honour_sticky, struct stat *root_dev);
int rm_rf_children_dangerous(int fd, bool only_dirs, bool honour_sticky, struct stat *root_dev);
int rm_rf(const char *path, bool only_dirs, bool delete_root, bool honour_sticky);
--
2.1.3
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
