Hi Karel,
On Mon, Jun 1, 2015 at 2:07 PM, Karel Zak <k...@redhat.com> wrote:
> The current implementation directly monitor /proc/self/mountinfo and
> /run/mount/utab files. It's really not optimal because utab file is
> private libmount stuff without any official guaranteed semantic.
>
> The libmount since v2.26 provides API to monitor mount kernel &
> userspace changes. This patch replaces the current implementation with
> libmount based solution.
>
> Now the manager.h includes <libmount.h>, so $MOUNT_CFLAGS has been
> necessary to add to many tests CFLAGS.
>
> Note that mnt_monitor_event_cleanup() in v2.26 is broken, so the patch
> uses mnt_monitor_next_change(). It's exactly the same solution which
> uses the current libmount HEAD (mnt_monitor_event_cleanup() is API
> shorcut only).
Tiny nitpick below, otherwise look good to me.
Cheers,
Tom
> ---
> Makefile.am | 33 ++++++++++++------
> configure.ac | 2 +-
> src/core/manager.c | 2 +-
> src/core/manager.h | 5 ++-
> src/core/mount.c | 100
> ++++++++++++-----------------------------------------
> 5 files changed, 49 insertions(+), 93 deletions(-)
>
> diff --git a/Makefile.am b/Makefile.am
> index ed5135d..3815e72 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -1352,7 +1352,8 @@ systemd_SOURCES = \
>
> systemd_CFLAGS = \
> $(AM_CFLAGS) \
> - $(SECCOMP_CFLAGS)
> + $(SECCOMP_CFLAGS) \
> + $(MOUNT_CFLAGS)
>
> systemd_LDADD = \
> libsystemd-core.la \
> @@ -1554,7 +1555,8 @@ test_engine_SOURCES = \
>
> test_engine_CFLAGS = \
> $(AM_CFLAGS) \
> - $(SECCOMP_CFLAGS)
> + $(SECCOMP_CFLAGS) \
> + $(MOUNT_CFLAGS)
>
> test_engine_LDADD = \
> libsystemd-core.la \
> @@ -1565,7 +1567,8 @@ test_job_type_SOURCES = \
>
> test_job_type_CFLAGS = \
> $(AM_CFLAGS) \
> - $(SECCOMP_CFLAGS)
> + $(SECCOMP_CFLAGS) \
> + $(MOUNT_CFLAGS)
>
> test_job_type_LDADD = \
> libsystemd-core.la \
> @@ -1609,7 +1612,8 @@ test_unit_name_SOURCES = \
>
> test_unit_name_CFLAGS = \
> $(AM_CFLAGS) \
> - $(SECCOMP_CFLAGS)
> + $(SECCOMP_CFLAGS) \
> + $(MOUNT_CFLAGS)
>
> test_unit_name_LDADD = \
> libsystemd-core.la \
> @@ -1620,7 +1624,8 @@ test_unit_file_SOURCES = \
>
> test_unit_file_CFLAGS = \
> $(AM_CFLAGS) \
> - $(SECCOMP_CFLAGS)
> + $(SECCOMP_CFLAGS) \
> + $(MOUNT_CFLAGS)
>
> test_unit_file_LDADD = \
> libsystemd-core.la \
> @@ -1838,7 +1843,8 @@ test_tables_CPPFLAGS = \
>
> test_tables_CFLAGS = \
> $(AM_CFLAGS) \
> - $(SECCOMP_CFLAGS)
> + $(SECCOMP_CFLAGS) \
> + $(MOUNT_CFLAGS)
>
> test_tables_LDADD = \
> libsystemd-logs.la \
> @@ -1973,7 +1979,8 @@ test_cgroup_mask_SOURCES = \
> src/test/test-cgroup-mask.c
>
> test_cgroup_mask_CPPFLAGS = \
> - $(AM_CPPFLAGS)
> + $(AM_CPPFLAGS) \
> + $(MOUNT_CFLAGS)
>
> test_cgroup_mask_CFLAGS = \
> $(AM_CFLAGS) \
> @@ -2022,7 +2029,8 @@ test_path_SOURCES = \
> src/test/test-path.c
>
> test_path_CFLAGS = \
> - $(AM_CFLAGS)
> + $(AM_CFLAGS) \
> + $(MOUNT_CFLAGS)
>
> test_path_LDADD = \
> libsystemd-core.la
> @@ -2031,7 +2039,8 @@ test_execute_SOURCES = \
> src/test/test-execute.c
>
> test_execute_CFLAGS = \
> - $(AM_CFLAGS)
> + $(AM_CFLAGS) \
> + $(MOUNT_CFLAGS)
>
> test_execute_LDADD = \
> libsystemd-core.la
> @@ -2061,7 +2070,8 @@ test_sched_prio_SOURCES = \
> src/test/test-sched-prio.c
>
> test_sched_prio_CPPFLAGS = \
> - $(AM_CPPFLAGS)
> + $(AM_CPPFLAGS) \
> + $(MOUNT_CFLAGS)
>
> test_sched_prio_CFLAGS = \
> $(AM_CFLAGS) \
> @@ -2133,7 +2143,8 @@ systemd_analyze_SOURCES = \
>
> systemd_analyze_CFLAGS = \
> $(AM_CFLAGS) \
> - $(SECCOMP_CFLAGS)
> + $(SECCOMP_CFLAGS) \
> + $(MOUNT_CFLAGS)
>
> systemd_analyze_LDADD = \
> libsystemd-core.la \
> diff --git a/configure.ac b/configure.ac
> index 48cedb5..74ec386 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -454,7 +454,7 @@ AM_CONDITIONAL(HAVE_BLKID, [test "$have_blkid" = "yes"])
>
> #
> ------------------------------------------------------------------------------
> have_libmount=no
> -PKG_CHECK_MODULES(MOUNT, [ mount >= 2.20 ],
> +PKG_CHECK_MODULES(MOUNT, [ mount >= 2.26 ],
> [AC_DEFINE(HAVE_LIBMOUNT, 1, [Define if libmount is available])
> have_libmount=yes], have_libmount=no)
> if test "x$have_libmount" = xno; then
> AC_MSG_ERROR([*** libmount support required but libraries not found])
> diff --git a/src/core/manager.c b/src/core/manager.c
> index b931b0d..6881bb2 100644
> --- a/src/core/manager.c
> +++ b/src/core/manager.c
> @@ -567,7 +567,7 @@ int manager_new(ManagerRunningAs running_as, bool
> test_run, Manager **_m) {
>
> m->idle_pipe[0] = m->idle_pipe[1] = m->idle_pipe[2] =
> m->idle_pipe[3] = -1;
>
> - m->pin_cgroupfs_fd = m->notify_fd = m->signal_fd = m->time_change_fd
> = m->dev_autofs_fd = m->private_listen_fd = m->kdbus_fd = m->utab_inotify_fd
> = -1;
> + m->pin_cgroupfs_fd = m->notify_fd = m->signal_fd = m->time_change_fd
> = m->dev_autofs_fd = m->private_listen_fd = m->kdbus_fd = -1;
> m->current_job_id = 1; /* start as id #1, so that we can leave #0
> around as "null-like" value */
>
> m->ask_password_inotify_fd = -1;
> diff --git a/src/core/manager.h b/src/core/manager.h
> index 4ef869d..5cd8884 100644
> --- a/src/core/manager.h
> +++ b/src/core/manager.h
> @@ -23,6 +23,7 @@
>
> #include <stdbool.h>
> #include <stdio.h>
> +#include <libmount.h>
>
> #include "sd-bus.h"
> #include "sd-event.h"
> @@ -176,10 +177,8 @@ struct Manager {
> Hashmap *devices_by_sysfs;
>
> /* Data specific to the mount subsystem */
> - FILE *proc_self_mountinfo;
> + struct libmnt_monitor *mount_monitor;
> sd_event_source *mount_event_source;
> - int utab_inotify_fd;
> - sd_event_source *mount_utab_event_source;
>
> /* Data specific to the swap filesystem */
> FILE *proc_swaps;
> diff --git a/src/core/mount.c b/src/core/mount.c
> index ba1dcf1..3b6a519 100644
> --- a/src/core/mount.c
> +++ b/src/core/mount.c
> @@ -23,8 +23,6 @@
> #include <stdio.h>
> #include <sys/epoll.h>
> #include <signal.h>
> -#include <libmount.h>
> -#include <sys/inotify.h>
>
> #include "manager.h"
> #include "unit.h"
> @@ -1539,16 +1537,13 @@ static int mount_load_proc_self_mountinfo(Manager *m,
> bool set_flags) {
> }
>
> static void mount_shutdown(Manager *m) {
> +
> assert(m);
>
> m->mount_event_source = sd_event_source_unref(m->mount_event_source);
> - m->mount_utab_event_source =
> sd_event_source_unref(m->mount_utab_event_source);
>
> - if (m->proc_self_mountinfo) {
> - fclose(m->proc_self_mountinfo);
> - m->proc_self_mountinfo = NULL;
> - }
> - m->utab_inotify_fd = safe_close(m->utab_inotify_fd);
> + mnt_unref_monitor(m->mount_monitor);
> + m->mount_monitor = NULL;
> }
>
> static int mount_get_timeout(Unit *u, uint64_t *timeout) {
> @@ -1567,53 +1562,41 @@ static int mount_get_timeout(Unit *u, uint64_t
> *timeout) {
>
> static int mount_enumerate(Manager *m) {
> int r;
> +
> assert(m);
>
> mnt_init_debug(0);
>
> - if (!m->proc_self_mountinfo) {
> - m->proc_self_mountinfo = fopen("/proc/self/mountinfo", "re");
> - if (!m->proc_self_mountinfo)
> - return -errno;
> + if (!m->mount_monitor) {
> + int fd;
>
> - r = sd_event_add_io(m->event, &m->mount_event_source,
> fileno(m->proc_self_mountinfo), EPOLLPRI, mount_dispatch_io, m);
> - if (r < 0)
> + m->mount_monitor = mnt_new_monitor();
> + if (!m->mount_monitor) {
> + r = -ENOMEM;
> goto fail;
> + }
>
> - /* Dispatch this before we dispatch SIGCHLD, so that
> - * we always get the events from /proc/self/mountinfo
> - * before the SIGCHLD of /usr/bin/mount. */
> - r = sd_event_source_set_priority(m->mount_event_source, -10);
> - if (r < 0)
> + r = mnt_monitor_enable_kernel(m->mount_monitor, 1);
> + if (r)
> goto fail;
> -
> - (void)
> sd_event_source_set_description(m->mount_event_source,
> "mount-mountinfo-dispatch");
> - }
> -
> - if (m->utab_inotify_fd < 0) {
> - m->utab_inotify_fd = inotify_init1(IN_NONBLOCK|IN_CLOEXEC);
> - if (m->utab_inotify_fd < 0) {
> - r = -errno;
> + r = mnt_monitor_enable_userspace(m->mount_monitor, 1, NULL);
> + if (r)
> goto fail;
> - }
> -
> - (void) mkdir_p_label("/run/mount", 0755);
>
> - r = inotify_add_watch(m->utab_inotify_fd, "/run/mount",
> IN_MOVED_TO);
> - if (r < 0) {
> - r = -errno;
> + /* mnt_unref_monitor() will close the fd */
> + fd = r = mnt_monitor_get_fd(m->mount_monitor);
> + if (r < 0)
> goto fail;
> - }
>
> - r = sd_event_add_io(m->event, &m->mount_utab_event_source,
> m->utab_inotify_fd, EPOLLIN, mount_dispatch_io, m);
> + r = sd_event_add_io(m->event, &m->mount_event_source, fd,
> EPOLLIN | EPOLLPRI, mount_dispatch_io, m);
> if (r < 0)
> goto fail;
>
> - r = sd_event_source_set_priority(m->mount_utab_event_source,
> -10);
> + r = sd_event_source_set_priority(m->mount_event_source, -10);
> if (r < 0)
> goto fail;
>
> - (void)
> sd_event_source_set_description(m->mount_utab_event_source,
> "mount-utab-dispatch");
> + sd_event_source_set_description(m->mount_event_source,
> "mount-monitor-dispatch");
This should be cast to (void) unless you check the return.
> }
>
> r = mount_load_proc_self_mountinfo(m, false);
> @@ -1638,46 +1621,9 @@ static int mount_dispatch_io(sd_event_source *source,
> int fd, uint32_t revents,
> assert(m);
> assert(revents & (EPOLLPRI | EPOLLIN));
>
> - /* The manager calls this for every fd event happening on the
> - * /proc/self/mountinfo file, which informs us about mounting
> - * table changes, and for /run/mount events which we watch
> - * for mount options. */
> -
> - if (fd == m->utab_inotify_fd) {
> - bool rescan = false;
> -
> - /* FIXME: We *really* need to replace this with
> - * libmount's own API for this, we should not hardcode
> - * internal behaviour of libmount here. */
> -
> - for (;;) {
> - union inotify_event_buffer buffer;
> - struct inotify_event *e;
> - ssize_t l;
> -
> - l = read(fd, &buffer, sizeof(buffer));
> - if (l < 0) {
> - if (errno == EAGAIN || errno == EINTR)
> - break;
> -
> - log_error_errno(errno, "Failed to read utab
> inotify: %m");
> - break;
> - }
> -
> - FOREACH_INOTIFY_EVENT(e, buffer, l) {
> - /* Only care about changes to utab,
> - * but we have to monitor the
> - * directory to reliably get
> - * notifications about when utab is
> - * replaced using rename(2) */
> - if ((e->mask & IN_Q_OVERFLOW) ||
> streq(e->name, "utab"))
> - rescan = true;
> - }
> - }
> -
> - if (!rescan)
> - return 0;
> - }
> + if (fd == mnt_monitor_get_fd(m->mount_monitor))
> + /* Drain all changes from the monitor */
> + while (mnt_monitor_next_change(m->mount_monitor, NULL, NULL)
> == 0);
>
> r = mount_load_proc_self_mountinfo(m, true);
> if (r < 0) {
> --
> 2.4.1
>
> _______________________________________________
> systemd-devel mailing list
> systemd-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
