Avoid manual handling of loop devices #533
Merged
Conversation
This file contains 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
Cryptsetup is since 1.3.0 capable of setting up a loop device if the device argument is a file. This has the additional benefit that those loop devices will get the AUTOCLEAR flag (available with Linux 2.6.25). This means those loop devices will be closed as soon they're unused (on luksClose).
Especially directly after each other.
previously it had dedicated cases for listing all tombs and a singular one, which duplicated code. The function got reworked, that it uses a different approach for findmnt. Instead of filtering the general result, it now uses --source on the tomb specific crypsetup mapper. Those are searched via general globbing of the devices in /dev/mapper. This allows to combine the previous separate cases. Additionally remove the usage of _sudo for findmnt, as it is not necessary.
similar to list_tomb_mounts, rework the findmnt usage to usage of the actual tomb mapper device. Simplifies the awk usage and just only one argument needed for the mapper function.
Instead of only looking for bind mounts from within a tomb due to bind-hooks, also consider bind mounts that happenfrom the outside (example: open a tomb and manually issue a mount --bind /media/tomb some/other/location). Such a mount wouldn't be filtered before (only looking for an additional [/path/] added to TARGET. Instead look for every mount that is related to the respective /dev/mapper/ entry of a tomb and also close or list them. This helps to avoid to loop again against mounted tombs inside the main loop which loops over mounted tombs.
As the argument for list_tomb_mounts uses the input directly, it needs to be uniform. Therefore one must make sure that extraneous character like parentheses are removed from the variable. And those are in place in tombname for slam_tomb().
|
The PR did escalate a little bit while researching if the comment I previously mentioned still applies. And with all those changes in place this PR is ready from my side |
|
I like it on a first read: less code and more readable, good simplifying strategy. Are all corner cases observed covered by test units? I fhink is worth adding a few test units to lock in behavior as desired through future regressions. |
|
Indeed, a manual bind mount isn't covered. Will add that. On a sidenote: The test tool has seen activity again and is now at version 1.2.1. Maybe it makes sense to update the copy in the repo. Although it won't be a drop-in update. They changed some parts fundamentaly between 1.0.0 and the most recent version. |
|
when anyone of us has time to update sharness lets do it in a separate PR. oh and BTW I really appreciate your substantial help on maintenance chores and updates ❤️ |
it may happen, that someone bind mounts manually or via an immutable setup the tomb mountdir somewhere else. Tomb should be able to discover such mounts and close them if the tomb itself is closed.
|
Yeah, didn't intend to cram that into this PR :D But may help to open an issue. Should maybe be restructed, as adding this case didn't fit in the existing structure. Also noticed, that the bind test pollutes |
|
Hmm... the added test is working locally. Now to figure out was is different on these container images. |
|
Hmm... it failed at the original bind hook test, which I didn't touch beside the whitespace issue. It couldn't mount the test tomb because of the key? Password was okay, but now the key isn't available? Relevant part: |
|
The cancelled container confirms, that it works in general: Was later aborted due to the first failing? |
|
I just gave the tests a rerun and is now passing, perhaps some hiccup. I think this is an excellent PR! and has priority over others to be merged. I will also look into raising your privileges so that you can trigger reruns of the CI tests. Again many thanks Narrat, feels a bit like we are working side by side, still at different time and places 😃✌🏽 |
|
Thank you very much for the kind words :) I'm glad, that I can give something back. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Cryptsetup is since 1.3.0 capable of setting up a loop device if the device argument is a file.
This has the additional benefit that those loop devices will get the AUTOCLEAR flag (available with Linux 2.6.25). This means those loop devices will be closed as soon their're unused (on luksClose).
Read about something unrelated, stumbled upon cryptsetup usage without explicit losetup fiddling and thought maybe this is something for tomb to adopt.
Alternatively
lo_preserve()could be dropped and respective changes at other places still could apply.losetup -dremoves devices lazily. If it cannot be closed it won't reportEBUSYanymore, but instead setAUTOCLEAR(since Linux 3.7). Status can be seen vialosetup --list.Would be nice to get rid of
lo_mount(nowlo_check), but such a change would change behaviour (e.g. mapper name).Things to check: If the comment from
umount_tombtomb/tomb
Lines 3117 to 3122 in 75aafc0
would still apply with the suggested changes