Use libmount in systemd-shutdown, add tests#8452
Merged
poettering merged 6 commits intosystemd:masterfrom Mar 20, 2018
Merged
Conversation
This one might actually might cause a crash.
medhefgo
reviewed
Mar 14, 2018
Contributor
medhefgo
left a comment
medhefgo
left a comment
There was a problem hiding this comment.
Yay. Moar homebrew parsing eliminated.
src/core/umount.c
Outdated
|
|
||
| m = new0(MountPoint, 1); | ||
| if (!m) | ||
| return -ENOMEM; |
Contributor
There was a problem hiding this comment.
For the cunescape call you log_oom().
|
@keszybz: this seems to avoid the crashes some of my users reported, however, there is still some off like: ... which affects mounts in |
|
As requested by @philmmanjaro, I am adding my information of my system which has one encrypted Successfull shutdown without any errors:
|
Contributor
|
@philmmanjaro Those look unrelated to me. Not that those log lines don't come from sd-shutdown. @Th3Z0ne Please boot with "printk.devkmsg=on loglevel=7" for full logs. |
|
The lines @philmmanjaro posted are from my system. crypttab contains two devices, both with options "luks,timeout=180" This is a more complete excerpt. |
Contributor
|
Once again, those are unrelated to this (or my) pull request. |
This is analogous to 8d3ae2b, except that now src/core/umount.c not src/core/mount.c is converted. Might help with https://bugzilla.redhat.com/show_bug.cgi?id=1554943, or not. In the patch, mnt_free_tablep and mnt_free_iterp are declared twice. It'd be nicer to define them just once in mount-setup.h, but then libmount.h would have to be included there. libmount.h seems to be buggy, and declares some defines which break other headers, and working around this is more pain than the two duplicate lines. So let's live with the duplication for now. This fixes memleak of MountPoint in mount_points_list_get() on error, not that it matters any.
The implementation seems buggy:
/* test_swap_list("/home/zbyszek/src/systemd/test/test-umount/example.swaps") */
path=0 o= f=0x0 try-ro=no dev=0:0
path=/some/swapfile2 o= f=0x0 try-ro=no dev=0:0
path=/some/swapfile o= f=0x0 try-ro=no dev=0:0
path=/dev/dm-2 o= f=0x0 try-ro=no dev=0:0
example.swaps with "(deleted)" does not cause bogus entries in the list now, but a memleak in libmount instead. The memleaks is not very important since this code is run just once. Reported as util-linux/util-linux#596. $ build/test-umount ... /* test_swap_list("/proc/swaps") */ path=/var/tmp/swap o= f=0x0 try-ro=no dev=0:0 path=/dev/dm-2 o= f=0x0 try-ro=no dev=0:0 /* test_swap_list("/home/zbyszek/src/systemd/test/test-umount/example.swaps") */ path=/some/swapfile o= f=0x0 try-ro=no dev=0:0 path=/dev/dm-2 o= f=0x0 try-ro=no dev=0:0 ==26912== ==26912== HEAP SUMMARY: ==26912== in use at exit: 16 bytes in 1 blocks ==26912== total heap usage: 1,546 allocs, 1,545 frees, 149,008 bytes allocated ==26912== ==26912== 16 bytes in 1 blocks are definitely lost in loss record 1 of 1 ==26912== at 0x4C31C15: realloc (vg_replace_malloc.c:785) ==26912== by 0x55C5D8C: _IO_vfscanf (in /usr/lib64/libc-2.26.so) ==26912== by 0x55D8AEC: vsscanf (in /usr/lib64/libc-2.26.so) ==26912== by 0x55D25C3: sscanf (in /usr/lib64/libc-2.26.so) ==26912== by 0x53236D0: mnt_table_parse_stream (in /usr/lib64/libmount.so.1.1.0) ==26912== by 0x53249B6: mnt_table_parse_file (in /usr/lib64/libmount.so.1.1.0) ==26912== by 0x10D157: swap_list_get (umount.c:194) ==26912== by 0x10B06E: test_swap_list (test-umount.c:34) ==26912== by 0x10B24B: main (test-umount.c:56) ==26912== ==26912== LEAK SUMMARY: ==26912== definitely lost: 16 bytes in 1 blocks ==26912== indirectly lost: 0 bytes in 0 blocks ==26912== possibly lost: 0 bytes in 0 blocks ==26912== still reachable: 0 bytes in 0 blocks ==26912== suppressed: 0 bytes in 0 blocks
keszybz
force-pushed
the
use-libmount-more
branch
from
c15310c to
71ae04c
Compare
Member
Author
|
@keszybz: according to @xabbu those errors got introduced by adopting cryptsetup v2.0 series. Building the current source against v1.7.5 removes those errors. So we have to see if cryptsetup itself or something in systemd should be modified/changed in that regard. However, this is not related to your current pull and work. |
Member
Author
|
Hmm, I see this error in my shutdown log too. Please open a new issue so we can gather all the info. |
Member
Author
|
#8472 is about the cryptsetup-related failures. |
poettering
reviewed
Mar 20, 2018
| #include <sys/swap.h> | ||
|
|
||
| /* This needs to be after sys/mount.h :( */ | ||
| #include <libmount.h> |
Member
There was a problem hiding this comment.
@karelzak hey, karel, this seems buggy in libmount.h!
Contributor
There was a problem hiding this comment.
Oooops, fixed -- the change will be in util-liux >= 2.32-rc2. Thanks for your report!
Contributor
There was a problem hiding this comment.
This patch karelzak@eb50b6d clean ups the includes to avoid collision between sys/mount.h (or libmount.h) and linux/fs.h. Works for me. See also #8507.
poettering
reviewed
Mar 20, 2018
| #include "virt.h" | ||
|
|
||
| DEFINE_TRIVIAL_CLEANUP_FUNC(struct libmnt_table*, mnt_free_table); | ||
| DEFINE_TRIVIAL_CLEANUP_FUNC(struct libmnt_iter*, mnt_free_iter); |
Member
There was a problem hiding this comment.
maybe add libmount-util.h for this?
Member
Author
There was a problem hiding this comment.
Let's wait until the situation with libmount.h is cleared.
poettering
reviewed
Mar 20, 2018
| r = cunescape(path, UNESCAPE_RELAX, &p); | ||
| if (r < 0) | ||
| return r; | ||
| if (cunescape(path, UNESCAPE_RELAX, &p) < 0) |
Member
There was a problem hiding this comment.
libmount doesn't do the unescaping on its own? it really should, @karelzak!
Contributor
There was a problem hiding this comment.
libmount decodes the paths of course... see unmangle in libmount code:
https://github.com/karelzak/util-linux/blob/master/libmount/src/tab_parse.c#L200
https://github.com/karelzak/util-linux/blob/master/lib/mangle.c#L51
Member
There was a problem hiding this comment.
Urks, then this patch is borked (and most likely some other code like it we already have elsewhere)
Contributor
There was a problem hiding this comment.
Note, I see unnecessary unescape() also in src/core/umount.c
Member
|
lgtm |
poettering
added
the
good-to-merge/waiting-for-ci 👍
Member
Author
|
@poettering it's been building for 4 days... waiting-for-ci doesn't make much sense. |
karelzak
added a commit
to util-linux/util-linux
that referenced
this pull request
Mar 20, 2018
Let's include sys/mount.h to be sure that our local libmount fallbacks are not used by default to avoid possible conflicts with later included sys/mount.h. Addresses: systemd/systemd#8452 Reported-by: Lennart Poettering <[email protected]> Signed-off-by: Karel Zak <[email protected]>
karelzak
added a commit
to util-linux/util-linux
that referenced
this pull request
Mar 20, 2018
Let's include sys/mount.h to be sure that our local libmount fallbacks are not used by default to avoid possible conflicts with later included sys/mount.h. Addresses: systemd/systemd#8452 Reported-by: Lennart Poettering <[email protected]> Signed-off-by: Karel Zak <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.