Use libmount more by keszybz · Pull Request #12218 · systemd/systemd

keszybz · 2019-04-04T12:23:34Z

This might be a solution for #12018, once util-linux/util-linux#780 is resolved.

evverx · 2019-04-04T12:58:10Z

This pull request introduces 2 alerts when merging 90b4063 into 949082a - view on LGTM.com

new alerts:

1 for Missing header guard
1 for Comparison result is always the same

Comment posted by LGTM.com

keszybz · 2019-04-04T13:13:51Z

Force-pushed to fix the two issues pointed out by LGTM.

src/shared/mount-util.c

keszybz · 2019-04-05T09:13:00Z

You're right about the unescaping. I remember this was discussed when the current code was being merged, but for some reason the unescaping was left in, even though it shouldn't. Did maybe libmount behave differently in the past? Anyway, I added a patch to remove the unescaping in all places it was done, and adjusted the new patches to not do it. PTAL.

poettering · 2019-04-05T09:32:26Z

src/core/mount.c

@@ -11,7 +11,6 @@
 #include "dbus-mount.h"
 #include "dbus-unit.h"


this commit should probably be added to v242 still?

Maybe the first three (refactoring, test, and cunescape removal)? I can submit separate PR.

poettering · 2019-04-05T09:33:40Z

src/shared/mount-util.c

+                iter = mnt_new_iter(MNT_ITER_FORWARD);
+                if (!table || !iter)
+                        return log_oom();



still logs at non-debug level

This one is different, it was logging at non-debug level before, I just kept the behaviour.

but please. let's fix this when touching that

It seems you were right, and I added logging here, but then removed it in one of the later commits. I now fixed this during the rebase to have return -ENOMEM.

poettering · 2019-04-05T09:36:58Z

src/shared/mount-util.c

