-
-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use temporary directories for mounting #17
base: master
Are you sure you want to change the base?
Use temporary directories for mounting #17
Conversation
Instead, pass the mount location through an environment variable.
The `$clone` directory is already used when figuring out disk space for unmounted partitions, so this could fail on the first run.
This allows using the script multiple times in parallel on different disks. To prevent all these directories littering the filesystem, an exit handler is registered to remove them again when the script exits (also unmounting any mounted filesystems just in case).
rpi-clone
Outdated
export clone=$(mktemp --tmpdir --directory rpi-clone-dst.XXXXXX) | ||
export clone_src=$(mktemp --tmpdir --directory rpi-clone-src.XXXXXX) | ||
|
||
if ! [ -n "$clone" -a -d "$clone" -a -n "$clone_src" -a -d "$clone_src" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Negated tests are hard to read. I suggest to use the following test instead
if [[ -z "$clone" || ! -d "$clone" || -z "$clone_src" || ! -d "$clone_src" ]]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agreed, also using [[
as you suggest makes it even more readable by allowing ||
instead of -o
.
rpi-clone
Outdated
|
||
# Make sure we cleanup on exit, regardless of how we exit | ||
cleanup() { | ||
umount "$clone" 2> /dev/null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to redirect all output into the log clone_log
umount "$clone" &>> $clone_log
Same for rmdir.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good plan, I've also added a message to the log beforehand, to notify users that any umount errors might not necessarily be problematic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this PR 👍 Please see my comments :wink
Thanks or your review! I've pushed a fixup commit (to be autosquashed manually before merging this commit) with the changes you suggested. I have not been able to test these changes yet, I expect I'll be able to next wednesday or thursday. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This creates a temporary directory for src and dst mounts, rather than hardcoding /mnt/clone and /mnt/clone-src. This allows running the script twice in parallel, without conflicts.
To prevent littering tons of directories around, automatic cleanup of these directories is added (which has the added advantage of also unmounting and cleaning up when the script is somehow interrupted or fails in an unexpected way).
This also changes the interface to the rpi-clone-setup script slightly, since that also used to hardcode this directory name.
See commit messages for some more details.
These changes were previously submitted at billw2#102 and have been in use in my project since that PR was submitted four years ago. Now I've just rebased and reviewed them, the changes still applied without issue.