[systemd-devel] Hosts without /etc/machine-id on boot

[Didier Roche]

Hey,

Some other topic related to "empty /etc" discussions: when preparing
some generic distro images, we are have the desire to ensure that all
new instances will get a different /etc/machine-id file.
As part of the empty /etc at boot, we first thought that removing
/etc/machine-id would be sufficient, however, the instance then doesn't
generate a new machine-id file and complain heavily.

The new debug message of systemd 216+ helped shading some lights on it:
http://cgit.freedesktop.org/systemd/systemd-stable/diff/src/core/machine-id-setup.c?h=v216-stable&id=896050eeb3acbf4106d71204a5173b4984cf1675,
and adding debug statement in machine_id_setup() from
src/core/machine-id-setup.c just before "open(etc_machine_id,
O_RDWR|O_CREAT|O_CLOEXEC|O_NOCTTY, 0444)" explains what happens with
/proc/mounts:

[ 2.119041] systemd[1]: rootfs / rootfs rw
[ 2.126775] systemd[1]: /dev/disk/by-uuid/ec8166e5-d5ed-45ec-b350-6cf5773904ac / ext4 ro,relatime,data=ordered


It's clear then that at this stage of the boot process / is readonly.
The error message (and code) will say that in this case, what is
supported is an empty /etc/machine-id. After reboot, the consequence is
that /etc/machine-id is mounted as a tmpfs:

tmpfs on /etc/machine-id type tmpfs (ro,relatime,size=204948k,mode=755)

However, this means is that each boot of this instance will result in a
different machine-id, which isn't what is desired in the empty /etc case
after a factory reset. I know that there is the utility
systemd-machine-id-setup that we are running on systemd postinst in
debian/ubuntu, but that doesn't cover the factory reset one.

Is there anything obvious that I'm missing to cover that case or
anything in the pipe?
Cheers,
Didier

[Lennart Poettering]

Hey,
Some other topic related to "empty /etc" discussions: when preparing some
generic distro images, we are have the desire to ensure that all new
instances will get a different /etc/machine-id file.
As part of the empty /etc at boot, we first thought that removing
/etc/machine-id would be sufficient, however, the instance then doesn't
generate a new machine-id file and complain heavily.
The new debug message of systemd 216+ helped shading some lights on it: http://cgit.freedesktop.org/systemd/systemd-stable/diff/src/core/machine-id-setup.c?h=v216-stable&id=896050eeb3acbf4106d71204a5173b4984cf1675,
and adding debug statement in machine_id_setup() from
src/core/machine-id-setup.c just before "open(etc_machine_id,
O_RDWR|O_CREAT|O_CLOEXEC|O_NOCTTY, 0444)" explains what happens with
[ 2.119041] systemd[1]: rootfs / rootfs rw
[ 2.126775] systemd[1]: /dev/disk/by-uuid/ec8166e5-d5ed-45ec-b350-6cf5773904ac / ext4 ro,relatime,data=ordered
It's clear then that at this stage of the boot process / is readonly.
The error message (and code) will say that in this case, what is supported
is an empty /etc/machine-id. After reboot, the consequence is that

Yes, generation of the machine ID is done very very early at boot,
before we fork off the first non-PID1 userspace process, and hence
before any file system could be remounted writable.

tmpfs on /etc/machine-id type tmpfs (ro,relatime,size=204948k,mode=755)
However, this means is that each boot of this instance will result in a
different machine-id, which isn't what is desired in the empty /etc case
after a factory reset. I know that there is the utility
systemd-machine-id-setup that we are running on systemd postinst in
debian/ubuntu, but that doesn't cover the factory reset one.
Is there anything obvious that I'm missing to cover that case or anything in
the pipe?

You have a couple of options:

a) make /etc writable before systemd is invoked. If you use an initrd
this is without risk, given that the initrd should really invoke
fsck on the root disk anyway, and there's hence little reason to
transition to a read-only root, rather than just doing rw
right-away.

