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
Fixes subvol delete on a non-btrfs volume #42272
Conversation
Inode numbers are guaranteed to be unique only within a filesystem. As such there is an edge case where these predicates are true on a non-btrfs filesystem. Closes moby#42271 Signed-off-by: Brett Milford <brettmilford@gmail.com>
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.
left a minor nit; but let me ask @tianon @AkihiroSuda to review
contrib/nuke-graph-directory.sh
Outdated
set -x | ||
btrfs subvolume delete "$subvol" | ||
) | ||
if [ "$(stat -f --format=%T $subvol)" == "btrfs" ]; then |
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 realize this script specified bash
, but I know we try to stay close to POSIX where possible, to allow running scripts in non-bash environments; perhaps change this to;
if [ "$(stat -f --format=%T $subvol)" == "btrfs" ]; then | |
if [ "$(stat -f --format=%T $subvol)" = "btrfs" ]; then |
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.
Full agreement from me 😇
I'd go one step further, too:
if [ "$(stat -f --format=%T $subvol)" == "btrfs" ]; then | |
if [ "$(stat -f --format=%T "$subvol")" = "btrfs" ]; then |
Or even (to catch any possible failing exit code from stat
):
if [ "$(stat -f --format=%T $subvol)" == "btrfs" ]; then | |
if subvolType="$(stat -f --format=%T "$subvol")" && [ "$subvolType" = "btrfs" ]; then |
contrib/nuke-graph-directory.sh
Outdated
@@ -61,10 +61,12 @@ if command -v btrfs > /dev/null 2>&1; then | |||
# Source: http://stackoverflow.com/a/32865333 | |||
for subvol in $(find "$dir" -type d -inum 256 | sort -r); do | |||
if [ "$dir" != "$subvol" ]; then |
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.
Oh, it's probably worth bringing that conditinoal all the way up to here, ala:
if [ "$dir" != "$subvol" ]; then | |
if [ "$dir" != "$subvol" ] && subvolType="$(stat -f --format=%T "$subvol")" && [ "$subvolType" = "btrfs" ]; then |
(So we only have a single if
)
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.
that's, nifty; and looks good to me.
Signed-off-by: Brett Milford <brettmilford@gmail.com>
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 😅
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
Inode numbers are guaranteed to be unique only within a filesystem.
As such there is an edge case where these predicates are true on a
non-btrfs filesystem.
Closes #42271
Signed-off-by: Brett Milford brettmilford@gmail.com
- What I did
Added an additional check for filesystem type.
- How to verify it
Tested this code block on btrfs and non-btrfs (zfs) filesystems.
- Description for the changelog
Fixed issue where
nuke-graph-directory.sh
executes btrfs subvolume delete on a non-btrfs file.