Skip to content
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.

Sign up for GitHub

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

fix(cryptsetup): update the list of actions #759

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

fix(cryptsetup): update the list of actions #759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2a527ca
fix(cryptsetup): update the list of actions
akinomyoga Jun 14, 2022
deddbf5
fix(cryptsetup): parse --help and man for actions
akinomyoga Jun 17, 2022
830a868
test(cryptsetup): add a skip condition
akinomyoga Jun 18, 2022
1c5be70
test(cryptsetup): update the skip condition
akinomyoga Jun 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 46 additions & 4 deletions completions/cryptsetup
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,51 @@ _comp_cmd_cryptsetup__device()
_comp_compgen -c "${cur:-/dev/}" filedir
}

_comp_cmd_cryptsetup__action()
{
local REPLY IFS=$' \t\n'
_comp_dequote "${1-}" || return 1
local cmd=${REPLY:-cryptsetup}
local -a actions
_comp_split -l actions "$(
{
LC_ALL=C "$cmd" --help 2>&1 |
sed -n '/^<action> is one of:/,/^[^[:space:]]/s/^[[:space:]]\{1,\}\([^[:space:]]\{1,\}\).*/\1/p'
LC_ALL=C man cryptsetup 2>&1 |
awk '/^[[:space:]]+[[:alnum:]_]+([[:space:]]+(-[^[:space:].]+|<[^<>]+>|\[[^][]+\]|or))*$/ {print $1}'
Comment on lines +21 to +24
Copy link
Owner

@scop scop Jun 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add test cases for the parsing part here like we have for dnssec-keygen and xrandr. The test cases should verify that an exact set of things are extracted to make sure we don't get false positives.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, with this extraction, there is no guarantee that we do not hit false positives. I just tested it with the version with v2.4.3 in Fedora 36. If we care about the false positives, maybe we should filter out them using strings for the result as well as used in the fallback.

} | sort -u
)"

if ((${#actions[@]} == 0)); then
# The fallback action list is extracted from the following source:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should drop the fallback altogether, unless we know there is an old version we want to support but cannot because its output is not feasible to parse. If there is, we should document what exactly it is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just so lazy that I didn't want to check the behavior of old versions. Do we need to check the behavior of old versions of cryptsetup?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's kind of my point -- if we do check old versions and want to support them and can't find a way we like to make that happen otherwise, then I'm fine with a hardcoded list and the strings + grep approach. My opinion is that if we go to such lengths, we should document the reasoning and affected versions why we are doing it, why it is important here etc.

Without verifying from the sources, I believe our current standard is not to use fallback lists at all (we do have them in a few places, but it's not a standard), and the strings+grep verification is completely new, so I'm just validating if there's something exceptionally important here that warrants that stuff.

# https://gitlab.com/cryptsetup/cryptsetup/-/blob/main/src/cryptsetup.c#L3154-3208 (search for "Handle aliases")
# https://gitlab.com/cryptsetup/cryptsetup/-/blob/main/src/cryptsetup.c#L2831-2867 (search for "struct action_type")
# https://gitlab.com/cryptsetup/cryptsetup/-/blob/main/src/cryptsetup_args.h#L28-53 (see the macros "*_ACTION")
actions=(benchmark bitlkClose bitlkDump bitlkOpen close config convert
create erase isLuks loopaesClose loopaesOpen luksAddKey
luksChangeKey luksClose luksConfig luksConvertKey luksDump
luksErase luksFormat luksHeaderBackup luksHeaderRestore
luksKillSlot luksOpen luksRemoveKey luksResume luksSuspend luksUUID
open plainClose plainOpen reencrypt refresh remove repair resize
status tcryptClose tcryptDump tcryptOpen token)

# We attempt to filter the supported actions by the strings in the binary.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we decide to keep the fallback, I suggest we don't do this. It's not something we do elsewhere, and frankly I think it's too much for a corner case here.

Then again, if there is a bunch of other cases we feel we should do this in other completions, then a separate helper function for doing it would be in order, in another PR. Not quite sure myself, but open to opinions.

local path
if path=$(type -P -- "$cmd" 2>/dev/null || type -P -- cryptsetup 2>/dev/null); then
local filtering_pattern
printf -v filtering_pattern '%s\n' "${actions[@]}"
filtering_pattern=${filtering_pattern%$'\n'}

local filtered_actions
filtered_actions=$(strings "$path" | command grep -Fx "$filtering_pattern" | sort -u) &&
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we decide to keep this block or to extract this as a helper to another PR, I suppose this would be in order:

Suggested change
filtered_actions=$(strings "$path" | command grep -Fx "$filtering_pattern" | sort -u) &&
filtered_actions=$(strings "$path" 2>/dev/null | command grep -Fx "$filtering_pattern" | sort -u) &&

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if the fallback in this function finally gets into the repository, but such a function would be useful anyway where hardcoded lists are needed. I'll later consider it.

[[ $filtered_actions ]] &&
_comp_split -l actions "$filtered_actions"
fi
fi

_comp_compgen -- -W '"${actions[@]}"'
}

_comp_cmd_cryptsetup()
{
local cur prev words cword was_split comp_args
Expand Down Expand Up @@ -91,10 +136,7 @@ _comp_cmd_cryptsetup()
_comp_compgen_help
[[ ${COMPREPLY-} == *= ]] && compopt -o nospace
else
_comp_compgen -- -W 'open close resize status benchmark repair
erase luksFormat luksAddKey luksRemoveKey luksChangeKey
luksKillSlot luksUUID isLuks luksDump tcryptDump luksSuspend
luksResume luksHeaderBackup luksHeaderRestore'
_comp_cmd_cryptsetup__action "$1"
fi
fi

Expand Down
8 changes: 8 additions & 0 deletions test/t/test_cryptsetup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,11 @@ def test_1(self, completion):
@pytest.mark.complete("cryptsetup -", require_cmd=True)
def test_2(self, completion):
assert completion

@pytest.mark.complete(
"cryptsetup luksE",
require_cmd=True,
skipif=r'! { cryptsetup --help; man cryptsetup; } 2>/dev/null | command grep -qE "(^|[[:space:]])luksErase([[:space:]]|\$)"',
)
def test_github_issue758(self, completion):
assert completion == "rase"
Loading