b) pre-initialize the machine ID before you boot, at build time.

c) live with random ids

d) pass in the id to use via $container_uuid (if you use a container
manager), or via the DMI uuid field (if you use kvm). Then, create
/etc/machine-id as an empty file, and systemd will initialize it to
this ID rather than a random one.

Usually, option d) is preferable in cloud setups I guess since it
allows seeding the machine id from some externally used UUID, the way
many container/virtualization managers define one anyway.

I'd be open to add another option on top of this:

e) boot up with /etc read-only and /etc/machine-id empty, so that the
usualy logic of c) generates a random machine id and overmounts
/etc/machine-id with it. But then, add a tiny new bootup service,
that runs shortly after local-fs.target (i.e. the point where /etc/
has been made writable if it's supposed to be made writable
according to fstab), and that syncs the random one used so far back
to disk, so that at the next boot-up it is fully initialized. This
tiny service should be properly conditioned so that it only runs if
/etc/machine-id is overmounted and /etc writable
(i.e. ConditionPathIsMountPoint=/etc/machine-id and
ConditionPathIsReadWrite=/etc). Special care should be taken so
that replacement of the mount by the normal file is
race-free. (This probably means the tool should open a new ount
namespace temporarily, unmount /etc/machine-id there, update the
file undearneath and then return to the original mount namespace
and unmount the file there too, so that at no time the filw is
invalid).

The guarantee with /etc/machine-id is really that it is valid at *any*
time, in early boot and late boot and all the time in between.

Hope this makes sense?

Lennart

[Didier Roche]

Post by Lennart Poettering

Hey,
Some other topic related to "empty /etc" discussions: when preparing some
generic distro images, we are have the desire to ensure that all new
instances will get a different /etc/machine-id file.
As part of the empty /etc at boot, we first thought that removing
/etc/machine-id would be sufficient, however, the instance then doesn't
generate a new machine-id file and complain heavily.
The new debug message of systemd 216+ helped shading some lights on it: http://cgit.freedesktop.org/systemd/systemd-stable/diff/src/core/machine-id-setup.c?h=v216-stable&id=896050eeb3acbf4106d71204a5173b4984cf1675,
and adding debug statement in machine_id_setup() from
src/core/machine-id-setup.c just before "open(etc_machine_id,
O_RDWR|O_CREAT|O_CLOEXEC|O_NOCTTY, 0444)" explains what happens with
[ 2.119041] systemd[1]: rootfs / rootfs rw
[ 2.126775] systemd[1]: /dev/disk/by-uuid/ec8166e5-d5ed-45ec-b350-6cf5773904ac / ext4 ro,relatime,data=ordered
It's clear then that at this stage of the boot process / is readonly.
The error message (and code) will say that in this case, what is supported
is an empty /etc/machine-id. After reboot, the consequence is that

Yes, generation of the machine ID is done very very early at boot,
before we fork off the first non-PID1 userspace process, and hence
before any file system could be remounted writable.

That makes sense.

Post by Lennart Poettering

tmpfs on /etc/machine-id type tmpfs (ro,relatime,size=204948k,mode=755)
However, this means is that each boot of this instance will result in a
different machine-id, which isn't what is desired in the empty /etc case
after a factory reset. I know that there is the utility
systemd-machine-id-setup that we are running on systemd postinst in
debian/ubuntu, but that doesn't cover the factory reset one.
Is there anything obvious that I'm missing to cover that case or anything in
the pipe?

a) make /etc writable before systemd is invoked. If you use an initrd
this is without risk, given that the initrd should really invoke
fsck on the root disk anyway, and there's hence little reason to
transition to a read-only root, rather than just doing rw
right-away.

