Skip to content

feat(smartctl): update for smartmontools 7.5 #1471

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+107 −33
Open

feat(smartctl): update for smartmontools 7.5 #1471

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
140 changes: 107 additions & 33 deletions completions/smartctl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,33 +1,60 @@
# bash completion for smartctl(8) -*- shell-script -*-
# Updated for smartmontools 7.5 (released 2025-04-30)

_comp_cmd_smartctl__set_nospace_if_any()
{
local i
for i in "${!COMPREPLY[@]}"; do
# shellcheck disable=SC2053
if [[ ${COMPREPLY[i]} == $1 ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can fix the pattern (and remove the argument). The intent of the function is clearer this way.

Suggested change
if [[ ${COMPREPLY[i]} == $1 ]]; then
if [[ ${COMPREPLY[i]} == *[,=] ]]; then

The name of the function may also be adjusted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.

compopt -o nospace
break
fi
done
}
_comp_cmd_smartctl__device()
{
case $cur in
areca* | 3ware* | megaraid* | cciss*)
_comp_compgen -- -W '${cur%%,*},{0..31}'
3ware,* | areca,* | cciss,* | megaraid,*)
_comp_compgen -- -W '"${cur%%,*}",{0..31}'
;;
aacraid,*,*,*)
_comp_compgen -- -W 'aacraid,{0..3},{0..7},{0..31}'
;;
aacraid,*,*)
_comp_compgen -- -W 'aacraid,{0..3},{0..7},'
compopt -o nospace
;;
sssraid,*,*)
_comp_compgen -- -W 'sssraid,{0..3},{0..31}'
;;
aacraid,* | sssraid,*)
_comp_compgen -- -W '"${cur%%,*}",{0..3},'
compopt -o nospace
;;
Comment on lines +21 to +34
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about using _comp_compgen -P? Also, interleaving the completions for aacraid and sssraid seems a bit tricky to understand at a glance.

Suggested change
aacraid,*,*,*)
_comp_compgen -- -W 'aacraid,{0..3},{0..7},{0..31}'
;;
aacraid,*,*)
_comp_compgen -- -W 'aacraid,{0..3},{0..7},'
compopt -o nospace
;;
sssraid,*,*)
_comp_compgen -- -W 'sssraid,{0..3},{0..31}'
;;
aacraid,* | sssraid,*)
_comp_compgen -- -W '"${cur%%,*}",{0..3},'
compopt -o nospace
;;
aacraid,*,*,*,*) ;;
aacraid,*,*,*)
_comp_compgen -P "${cur%,*}," -- -W '{0..31}'
;;
aacraid,*,*)
_comp_compgen -P "${cur%,*}," -- -W '{0..7},'
compopt -o nospace
;;
aacraid,*)
_comp_compgen -P "${cur%,*}," -- -W '{0..3},'
compopt -o nospace
;;
sssraid,*,*,*) ::
sssraid,*,*)
_comp_compgen -P "${cur%,*}," -- -W '{0..31}'
;;
sssraid,*)
_comp_compgen -P "${cur%,*}," -- -W '{0..3},'
compopt -o nospace
;;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, no. This happily completes nonsense like:

$ /usr/sbin/smartctl -d aacraid,foo,bar,[TAB][TAB]
aacraid,foo,bar,0   aacraid,foo,bar,16  aacraid,foo,bar,23  aacraid,foo,bar,30
...
aacraid,foo,bar,15  aacraid,foo,bar,22  aacraid,foo,bar,3   aacraid,foo,bar,9

Copy link
Collaborator

@akinomyoga akinomyoga Oct 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I know that behavior, but I'm not sure whether we really need to care about the parts other than the currently completed part. Anyway, if you want to stop the completion when the previous part is wrong, it is also a valid decision. I was just concerned about the fact that aacraid,{0..3},{0..7},{0..31} internally generates 1000 candidates, which will finally be filtered out to less than or equal to 32 candidates always.

Copy link
Collaborator

@akinomyoga akinomyoga Oct 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Anyway, the point you raised can be easily fixed by slightly modifying the patterns in the suggested code, e.g. from aacraid,*,*,* to aacraid,[0-3],[0-7],*.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.

BTW, even much larger expansions are still reasonably fast:

someone@debian-12:~$ time bash -c 'x=($(echo aacraid,{0..7},{0..15},{0..127})) ; echo "${x[@]}" | wc'
      1   16384  254208
real    0m0.032s
user    0m0.031s
sys     0m0.004s

