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

[Didier Roche]

Le 01/12/2014 18:37, Lennart Poettering a écrit :
> 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.

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Add-a-machine_id_commit-call-to-commit-on-disk-a-tra.patch
Type: text/x-diff
Size: 6769 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20141202/9ea5b066/attachment.patch>