[systemd-devel] [PATCH 2/5] Add a machine_id_commit call to commit on disk a, transient machine-id

[Lennart Poettering]

On Mon, 24.11.14 12:35, Didier Roche (didrocks at 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 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

-- 
Lennart Poettering, Red Hat