Interesting, I run that through our kernel team. However, we run fsck a
little bit later on in the boot process to be able to pipe the output to
plymouth.
I'm not sure we should then have two code paths:
- one fscking from the initrd if /etc/machine-id is empty (like after a
factory reset), showing the results and eventual failures to the user in
some way
- and then, the general use case: fscking through the systemd service
via systemd-fsck-root.service before local-fs.target and piping the
result in plymouth

Post by Lennart Poettering
b) pre-initialize the machine ID before you boot, at build time.
c) live with random ids

Those 2 are actually nice features, but not applying with a machine if
we factory reset it with an empty /etc.

Post by Lennart Poettering
d) pass in the id to use via $container_uuid (if you use a container
manager), or via the DMI uuid field (if you use kvm). Then, create
/etc/machine-id as an empty file, and systemd will initialize it to
this ID rather than a random one.
Usually, option d) is preferable in cloud setups I guess since it
allows seeding the machine id from some externally used UUID, the way
many container/virtualization managers define one anyway.

Fully agreed (not the case I showed up here, but nice to know we can
pass pre-generated uuid to containers/vms).

Post by Lennart Poettering
e) boot up with /etc read-only and /etc/machine-id empty, so that the
usualy logic of c) generates a random machine id and overmounts
/etc/machine-id with it. But then, add a tiny new bootup service,
that runs shortly after local-fs.target (i.e. the point where /etc/
has been made writable if it's supposed to be made writable
according to fstab), and that syncs the random one used so far back
to disk, so that at the next boot-up it is fully initialized. This
tiny service should be properly conditioned so that it only runs if
/etc/machine-id is overmounted and /etc writable
(i.e. ConditionPathIsMountPoint=/etc/machine-id and
ConditionPathIsReadWrite=/etc). Special care should be taken so
that replacement of the mount by the normal file is
race-free. (This probably means the tool should open a new ount
namespace temporarily, unmount /etc/machine-id there, update the
file undearneath and then return to the original mount namespace
and unmount the file there too, so that at no time the filw is
invalid).
The guarantee with /etc/machine-id is really that it is valid at *any*
time, in early boot and late boot and all the time in between.

I think I will go that path which is an interesting one and mapping some
of my thoughts. Thanks for the guidance and documentation on what's the
right approach to achieve this race-free! I'll work on something around
that and propose a patch.
This should bring us one step closer (even if it will require an empty
/etc/machine-id file for now) to the factory reset (with an empty /etc)
case.

Post by Lennart Poettering
Hope this makes sense?

It completely does, thanks again for your detailed answer!

Cheers,
Didier

[Lennart Poettering]

Post by Didier Roche

Post by Lennart Poettering
a) make /etc writable before systemd is invoked. If you use an initrd
this is without risk, given that the initrd should really invoke
fsck on the root disk anyway, and there's hence little reason to
transition to a read-only root, rather than just doing rw
right-away.

Interesting, I run that through our kernel team. However, we run fsck a
little bit later on in the boot process to be able to pipe the output to
plymouth.

At least on Fedora plymouth already runs on the initrd. If Ubuntu does
the same, then there shouldn't be a difference regarding where fsck is
run...

Note that running fsck in the initrd for the root fs is really the
right thing to do: running fsck from the file system you are about to
check, which you hence cannot trust, is really wrong.

Post by Didier Roche
- one fscking from the initrd if /etc/machine-id is empty (like after a
factory reset), showing the results and eventual failures to the user in
some way
- and then, the general use case: fscking through the systemd service via
systemd-fsck-root.service before local-fs.target and piping the result in
plymouth

The latter is useful only really on non-initrd boots where there isn't
any initrd where the fsck could run. General purpose distributions
should really run fsck in the initrd.

Note that systemd-fsck-root.service skips itself when it notices that
the fs was already checked (via a flag file in /run).

Post by Didier Roche

Post by Lennart Poettering
The guarantee with /etc/machine-id is really that it is valid at *any*
time, in early boot and late boot and all the time in between.