hpt,*/*/*)
_comp_compgen -- -W 'hpt,{1..4}/{1..8}/{1..4}'
;;
hpt,*/*)
_comp_compgen -- -W 'hpt,{1..4}/{1..8}{,/}'
compopt -o nospace
;;
hpt*)
_comp_compgen -- -W 'hpt,{1..4}/{1..8} hpt,{1..4}/{1..8}/{1..5}'
hpt,*)
_comp_compgen -- -W 'hpt,{1..4}/'
compopt -o nospace
;;
jmb39x,* | jmb39x-q,* | jmb39x-q2,* | jms56x,*)
_comp_compgen -- -W '"${cur%%,*}",{0..4}'
;;
*)
_comp_compgen -- -W 'ata scsi sat usbcypress usbjmicron usbsunplus
marvell areca 3ware hpt megaraid cciss auto test'
case "${COMPREPLY[@]}" in
areca | 3ware | hpt | megaraid | cciss)
compopt -o nospace
;;
esac
_comp_compgen -- -W 'auto test ata scsi nvme{,\,}
sat{,\,auto}{,\,12} usb{jmicron,prolific,sunplus}
snt{asmedia,jmicron,realtek}{,/sat} 3ware, aacraid, areca,
cciss, hpt, megaraid, sssraid, jmb39x{,-q,-q2}, jms56x,'
_comp_cmd_smartctl__set_nospace_if_any '*,'
;;
esac
}
_comp_cmd_smartctl__test()
{
[[ $cur == @(pending|scttempint|vendor), ]] && return
_comp_compgen -- -W 'offline short long conveyance select, select,redo
select,next afterselect,on afterselect,off pending, scttempint,
vendor,'
[[ ${COMPREPLY-} == *, ]] && compopt -o nospace
}
_comp_cmd_smartctl__drivedb()
{
local prefix=
Expand All @@ -38,13 +65,39 @@ _comp_cmd_smartctl__drivedb()
_comp_compgen_filedir h && [[ $prefix ]] &&
_comp_compgen -Rv COMPREPLY -- -P "$prefix" -W '"${COMPREPLY[@]}"'
}
_comp_cmd_smartctl__vendorattribute()
{
local fmt
case $cur in
[1-9N],* | [1-9][0-9],* | 1[0-9][0-9],* | 2[0-4][0-9],* | 25[0-5],*)
fmt='raw{8,16,48,56,64},hex{48,56,64},raw24/raw{24,32}'
Copy link
Collaborator

Choose a reason for hiding this comment

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

man smartctl in my environment (Arch Linux with smartctl 7.5) also contains raw16(raw16), raw16(avg16), and raw24(raw8). What are they, and why are they excluded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, good catch.

These options prints the raw value like 123 if the extra words are zero else like 123 (456 789) , see atacmds.cpp.

These were excluded because the (...) require shell quoting which didn't work. If added to _comp_compgen -- -W as raw16\\(raw16\\), it correctly returns raw16\(raw16\) but further completion does not work then. I also tried to pass raw16'('raw16')' with some \. Sorry if I missed something.

Please note that completeness is not very important here because these options were primarily added for the drivedb.h file and are rarely used from command line. Otherwise I wouldn't have chosen such syntax.

Copy link
Collaborator

Choose a reason for hiding this comment

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

it correctly returns raw16\(raw16\) but further completion does not work then.

I fetched your fork and checked the behavior. It seems the problem can be traced down to the fact that the compgen builtin performs the filtering under the assumption that the words specified to -W are the ones before the shell quoting.

$ compgen -W 'raw16\\(raw16\\)' -- 'raw16\('
$ compgen -W 'raw16\\(raw16\\)' -- 'raw16\\('
raw16\(raw16\)

One way is to generate it without quoting (i.e., -W 'raw16(raw16)') with compopt -o fullquote, but compopt -o fullquote is only available in Bash >= 5.3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bash >= 5.3 has been released end of July 2025. Even Ubuntu unstable ('questing') does not provide it yet. I suggest to exclude these three options for now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you leave code comments about the three values?

Copy link
Contributor Author

@chrfranke chrfranke Oct 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Of course, something like this?

        [1-9N],* | [1-9][0-9],* | 1[0-9][0-9],* | 2[0-4][0-9],* | 25[0-5],*)
            # 'raw16(raw16)', 'raw16(avg16)' and 'raw24(raw8)' are excluded
            # because these would require 'compopt -o fullquote' (Bash >= 5.3).
            # TODO: add these if Bash >= 5.3 is detected.
            fmt='raw{8,16,48,56,64},hex{48,56,64},raw24/raw{24,32}'

Copy link
Collaborator

@akinomyoga akinomyoga Oct 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, but for lower versions of Bash, I'm actually thinking about incorporating __bat_escape_completions (e.g. into our _comp_compgen with a flag -q), so you don't have to describe how it would be solved in 5.3. But if you want to, you can.

fmt+=',msec24hour32,{sec,min,halfmin}2hour,temp10x,tempminmax'
_comp_compgen -- -W '"${cur%%,*}",{'"$fmt"'}{,\,,}'
Copy link
Collaborator

Choose a reason for hiding this comment

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

{,\,,} generates two empty suffixes. I guess

Suggested change
_comp_compgen -- -W '"${cur%%,*}",{'"$fmt"'}{,\,,}'
_comp_compgen -- -W '"${cur%%,*}",{'"$fmt"'}{,\,,:}'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.

