From 13dcfe4661b467131c943620d0f44711798bfd54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 4 Apr 2019 13:36:19 +0200 Subject: shared/mount-util: convert to libmount It seems better to use just a single parsing algorithm for /proc/self/mountinfo. Also, unify the naming of variables in all places that use mnt_table_next_fs(). It makes it easier to compare the different call sites. --- src/core/mount.c | 21 ++++---- src/shared/mount-util.c | 133 +++++++++++++++++++++--------------------------- src/shutdown/umount.c | 14 ++--- 3 files changed, 74 insertions(+), 94 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index b7fd35fc67..bb42955e84 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -1597,31 +1597,30 @@ static int mount_setup_unit( } static int mount_load_proc_self_mountinfo(Manager *m, bool set_flags) { - _cleanup_(mnt_free_tablep) struct libmnt_table *t = NULL; - _cleanup_(mnt_free_iterp) struct libmnt_iter *i = NULL; + _cleanup_(mnt_free_tablep) struct libmnt_table *table = NULL; + _cleanup_(mnt_free_iterp) struct libmnt_iter *iter = NULL; int r; assert(m); - t = mnt_new_table(); - i = mnt_new_iter(MNT_ITER_FORWARD); - if (!t || !i) + table = mnt_new_table(); + iter = mnt_new_iter(MNT_ITER_FORWARD); + if (!table || !iter) return log_oom(); - r = mnt_table_parse_mtab(t, NULL); + r = mnt_table_parse_mtab(table, NULL); if (r < 0) return log_error_errno(r, "Failed to parse /proc/self/mountinfo: %m"); for (;;) { struct libmnt_fs *fs; const char *device, *path, *options, *fstype; - int k; - k = mnt_table_next_fs(t, i, &fs); - if (k == 1) + r = mnt_table_next_fs(table, iter, &fs); + if (r == 1) break; - if (k < 0) - return log_error_errno(k, "Failed to get next entry from /proc/self/mountinfo: %m"); + if (r < 0) + return log_error_errno(r, "Failed to get next entry from /proc/self/mountinfo: %m"); device = mnt_fs_get_source(fs); path = mnt_fs_get_target(fs); diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index 680c4522ad..056106488d 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -8,16 +8,13 @@ #include #include -/* Include later */ -#include - #include "alloc-util.h" -#include "escape.h" #include "extract-word.h" #include "fd-util.h" #include "fileio.h" #include "fs-util.h" #include "hashmap.h" +#include "libmount-util.h" #include "mount-util.h" #include "mountpoint-util.h" #include "parse-util.h" @@ -36,52 +33,43 @@ int umount_recursive(const char *prefix, int flags) { * unmounting them until they are gone. */ do { - _cleanup_fclose_ FILE *proc_self_mountinfo = NULL; + _cleanup_(mnt_free_tablep) struct libmnt_table *table = NULL; + _cleanup_(mnt_free_iterp) struct libmnt_iter *iter = NULL; again = false; - r = fopen_unlocked("/proc/self/mountinfo", "re", &proc_self_mountinfo); + table = mnt_new_table(); + iter = mnt_new_iter(MNT_ITER_FORWARD); + if (!table || !iter) + return -ENOMEM; + + r = mnt_table_parse_mtab(table, NULL); if (r < 0) - return r; + return log_debug_errno(r, "Failed to parse /proc/self/mountinfo: %m"); for (;;) { - _cleanup_free_ char *path = NULL, *p = NULL; - int k; - - k = fscanf(proc_self_mountinfo, - "%*s " /* (1) mount id */ - "%*s " /* (2) parent id */ - "%*s " /* (3) major:minor */ - "%*s " /* (4) root */ - "%ms " /* (5) mount point */ - "%*s" /* (6) mount options */ - "%*[^-]" /* (7) optional fields */ - "- " /* (8) separator */ - "%*s " /* (9) file system type */ - "%*s" /* (10) mount source */ - "%*s" /* (11) mount options 2 */ - "%*[^\n]", /* some rubbish at the end */ - &path); - if (k != 1) { - if (k == EOF) - break; + struct libmnt_fs *fs; + const char *path; - continue; - } + r = mnt_table_next_fs(table, iter, &fs); + if (r == 1) + break; + if (r < 0) + return log_debug_errno(r, "Failed to get next entry from /proc/self/mountinfo: %m"); - k = cunescape(path, UNESCAPE_RELAX, &p); - if (k < 0) - return k; + path = mnt_fs_get_target(fs); + if (!path) + continue; - if (!path_startswith(p, prefix)) + if (!path_startswith(path, prefix)) continue; - if (umount2(p, flags) < 0) { - r = log_debug_errno(errno, "Failed to umount %s: %m", p); + if (umount2(path, flags) < 0) { + r = log_debug_errno(errno, "Failed to umount %s: %m", path); continue; } - log_debug("Successfully unmounted %s", p); + log_debug("Successfully unmounted %s", path); again = true; n++; @@ -91,7 +79,7 @@ int umount_recursive(const char *prefix, int flags) { } while (again); - return r < 0 ? r : n; + return n; } static int get_mount_flags(const char *path, unsigned long *flags) { @@ -141,6 +129,8 @@ int bind_remount_recursive_with_mountinfo( for (;;) { _cleanup_set_free_free_ Set *todo = NULL; + _cleanup_(mnt_free_tablep) struct libmnt_table *table = NULL; + _cleanup_(mnt_free_iterp) struct libmnt_iter *iter = NULL; bool top_autofs = false; char *x; unsigned long orig_flags; @@ -149,58 +139,52 @@ int bind_remount_recursive_with_mountinfo( if (!todo) return -ENOMEM; + table = mnt_new_table(); + iter = mnt_new_iter(MNT_ITER_FORWARD); + if (!table || !iter) + return -ENOMEM; + rewind(proc_self_mountinfo); - for (;;) { - _cleanup_free_ char *path = NULL, *p = NULL, *type = NULL; - int k; - - k = fscanf(proc_self_mountinfo, - "%*s " /* (1) mount id */ - "%*s " /* (2) parent id */ - "%*s " /* (3) major:minor */ - "%*s " /* (4) root */ - "%ms " /* (5) mount point */ - "%*s" /* (6) mount options (superblock) */ - "%*[^-]" /* (7) optional fields */ - "- " /* (8) separator */ - "%ms " /* (9) file system type */ - "%*s" /* (10) mount source */ - "%*s" /* (11) mount options (bind mount) */ - "%*[^\n]", /* some rubbish at the end */ - &path, - &type); - if (k != 2) { - if (k == EOF) - break; + r = mnt_table_parse_stream(table, proc_self_mountinfo, "/proc/self/mountinfo"); + if (r < 0) + return log_debug_errno(r, "Failed to parse /proc/self/mountinfo: %m"); - continue; - } + for (;;) { + struct libmnt_fs *fs; + const char *path, *type; - r = cunescape(path, UNESCAPE_RELAX, &p); + r = mnt_table_next_fs(table, iter, &fs); + if (r == 1) + break; if (r < 0) - return r; + return log_debug_errno(r, "Failed to get next entry from /proc/self/mountinfo: %m"); + + path = mnt_fs_get_target(fs); + type = mnt_fs_get_fstype(fs); + if (!path || !type) + continue; - if (!path_startswith(p, cleaned)) + if (!path_startswith(path, cleaned)) continue; - /* Ignore this mount if it is blacklisted, but only if it isn't the top-level mount we shall - * operate on. */ - if (!path_equal(cleaned, p)) { + /* Ignore this mount if it is blacklisted, but only if it isn't the top-level mount + * we shall operate on. */ + if (!path_equal(path, cleaned)) { bool blacklisted = false; char **i; STRV_FOREACH(i, blacklist) { - if (path_equal(*i, cleaned)) continue; if (!path_startswith(*i, cleaned)) continue; - if (path_startswith(p, *i)) { + if (path_startswith(path, *i)) { blacklisted = true; - log_debug("Not remounting %s blacklisted by %s, called for %s", p, *i, cleaned); + log_debug("Not remounting %s blacklisted by %s, called for %s", + path, *i, cleaned); break; } } @@ -215,15 +199,12 @@ int bind_remount_recursive_with_mountinfo( * already triggered, then we will find * another entry for this. */ if (streq(type, "autofs")) { - top_autofs = top_autofs || path_equal(cleaned, p); + top_autofs = top_autofs || path_equal(path, cleaned); continue; } - if (!set_contains(done, p)) { - r = set_consume(todo, p); - p = NULL; - if (r == -EEXIST) - continue; + if (!set_contains(done, path)) { + r = set_put_strdup(todo, path); if (r < 0) return r; } diff --git a/src/shutdown/umount.c b/src/shutdown/umount.c index 6afa512bff..ea9fba8831 100644 --- a/src/shutdown/umount.c +++ b/src/shutdown/umount.c @@ -55,18 +55,18 @@ void mount_points_list_free(MountPoint **head) { } int mount_points_list_get(const char *mountinfo, MountPoint **head) { - _cleanup_(mnt_free_tablep) struct libmnt_table *t = NULL; - _cleanup_(mnt_free_iterp) struct libmnt_iter *i = NULL; + _cleanup_(mnt_free_tablep) struct libmnt_table *table = NULL; + _cleanup_(mnt_free_iterp) struct libmnt_iter *iter = NULL; int r; assert(head); - t = mnt_new_table(); - i = mnt_new_iter(MNT_ITER_FORWARD); - if (!t || !i) + table = mnt_new_table(); + iter = mnt_new_iter(MNT_ITER_FORWARD); + if (!table || !iter) return log_oom(); - r = mnt_table_parse_mtab(t, mountinfo); + r = mnt_table_parse_mtab(table, mountinfo); if (r < 0) return log_error_errno(r, "Failed to parse %s: %m", mountinfo); @@ -79,7 +79,7 @@ int mount_points_list_get(const char *mountinfo, MountPoint **head) { bool try_remount_ro; _cleanup_free_ MountPoint *m = NULL; - r = mnt_table_next_fs(t, i, &fs); + r = mnt_table_next_fs(table, iter, &fs); if (r == 1) break; if (r < 0) -- cgit v1.2.1 From 7d991d4818dcf55916c1076003c3508c91df9934 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 4 Apr 2019 13:41:47 +0200 Subject: mount-tool: use libmount to parse /proc/self/mountinfo Same motivation as in other places: let's use a single logic to parse this. Use path_equal() to compare the path. A bug in error handling is fixed: if we failed after the GREEDY_REALLOC but before the line that sets the last item to NULL, we would jump to _cleanup_strv_free_ with the strv unterminated. Let's use GREEDY_REALLOC0 to avoid the issue. --- meson.build | 1 + src/mount/mount-tool.c | 63 +++++++++++++++++++++++--------------------------- 2 files changed, 30 insertions(+), 34 deletions(-) diff --git a/meson.build b/meson.build index 15e3394b91..d24e8000e9 100644 --- a/meson.build +++ b/meson.build @@ -2470,6 +2470,7 @@ exe = executable('systemd-mount', 'src/mount/mount-tool.c', include_directories : includes, link_with : [libshared], + dependencies: [libmount], install_rpath : rootlibexecdir, install : true) public_programs += exe diff --git a/src/mount/mount-tool.c b/src/mount/mount-tool.c index b290095b0e..fc18dd28d7 100644 --- a/src/mount/mount-tool.c +++ b/src/mount/mount-tool.c @@ -17,6 +17,7 @@ #include "format-util.h" #include "fs-util.h" #include "fstab-util.h" +#include "libmount-util.h" #include "main-func.h" #include "mount-util.h" #include "mountpoint-util.h" @@ -713,9 +714,11 @@ static int start_transient_automount( } static int find_mount_points(const char *what, char ***list) { - _cleanup_fclose_ FILE *proc_self_mountinfo = NULL; + _cleanup_(mnt_free_tablep) struct libmnt_table *table = NULL; + _cleanup_(mnt_free_iterp) struct libmnt_iter *iter = NULL; _cleanup_strv_free_ char **l = NULL; size_t bufsize = 0, n = 0; + int r; assert(what); assert(list); @@ -723,55 +726,47 @@ static int find_mount_points(const char *what, char ***list) { /* Returns all mount points obtained from /proc/self/mountinfo in *list, * and the number of mount points as return value. */ - proc_self_mountinfo = fopen("/proc/self/mountinfo", "re"); - if (!proc_self_mountinfo) - return log_error_errno(errno, "Can't open /proc/self/mountinfo: %m"); + table = mnt_new_table(); + iter = mnt_new_iter(MNT_ITER_FORWARD); + if (!table || !iter) + return log_oom(); - for (;;) { - _cleanup_free_ char *path = NULL, *where = NULL, *dev = NULL; - int r; + r = mnt_table_parse_mtab(table, NULL); + if (r < 0) + return log_error_errno(r, "Failed to parse /proc/self/mountinfo: %m"); - r = fscanf(proc_self_mountinfo, - "%*s " /* (1) mount id */ - "%*s " /* (2) parent id */ - "%*s " /* (3) major:minor */ - "%*s " /* (4) root */ - "%ms " /* (5) mount point */ - "%*s" /* (6) mount options */ - "%*[^-]" /* (7) optional fields */ - "- " /* (8) separator */ - "%*s " /* (9) file system type */ - "%ms" /* (10) mount source */ - "%*s" /* (11) mount options 2 */ - "%*[^\n]", /* some rubbish at the end */ - &path, &dev); - if (r != 2) { - if (r == EOF) - break; + for (;;) { + struct libmnt_fs *fs; + const char *source, *target; - continue; - } + r = mnt_table_next_fs(table, iter, &fs); + if (r == 1) + break; + if (r < 0) + return log_error_errno(r, "Failed to get next entry from /proc/self/mountinfo: %m"); - if (!streq(what, dev)) + source = mnt_fs_get_source(fs); + target = mnt_fs_get_target(fs); + if (!source || !target) continue; - r = cunescape(path, UNESCAPE_RELAX, &where); - if (r < 0) + if (!path_equal(source, what)) continue; /* one extra slot is needed for the terminating NULL */ - if (!GREEDY_REALLOC(l, bufsize, n + 2)) + if (!GREEDY_REALLOC0(l, bufsize, n + 2)) return log_oom(); - l[n++] = TAKE_PTR(where); + l[n] = strdup(target); + if (!l[n]) + return log_oom(); + n++; } - if (!GREEDY_REALLOC(l, bufsize, n + 1)) + if (!GREEDY_REALLOC0(l, bufsize, n + 1)) return log_oom(); - l[n] = NULL; *list = TAKE_PTR(l); - return n; } -- cgit v1.2.1 From e2857b3d87306d93f0fba526f3e79f4f6806fb02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 5 Apr 2019 14:43:06 +0200 Subject: Add helper function for mnt_table_parse_{stream,mtab} This wraps a few common steps. It is defined as inline function instead of in a .c file to avoid having a .c file. With a .c file, we would have three choices: - either link it into libshared, but then then libshared would have to be linked to libmount. - or compile the .c file into each target separately. This has the disdvantage that configuration of every target has to be updated and stuff will be compiled multiple times anyway, which is not too different from keeping this in the header file. - or create a new convenience library just for this. This also has the disadvantage that the every target would have to be updated, and a separate library for a 10 line function seems overkill. By keeping everything in a header file, we compile this a few times, but otherwise it's the least painful option. The compiler can optimize most of the function away, because it knows if 'source' is set or not. --- src/core/mount.c | 7 +------ src/mount/mount-tool.c | 7 +------ src/shared/libmount-util.h | 32 ++++++++++++++++++++++++++++++++ src/shared/mount-util.c | 14 ++------------ src/shutdown/umount.c | 7 +------ src/test/test-libmount.c | 5 +---- 6 files changed, 38 insertions(+), 34 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index bb42955e84..e32db2bf63 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -1603,12 +1603,7 @@ static int mount_load_proc_self_mountinfo(Manager *m, bool set_flags) { assert(m); - table = mnt_new_table(); - iter = mnt_new_iter(MNT_ITER_FORWARD); - if (!table || !iter) - return log_oom(); - - r = mnt_table_parse_mtab(table, NULL); + r = libmount_parse(NULL, NULL, &table, &iter); if (r < 0) return log_error_errno(r, "Failed to parse /proc/self/mountinfo: %m"); diff --git a/src/mount/mount-tool.c b/src/mount/mount-tool.c index fc18dd28d7..739f52c219 100644 --- a/src/mount/mount-tool.c +++ b/src/mount/mount-tool.c @@ -726,12 +726,7 @@ static int find_mount_points(const char *what, char ***list) { /* Returns all mount points obtained from /proc/self/mountinfo in *list, * and the number of mount points as return value. */ - table = mnt_new_table(); - iter = mnt_new_iter(MNT_ITER_FORWARD); - if (!table || !iter) - return log_oom(); - - r = mnt_table_parse_mtab(table, NULL); + r = libmount_parse(NULL, NULL, &table, &iter); if (r < 0) return log_error_errno(r, "Failed to parse /proc/self/mountinfo: %m"); diff --git a/src/shared/libmount-util.h b/src/shared/libmount-util.h index 7d94468e52..6e8e61ed6c 100644 --- a/src/shared/libmount-util.h +++ b/src/shared/libmount-util.h @@ -1,6 +1,8 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #pragma once +#include + /* This needs to be after sys/mount.h */ #include @@ -8,3 +10,33 @@ DEFINE_TRIVIAL_CLEANUP_FUNC(struct libmnt_table*, mnt_free_table); DEFINE_TRIVIAL_CLEANUP_FUNC(struct libmnt_iter*, mnt_free_iter); + +static inline int libmount_parse( + const char *path, + FILE *source, + struct libmnt_table **ret_table, + struct libmnt_iter **ret_iter) { + + _cleanup_(mnt_free_tablep) struct libmnt_table *table = NULL; + _cleanup_(mnt_free_iterp) struct libmnt_iter *iter = NULL; + int r; + + /* Older libmount seems to require this. */ + assert(!source || path); + + table = mnt_new_table(); + iter = mnt_new_iter(MNT_ITER_FORWARD); + if (!table || !iter) + return -ENOMEM; + + if (source) + r = mnt_table_parse_stream(table, source, path); + else + r = mnt_table_parse_mtab(table, path); + if (r < 0) + return r; + + *ret_table = TAKE_PTR(table); + *ret_iter = TAKE_PTR(iter); + return 0; +} diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index 056106488d..97cc7b44c5 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -38,12 +38,7 @@ int umount_recursive(const char *prefix, int flags) { again = false; - table = mnt_new_table(); - iter = mnt_new_iter(MNT_ITER_FORWARD); - if (!table || !iter) - return -ENOMEM; - - r = mnt_table_parse_mtab(table, NULL); + r = libmount_parse(NULL, NULL, &table, &iter); if (r < 0) return log_debug_errno(r, "Failed to parse /proc/self/mountinfo: %m"); @@ -139,14 +134,9 @@ int bind_remount_recursive_with_mountinfo( if (!todo) return -ENOMEM; - table = mnt_new_table(); - iter = mnt_new_iter(MNT_ITER_FORWARD); - if (!table || !iter) - return -ENOMEM; - rewind(proc_self_mountinfo); - r = mnt_table_parse_stream(table, proc_self_mountinfo, "/proc/self/mountinfo"); + r = libmount_parse("/proc/self/mountinfo", proc_self_mountinfo, &table, &iter); if (r < 0) return log_debug_errno(r, "Failed to parse /proc/self/mountinfo: %m"); diff --git a/src/shutdown/umount.c b/src/shutdown/umount.c index ea9fba8831..5b1160833b 100644 --- a/src/shutdown/umount.c +++ b/src/shutdown/umount.c @@ -61,12 +61,7 @@ int mount_points_list_get(const char *mountinfo, MountPoint **head) { assert(head); - table = mnt_new_table(); - iter = mnt_new_iter(MNT_ITER_FORWARD); - if (!table || !iter) - return log_oom(); - - r = mnt_table_parse_mtab(table, mountinfo); + r = libmount_parse(mountinfo, NULL, &table, &iter); if (r < 0) return log_error_errno(r, "Failed to parse %s: %m", mountinfo); diff --git a/src/test/test-libmount.c b/src/test/test-libmount.c index fc28f27d53..c3395493d4 100644 --- a/src/test/test-libmount.c +++ b/src/test/test-libmount.c @@ -21,13 +21,10 @@ static void test_libmount_unescaping_one( _cleanup_(mnt_free_iterp) struct libmnt_iter *iter = NULL; _cleanup_fclose_ FILE *f = NULL; - assert_se(table = mnt_new_table()); - assert_se(iter = mnt_new_iter(MNT_ITER_FORWARD)); - f = fmemopen((char*) string, strlen(string), "re"); assert_se(f); - assert_se(mnt_table_parse_stream(table, f, title) >= 0); + assert_se(libmount_parse(title, f, &table, &iter) >= 0); struct libmnt_fs *fs; const char *source, *target; -- cgit v1.2.1 From 2f2d81d957b2d5ef7a0f8a702dcf7f71348cffe3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 23 Apr 2019 23:52:15 +0200 Subject: shared/mount-util: make sure utab is ignored in umount_recursive() See https://github.com/systemd/systemd/pull/12218#pullrequestreview-226029985. --- src/shared/libmount-util.h | 7 ++++++- src/shared/mount-util.c | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/shared/libmount-util.h b/src/shared/libmount-util.h index 6e8e61ed6c..7c3b855df5 100644 --- a/src/shared/libmount-util.h +++ b/src/shared/libmount-util.h @@ -29,10 +29,15 @@ static inline int libmount_parse( if (!table || !iter) return -ENOMEM; + /* If source or path are specified, we use on the functions which ignore utab. + * Only if both are empty, we use mnt_table_parse_mtab(). */ + if (source) r = mnt_table_parse_stream(table, source, path); + else if (path) + r = mnt_table_parse_file(table, path); else - r = mnt_table_parse_mtab(table, path); + r = mnt_table_parse_mtab(table, NULL); if (r < 0) return r; diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index 97cc7b44c5..d84f28250d 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -38,7 +38,7 @@ int umount_recursive(const char *prefix, int flags) { again = false; - r = libmount_parse(NULL, NULL, &table, &iter); + r = libmount_parse("/proc/self/mountinfo", NULL, &table, &iter); if (r < 0) return log_debug_errno(r, "Failed to parse /proc/self/mountinfo: %m"); -- cgit v1.2.1