Re: [systemd-devel] 220 regression: path_is_mount_point() for non-directories
On Mon, 25.05.15 21:06, Martin Pitt (martin.p...@ubuntu.com) wrote: > Hello all, > > in 220, path_is_mount_point() now always fails with "-20 ENOTDIR" when > calling it on files. This is problematic as it's perfectly valid to > have bind-mounted files; in fact, systemd's machine_id_setup() itself > creates a /run/machine-id → /etc/machine-id bind mount if /etc is > read-only at that time (and systemd-machine-id-commit will write it > once root fs becomes writable). > > This was first introduced here: > > http://cgit.freedesktop.org/systemd/systemd/commit/?id=27cc6f166 > path-util: fix path_is_mount_point() for symlinks > > which added O_DIRECTORY, so that openat() fails with the above error. > It also replaced path_get_parent() (which works fine for files) with > name_to_handle_at(fd, "..", ...) which only works for directories. > > In fd_is_mount_point() we obviously don't have access to the file name > any more, and presumably we do want to keep it for efficiently > implementing the "rm-rf: never cross mount points" commit f25afeb. > > Since all possible checks in fd_is_mount_point() rely on *at(, "..") > which doesn't work for files, one solution that I see > is to add a fallback path_is_mountpoint() if fd_is_mount_point() fails > with ENOTDIR; in that case, should we implement all fallbacks again > (lots of duplicated code), or just one (st_dev comparison, which seems > the most widely supported one)? > > Alternatively we could pass a "parent_path" to fd_is_mount_point(), > compute that in path_is_mountpoint(), and don't specify one (or maybe > we even can specify it easily, I didn't check) in rm_rf_children() as > file bind mounts don't seem relevant there. This would be simpler, but > it would technically be an API break (unless we want to add > fd_is_mount_point_with_parent). Not an API break. Only calls prefixed with "sd_" are API, we never export any others. But yeah, path_is_mount_point() should be reworked to operate like openat() and similar calls, i.e. taking an fd to a dir, plus a filename below that dir, instead of directly and only an fd to the file below that dir. 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] 220 regression: path_is_mount_point() for non-directories
Martin Pitt [2015-05-25 21:06 +0200]: > Alternatively we could pass a "parent_path" to fd_is_mount_point(), > compute that in path_is_mountpoint(), and don't specify one (or maybe > we even can specify it easily, I didn't check) in rm_rf_children() as > file bind mounts don't seem relevant there. This would be simpler, but > it would technically be an API break (unless we want to add > fd_is_mount_point_with_parent). Ah no, it's just in src/shared, i. e. in libsystemd-shared.la, but not exported to libsystemd.so. Thus no API break. Martin -- Martin Pitt| http://www.piware.de Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org) ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] 220 regression: path_is_mount_point() for non-directories
Hello all, in 220, path_is_mount_point() now always fails with "-20 ENOTDIR" when calling it on files. This is problematic as it's perfectly valid to have bind-mounted files; in fact, systemd's machine_id_setup() itself creates a /run/machine-id → /etc/machine-id bind mount if /etc is read-only at that time (and systemd-machine-id-commit will write it once root fs becomes writable). This was first introduced here: http://cgit.freedesktop.org/systemd/systemd/commit/?id=27cc6f166 path-util: fix path_is_mount_point() for symlinks which added O_DIRECTORY, so that openat() fails with the above error. It also replaced path_get_parent() (which works fine for files) with name_to_handle_at(fd, "..", ...) which only works for directories. In fd_is_mount_point() we obviously don't have access to the file name any more, and presumably we do want to keep it for efficiently implementing the "rm-rf: never cross mount points" commit f25afeb. Since all possible checks in fd_is_mount_point() rely on *at(, "..") which doesn't work for files, one solution that I see is to add a fallback path_is_mountpoint() if fd_is_mount_point() fails with ENOTDIR; in that case, should we implement all fallbacks again (lots of duplicated code), or just one (st_dev comparison, which seems the most widely supported one)? Alternatively we could pass a "parent_path" to fd_is_mount_point(), compute that in path_is_mountpoint(), and don't specify one (or maybe we even can specify it easily, I didn't check) in rm_rf_children() as file bind mounts don't seem relevant there. This would be simpler, but it would technically be an API break (unless we want to add fd_is_mount_point_with_parent). Does anyone have a better idea? Thanks, Martin -- Martin Pitt| http://www.piware.de Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org) ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
