check-capture.sh: fix quoting and "ret=$?" warnings #1041
Conversation
Per issue #729, "Fix all shellcheck warnings". Prior to this commit there were 13 in this file, now zero.
| func_lib_lsof_error_dump $snd | ||
| if ! arecord_opts -D"$dev" -r "$rate" -c "$channel" -f "$fmt_elem" -d $duration "$file" -v -q; | ||
| then | ||
| func_lib_lsof_error_dump "$snd" |
There was a problem hiding this comment.
Nit: I prefer arecord_opts ... || { fail ... because it reads more like plain english and avoids a negation. Also, ! can't be copied to the command line (try it!)
There was a problem hiding this comment.
if this pr only fix shellcheck warnings, maybe we leave this to next PR.
| fi | ||
|
|
||
| for fmt_elem in $(echo $fmt) | ||
| for fmt_elem in $fmt |
There was a problem hiding this comment.
The previous code was really weird, I don't understand what it was trying to achieve so I'm a bit concerned that the fix seems "too simple". @aiChaoSONG you added this code in commit aedc851, can you please review this PR?
There was a problem hiding this comment.
@greg-intel can you please test the -F option before and after this PR and compare the logs, try to spot any difference?
There was a problem hiding this comment.
I didn't spot a difference, but I think maybe the device only had one format. I did (during testing) check behavior of this line when there were two items in the list, and it iterated over each individually. (I added " testing" to fmt, it ran the known value first and the second generated an error because it didn't exist.)
There was a problem hiding this comment.
I could not recall why it is $(echo $fmt), let's do a test see if $fmt is ok, if yes, it will be good then.
greg-intel
changed the title
Per issue #729, "Fix all shellcheck warnings".
Prior to this commit there were 13 issues in this file, now there are none.
check-capture.sh was run before and after the fixes, no change in output.
Signed off by Greg Galloway [email protected]