On 2025-08-13 21:05, Alison Schofield wrote: > On Wed, Aug 13, 2025 at 06:28:14PM -0700, Marc Herbert wrote: >> Reviewing only the shell language part, not the CXL logic. >> >> On 2025-08-04 02:01, alison.schofi...@intel.com wrote: >> >>> +# Allow what shellcheck suspects are unused - the arrays >>> +# shellcheck disable=SC2034 >> >> That's very indiscriminate and shellcheck -x is usually pretty good >> there... did you use -x? > > Yes. Shellcheck -x doesn't ignore SC2034.
It does not, but -x is required to fix SC2034 when declaration and usage are split across files. It's indeed off-topic for these arrays, sorry about that. On the other hand, it does apply to the `rc` variable in this file. See complete patch at the bottom. >> Alternatively, could you place this only before the arrays that >> shellcheck gets wrong for some reason? (which reason?) > > I considered and chose the global disable for the arrays and commented > same. The syntax is valid bash syntax that shellcheck has not caught up > to understanding yet. So I got curious and just learned that shellcheck has made the deliberate choice to ignore references / indirections. Interesting! https://www.shellcheck.net/wiki/SC2034 If nothing else, the comment should be a bit more specific and mention the relevant "nameref" or "indirection" keyword. I haven't seen name references used much, most people seem to "brute-force" this with an 'eval' instead (which makes SC2034 obvious), but the former are much more elegant! Yet not elegant enough for shellcheck :-) > There is always this option to see what may be masked by shellcheck > disables: > > grep -v -E '^\s*#\s*shellcheck\s+disable=' cxl-translate.sh | shellcheck -x - Mmm... that's not very convenient. I found a selective and concise fix, see below. This is a useful warning. >> >>> + nr_pass=$(echo "$log" | grep -c "CXL Translate Test.*PASS") || nr_pass=0 >>> + nr_fail=$(echo "$log" | grep -c "CXL Translate Test.*FAIL") || >>> nr_fail=0 >> >> Not sure about reading the entire log in memory. Also not sure about >> size limit with variables... How about something like this instead: >> >> local jnl_cmd='journalctl --reverse -k --grep="CXL Translate Test" >> --since '"$NDTEST_START" >> local nr_pass; nr_pass=$($jnl_cmd | grep 'PASS' | wc -l) >> local nr_fail; nr_pass=$($jnl_cmd | grep 'FAIL' | wc -l) > This is reading the dmesg log per test_table or test_sample_set. > There are currently 6 data sets that are run thru the test and results > checked per data set. Each set has an expected number of PASS or FAIL. > I'm pretty sure any one log is much smaller than any log we typically > generate just by loading the cxl-test module in other cxl-tests. Thanks! It still looks inefficient to me not to use the --grep feature built-in journalctl but if it's that small then I agree it does not matter. > I don't expect to use the general check_dmesg() here because anything > this test wants to evaluate is in the logs it reads after each data > set. I had understood at least that :-) >> >> >>> + if [ "$expect_failures" = "false" ]; then >> >> if "$expect_failures"; then > > Is the existing pattern wrong or is that an ask for brevity? Yes, just for brevity and clarity. I mean you would not write "if (some_bool == false)" in another language. Not important. --- a/test/cxl-translate.sh +++ b/test/cxl-translate.sh @@ -2,10 +2,8 @@ # SPDX-License-Identifier: GPL-2.0 # Copyright (C) 2025 Intel Corporation. All rights reserved. -# Allow what shellcheck suspects are unused - the arrays -# shellcheck disable=SC2034 - # source common to get the err() +# shellcheck source=test/common . "$(dirname "$0")"/common trap 'err $LINENO' ERR @@ -140,6 +138,7 @@ generate_sample_tests() { test_sample_sets() { local sample_name=$1 + # shellcheck disable=SC2034 local -n sample_set=$1 local generated_tests=() @@ -283,6 +282,12 @@ test_tables() { check_dmesg_results "${#table_ref[@]}" "$expect_failures" } +# Used only as "nameref" +# shellcheck disable=SC2034 +declare -a Expect_Fail_Table XOR_Table_4R_4H XOR_Table_8R_4H XOR_Table_12R_12H +# shellcheck disable=SC2034 +declare -A Sample_4R_4H Sample_12R_12H + test_tables Expect_Fail_Table true test_sample_sets Sample_4R_4H test_sample_sets Sample_12R_12H