I think I will go that path which is an interesting one and mapping some of
my thoughts. Thanks for the guidance and documentation on what's the right
approach to achieve this race-free! I'll work on something around that and
propose a patch.

Looking forword to it.

Lennart

[Didier Roche]

Post by Lennart Poettering

Post by Didier Roche

Post by Lennart Poettering
a) make /etc writable before systemd is invoked. If you use an initrd
this is without risk, given that the initrd should really invoke
fsck on the root disk anyway, and there's hence little reason to
transition to a read-only root, rather than just doing rw
right-away.

Interesting, I run that through our kernel team. However, we run fsck a
little bit later on in the boot process to be able to pipe the output to
plymouth.

At least on Fedora plymouth already runs on the initrd. If Ubuntu does
the same, then there shouldn't be a difference regarding where fsck is
run...
Note that running fsck in the initrd for the root fs is really the
right thing to do: running fsck from the file system you are about to
check, which you hence cannot trust, is really wrong.

Post by Didier Roche
- one fscking from the initrd if /etc/machine-id is empty (like after a
factory reset), showing the results and eventual failures to the user in
some way
- and then, the general use case: fscking through the systemd service via
systemd-fsck-root.service before local-fs.target and piping the result in
plymouth

The latter is useful only really on non-initrd boots where there isn't
any initrd where the fsck could run. General purpose distributions
should really run fsck in the initrd.
Note that systemd-fsck-root.service skips itself when it notices that
the fs was already checked (via a flag file in /run).

Indeed, that makes sense and we should investigate that with our kernel
team in the near future. I'm going to open a thread on it.

Post by Lennart Poettering

Post by Didier Roche

Post by Lennart Poettering
The guarantee with /etc/machine-id is really that it is valid at *any*
time, in early boot and late boot and all the time in between.

I think I will go that path which is an interesting one and mapping some of
my thoughts. Thanks for the guidance and documentation on what's the right
approach to achieve this race-free! I'll work on something around that and
propose a patch.

Looking forword to it.

Here we go :)
I did some factorization to reuse some functions on the first path and
added the binary helper, unit and man page.

It should follow your advice and be race-free. Tested various cases locally.

Cheers,
Didier

[Didier Roche]

[Lennart Poettering]

+static int get_valid_machine_id(int fd, char id[34]) {
+ assert(fd >= 0);
+ assert(id);
+
+ if (loop_read(fd, id, 33, false) == 33 && id[32] == '\n') {
+ id[32] = 0;
+
+ if (id128_is_valid(id)) {
+ id[32] = '\n';
+ id[33] = 0;
+ return 0;
+ }
+ }
+
+ return -EINVAL;
+}

As a matter of coding style we try hard to avoid functions that clobber
their parameters if they fail. Please follow the same scheme here.

Lennart

[Didier Roche]

Post by Lennart Poettering

+static int get_valid_machine_id(int fd, char id[34]) {
+ assert(fd >= 0);
+ assert(id);
+
+ if (loop_read(fd, id, 33, false) == 33 && id[32] == '\n') {
+ id[32] = 0;
+
+ if (id128_is_valid(id)) {
+ id[32] = '\n';
+ id[33] = 0;
+ return 0;
+ }
+ }
+
+ return -EINVAL;
+}

As a matter of coding style we try hard to avoid functions that clobber
their parameters if they fail. Please follow the same scheme here.

That makes sense, I rewrote the function to avoid this.

Didier

[Lennart Poettering]

On Tue, 02.12.14 11:43, Didier Roche (***@ubuntu.com) wrote:

Looks good! Applied!

Post by Didier Roche

Post by Lennart Poettering

+static int get_valid_machine_id(int fd, char id[34]) {
+ assert(fd >= 0);
+ assert(id);
+
+ if (loop_read(fd, id, 33, false) == 33 && id[32] == '\n') {
+ id[32] = 0;
+
+ if (id128_is_valid(id)) {
+ id[32] = '\n';
+ id[33] = 0;
+ return 0;
+ }
+ }
+
+ return -EINVAL;
+}