;;
[1-9] | [1-9][0-9] | 1[0-9][0-9] | 2[0-4][0-9] | 25[0-5])
_comp_compgen -- -W '{1..255},'
;;
*)
_comp_compgen -- -W 'N, help'
;;
esac
compopt -o nospace
}
_comp_cmd_smartctl__set()
{
_comp_compgen -- -W 'aam,{,off} apm,{,off} dsn,{on,off}
lookahead,{on,off} rcache,{on,off} security-freeze standby,{,off,now}
wcache,{on,off} wcache-sct,{ata,on,off}{,\,p}
wcreorder,{on,off}{,\,p} '"$1"
_comp_cmd_smartctl__set_nospace_if_any '*,'
}

_comp_cmd_smartctl()
{
local cur prev words cword was_split comp_args
_comp_initialize -s -- "$@" || return

local noargopts='!(-*|*[qdTbrnsoSlvFPBt]*)'
local noargopts='!(-*|*[qdTbrnoSlvFPBtfgs]*)'
# shellcheck disable=SC2254
case $prev in
--quietmode | -${noargopts}q)
Expand All @@ -64,32 +117,34 @@ _comp_cmd_smartctl()
return
;;
--report | -${noargopts}r)
_comp_compgen -- -W 'ioctl ataioctl scsiioctl'
_comp_compgen -- -W '{,ata,scsi,nvme}ioctl{,\,2}'
return
;;
--nocheck | -${noargopts}n)
_comp_compgen -- -W 'never sleep standby idle'
_comp_compgen -- -W 'never sleep{,\,} standby{,\,} idle{,\,}'
_comp_cmd_smartctl__set_nospace_if_any '*,'
return
;;
--smart | --offlineauto | --saveauto | -${noargopts}[soS])
--smart | --offlineauto | --saveauto | -${noargopts}[oS])
_comp_compgen -- -W 'on off'
return
;;
--log | -${noargopts}l)
_comp_compgen -- -W 'error selftest selective directory background
sasphy sasphy,reset sataphy sataphy,reset scttemp scttempsts
scttemphist scterc gplog smartlog xerror xselftest'
_comp_compgen -- -W 'background defects{,\,} devstat{,\,}
directory{,\,g,\,s} envrep error farm genstats gplog, nvmelog,
sasphy{,\,reset} sataphy{,\,reset} scterc{,\,,\,p,\,reset}
scttemp{,sts,int\,,hist} selective selftest smartlog, ssd
tapealert tapedevstat xerror{,\,,\,error}
xselftest{,\,,\,selftest} zdevstat'
_comp_cmd_smartctl__set_nospace_if_any '*,'
return
;;
--vendorattribute | -${noargopts}v)
_comp_compgen -- -W 'help 9,minutes 9,seconds 9,halfminutes 9,temp
192,emergencyretractcyclect 193,loadunload 194,10xCelsius
194,unknown 198,offlinescanuncsectorct 200,writeerrorcount
201,detectedtacount 220,temp'
Comment on lines -85 to -88
Copy link
Collaborator

Choose a reason for hiding this comment

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

The man page says "The following old arguments to '-v' are also still valid:". Shouldn't we suggest these for users who are familiar with the old ones? Or maybe we can generate these as a fallback in the case that no completions are generated for the new arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, no. It is planned for 8.0 to print a deprecation warning and later remove these in 8.2.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK.

_comp_cmd_smartctl__vendorattribute
return
;;
--firmwarebug | -${noargopts}F)
_comp_compgen -- -W 'none samsung samsung2 samsung3 swapid'
_comp_compgen -- -W 'none nologdir samsung{,2,3} swapid xerrorlba'
return
;;
--presets | -${noargopts}P)
Expand All @@ -101,7 +156,26 @@ _comp_cmd_smartctl()
return
;;
--test | -${noargopts}t)
_comp_cmd_smartctl__test
_comp_compgen -- -W 'afterselect,{on,off} conveyance force long
offline pending, scttempint, select,{,redo,next} short vendor,'
_comp_cmd_smartctl__set_nospace_if_any '*,'
return
;;
--format | -${noargopts}f)
_comp_compgen -- -W 'brief hex{,\,id,\,val} old'
return
;;
--get | -${noargopts}g)
_comp_compgen -- -W 'aam all apm dsn lookahead rcache security
wcache{,-sct} wcreorder'
return
;;
--set)
_comp_cmd_smartctl__set ''
return
;;
-${noargopts}s) # -s {on,off}: --smart {on,off}; -s OTHER: --set OTHER
_comp_cmd_smartctl__set 'on off'
return
;;
esac
Expand All @@ -110,7 +184,7 @@ _comp_cmd_smartctl()

if [[ $cur == -* ]]; then
_comp_compgen_help
[[ ${COMPREPLY-} == *= ]] && compopt -o nospace
_comp_cmd_smartctl__set_nospace_if_any '*='
else
_comp_compgen -c "${cur:-/dev/}" filedir
fi
Expand Down