-                                        continue;
+                        if (!set_contains(done, path)) {
+                                r = set_put_strdup(todo, path);
                                if (r < 0)


needs to handle EEXIST gracefully, as before

set_put_strdup return 0 if the entry exists.

poettering · 2019-04-05T09:50:46Z

src/mount/mount-tool.c

-        for (;;) {
-                _cleanup_free_ char *path = NULL, *where = NULL, *dev = NULL;
-                int r;
+        r = mnt_table_parse_mtab(table, NULL);


maybe we should add a helper for this to libmount-util:

int mnt_table_proc_self_mount_info(struct libmnt_table **ret);

which allocates the table and opens it in one go. and maybe even go one step further and also allocate an iter at the same time and return that. After all we currently always do these three steps separately, always the same way

keszybz · 2019-04-05T13:13:04Z

Force pushed with a bugfix (wrong return value on error), and the suggested change to add a helper function. I also added one more commit to relax the test case which fails in CI.

keszybz · 2019-04-08T14:59:21Z

I think the CI failure is again caused by old libmount version. I hope it'll pass now.

poettering · 2019-04-08T15:22:37Z

src/shared/libmount-util.h

 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(


hmm, static inline? I think this should just be a regular function, it's complicated enough to warrant that.

This was explained in the commit message that adds the function. I'll repush for the other things, and then I'll try to make the explanation clearer.

poettering · 2019-04-08T15:23:17Z

src/shared/libmount-util.h

+
+        if (source)
+                /* Older libmount seems to require this. */
+                assert(path);


assert(!source || path)?

poettering · 2019-04-08T15:24:58Z

src/test/test-libmount.c


-        assert_se(mnt_table_next_fs(table, iter, &fs) == 0);
+        r = mnt_table_next_fs(table, iter, &fs);
+        if (r < 0 && may_fail) {


can you add a comment here maybe, that says "we allow this to fail in some tests, see below", or so?

keszybz · 2019-04-08T21:18:52Z

Updated:

the test is updated to reflect the changes in libmount that were merged upstream in the meantime
things are skipped more consistently in tests, and the requested comment has been added
the log_oom() thing pointed out in review is fixed
assert() is updated as requested

src/shared/mount-util.c

keszybz · 2019-04-10T12:07:34Z

bionic-i386 fails in two places: test/TEST-29-UDEV-ID_RENAMING test/TEST-30-ONCLOCKCHANGE.
TEST-29 fails with "systemd-testsuite login: Failed to wait for daemon to reply: Connection timed out", might be just a timeout.
TEST-30 is known flakey.
I'll force push to fix the errors pointed out by @mrc0mmand.

mrc0mmand · 2019-04-10T13:34:40Z

The Travis CI integration is having an aneurysm, so I triggered it manually: https://travis-ci.org/systemd/systemd/builds/517648222 ✅

poettering · 2019-04-12T11:30:22Z

src/shared/mount-util.c

                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);


hmm, so i wonder about the two uses in bind_remount_recursive_with_mountinfo() and umount_recursive(). We use this when setting up a namespace environment for a service. i.e. we operate on a different root than the real one. libmount is unlikely to understand that though and will merge data from /run/mount/utab into what is read here, but that's data about host mounts, not about mounts in the little namespaced environment we are setting up. Hence, we should somehow massage libmount into not loading utab meta info here if we want to use it in an entirely generic function like these two that should make no assumptions on the context they are called in.

@karelzak can you comment on this maybe? is there a way we can use the libmount mnt_table_xyz API without it reading utab for such namespaced environments?

@karelzak can you comment on this maybe? is there a way we can use the libmount mnt_table_xyz API without it reading utab for such namespaced environments?

Only mnt_table_parse_mtab() reads utab userspace junk.

So, all you need is to use mnt_table_parse_file(), mnt_new_table_from_file() or mnt_table_parse_stream() with "/proc/self/mountinfo" as path to the table.

And for libmount_monitor API -- the utab is enabled only if explicitly requested by mnt_monitor_enable_userspace().

Thanks. So it seems only umount_recursive() needs changing. bind_remount_recursive_with_mountinfo() already uses mnt_table_parse_stream(). I pushed one more commit to do this.

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.

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.

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.

See systemd#12218 (review).

See systemd/systemd#12218 (review).

keszybz added util-lib tests mount labels Apr 4, 2019

keszybz force-pushed the use-libmount-more branch from 90b4063 to 3f52e8b Compare April 4, 2019 13:13

poettering reviewed Apr 4, 2019

View reviewed changes

src/shared/mount-util.c Outdated Show resolved Hide resolved

poettering reviewed Apr 4, 2019

View reviewed changes

src/shared/mount-util.c Outdated Show resolved Hide resolved

poettering reviewed Apr 4, 2019

View reviewed changes

src/shared/mount-util.c Outdated Show resolved Hide resolved

poettering reviewed Apr 4, 2019

View reviewed changes

src/shared/mount-util.c Outdated Show resolved Hide resolved

poettering reviewed Apr 4, 2019

View reviewed changes

src/shared/mount-util.c Outdated Show resolved Hide resolved

poettering reviewed Apr 4, 2019

View reviewed changes

src/shared/mount-util.c Outdated Show resolved Hide resolved

poettering reviewed Apr 4, 2019

View reviewed changes

src/shared/mount-util.c Outdated Show resolved Hide resolved

poettering reviewed Apr 4, 2019

View reviewed changes

src/shared/mount-util.c Outdated Show resolved Hide resolved

keszybz force-pushed the use-libmount-more branch from 3f52e8b to d6e8280 Compare April 5, 2019 09:13

poettering reviewed Apr 5, 2019

View reviewed changes

keszybz added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Apr 5, 2019

keszybz force-pushed the use-libmount-more branch from d6e8280 to 92f1194 Compare April 5, 2019 13:11

keszybz added ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Apr 5, 2019

keszybz force-pushed the use-libmount-more branch from 92f1194 to a3eddfd Compare April 8, 2019 14:57

keszybz removed the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label Apr 8, 2019

keszybz mentioned this pull request Apr 8, 2019

Don't unescape paths from libmount #12252

Merged

poettering reviewed Apr 8, 2019

View reviewed changes

keszybz force-pushed the use-libmount-more branch from a3eddfd to ab57b19 Compare April 8, 2019 21:13

keszybz force-pushed the use-libmount-more branch from ab57b19 to 79d8329 Compare April 8, 2019 21:17

keszybz force-pushed the use-libmount-more branch from 79d8329 to 8cab8c6 Compare April 9, 2019 07:09

mrc0mmand reviewed Apr 10, 2019

View reviewed changes

src/shared/mount-util.c Outdated Show resolved Hide resolved

mrc0mmand reviewed Apr 10, 2019

View reviewed changes

src/shared/mount-util.c Outdated Show resolved Hide resolved

keszybz force-pushed the use-libmount-more branch from 8cab8c6 to 75b9abc Compare April 10, 2019 12:08

mrc0mmand mentioned this pull request Apr 10, 2019

TEST-30-ONCLOCKCHANGE is very flakey #12268

Closed

poettering reviewed Apr 12, 2019

View reviewed changes

yuwata added the needs-rebase label Apr 13, 2019

keszybz added 4 commits April 23, 2019 23:29

shared/mount-util: convert to libmount

13dcfe4

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.

shared/mount-util: make sure utab is ignored in umount_recursive()

2f2d81d

See systemd#12218 (review).

keszybz force-pushed the use-libmount-more branch from 75b9abc to 2f2d81d Compare April 23, 2019 21:55

keszybz removed the needs-rebase label Apr 23, 2019

poettering merged commit adb7b78 into systemd:master Apr 30, 2019

keszybz deleted the use-libmount-more branch May 3, 2019 08:23

edevolder pushed a commit to edevolder/systemd that referenced this pull request Jun 26, 2019

shared/mount-util: make sure utab is ignored in umount_recursive()

4a6f35c

See systemd#12218 (review).

Yamakuzure pushed a commit to elogind/elogind that referenced this pull request Sep 23, 2019

shared/mount-util: make sure utab is ignored in umount_recursive()

60aa8f9

See systemd/systemd#12218 (review).

		@@ -11,7 +11,6 @@
		#include "dbus-mount.h"
		#include "dbus-unit.h"

Uh oh!

Conversation

keszybz commented Apr 4, 2019

Uh oh!

evverx commented Apr 4, 2019

Uh oh!

keszybz commented Apr 4, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keszybz commented Apr 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keszybz commented Apr 5, 2019

Uh oh!

keszybz commented Apr 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keszybz commented Apr 8, 2019

Uh oh!

Uh oh!

Uh oh!

keszybz commented Apr 10, 2019

Uh oh!

mrc0mmand commented Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

mrc0mmand commented Apr 10, 2019 •

edited

Loading