As a matter of coding style we try hard to avoid functions that clobber
their parameters if they fail. Please follow the same scheme here.

That makes sense, I rewrote the function to avoid this.
Didier

Post by Lennart Poettering
From 7cf7322ab08c9434ba303d9958517f262b1797e0 Mon Sep 17 00:00:00 2001

Date: Mon, 24 Nov 2014 09:40:57 +0100
Subject: [PATCH 1/5] Factorize some machine-id-setup functions to be reused
---
src/core/machine-id-setup.c | 44 ++++++++++++++++++++++++++++++++++----------
1 file changed, 34 insertions(+), 10 deletions(-)
diff --git a/src/core/machine-id-setup.c b/src/core/machine-id-setup.c
index 6710038..f3763ed 100644
--- a/src/core/machine-id-setup.c
+++ b/src/core/machine-id-setup.c
@@ -157,6 +157,37 @@ static int generate(char id[34], const char *root) {
return 0;
}
+static int get_valid_machine_id(int fd, char id[34]) {
+ char id_to_validate[34];
+
+ assert(fd >= 0);
+ assert(id);
+
+ if (loop_read(fd, id_to_validate, 33, false) == 33 && id_to_validate[32] == '\n') {
+ id_to_validate[32] = 0;
+
+ if (id128_is_valid(id_to_validate)) {
+ strncpy(id, id_to_validate, sizeof(id_to_validate));
+ id[32] = '\n';
+ id[33] = 0;
+ return 0;
+ }
+ }
+
+ return -EINVAL;
+}
+
+static int write_machine_id(int fd, char id[34]) {
+ assert(fd >= 0);
+ assert(id);
+ lseek(fd, 0, SEEK_SET);
+
+ if (loop_write(fd, id, 33, false) == 33)
+ return 0;
+
+ return -errno;
+}
+
int machine_id_setup(const char *root) {
const char *etc_machine_id, *run_machine_id;
_cleanup_close_ int fd = -1;
@@ -207,13 +238,8 @@ int machine_id_setup(const char *root) {
if (fstat(fd, &st) < 0)
return log_error_errno(errno, "fstat() failed: %m");
- if (S_ISREG(st.st_mode))
- if (loop_read(fd, id, 33, false) == 33 && id[32] == '\n') {
- id[32] = 0;
-
- if (id128_is_valid(id))
- return 0;
- }
+ if (S_ISREG(st.st_mode) && get_valid_machine_id(fd, id) == 0)
+ return 0;
/* Hmm, so, the id currently stored is not useful, then let's
* generate one */
@@ -223,9 +249,7 @@ int machine_id_setup(const char *root) {
return r;
if (S_ISREG(st.st_mode) && writable) {
- lseek(fd, 0, SEEK_SET);
-
- if (loop_write(fd, id, 33, false) == 33)
+ if (write_machine_id(fd, id) == 0)
return 0;
}
--
2.1.3

Lennart

[Didier Roche]

[Lennart Poettering]

+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 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...

+ 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.

Lennart

[Didier Roche]

Post by Lennart Poettering

+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.

Post by Lennart Poettering

+
+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.

Post by Lennart Poettering

+
+ /* 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
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.

Post by Lennart Poettering

+ 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)!

Post by Lennart Poettering

+ 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

[Lennart Poettering]

On Tue, 02.12.14 11:43, Didier Roche (***@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

[Didier Roche]

Post by Lennart Poettering
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

[Didier Roche]

[Lennart Poettering]

On Mon, 24.11.14 12:35, Didier Roche (***@canonical.com) wrote:

This one looks flawless to me!

Lennart

[Didier Roche]

[Didier Roche]

[Lennart Poettering]

On Mon, 24.11.14 12:36, Didier Roche (***@ubuntu.com) wrote:

The last two look great too!

Thanks,

Lennart