Re: [systemd-devel] [PATCH 2/5] Add a machine_id_commit call to commit on disk a, transient machine-id
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 =
Re: [systemd-devel] [PATCH 2/5] Add a machine_id_commit call to commit on disk a, transient machine-id
On Tue, 02.12.14 11:43, Didier Roche (didro...@ubuntu.com) wrote: Heya! Applied the patch with some changes (converted all log messages to the new errno logging). Please check if everything still works as intended. Also applied patches 3, 4, 5 after that. Thanks! Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/5] Add a machine_id_commit call to commit on disk a, transient machine-id
Le 03/12/2014 03:44, Lennart Poettering a écrit : On Tue, 02.12.14 11:43, Didier Roche (didro...@ubuntu.com) wrote: Heya! Applied the patch with some changes (converted all log messages to the new errno logging). Please check if everything still works as intended. Also applied patches 3, 4, 5 after that. Hey, I saw the changes, will use that function for future patches then. (Also, I saw you changed the const casting in other part of the code already there). Everything should work as expected, I'll give it a try. Many thanks! Didier ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/5] Add a machine_id_commit call to commit on disk a, transient machine-id
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.
+
+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.
+
+/* 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);
+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
