| Message ID | 20231212074228.1261164-2-lizhijian@fujitsu.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ndctl,v2,1/2] test/cxl-region-sysfs.sh: use '[[ ]]' command to evaluate operands as arithmetic expressions | expand |
On Tue, Dec 12, 2023 at 03:42:28PM +0800, Li Zhijian wrote: > A space is missing before ']' What's happens when that space is missing? That's partly a request to add an impact statement, but also for my education as I'm just learning all this shellcheck stuff too. BTW - if any of this was found using a tool, rather than by inspection, please include a note of the tool used. Thanks, Alison > > Acked-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Li Zhijian <lizhijian@fujitsu.com> > --- > test/cxl-region-sysfs.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/test/cxl-region-sysfs.sh b/test/cxl-region-sysfs.sh > index 6a5da6d..db1a163 100644 > --- a/test/cxl-region-sysfs.sh > +++ b/test/cxl-region-sysfs.sh > @@ -104,7 +104,7 @@ do > iw=$(cat /sys/bus/cxl/devices/$i/interleave_ways) > ig=$(cat /sys/bus/cxl/devices/$i/interleave_granularity) > [ $iw -ne $nr_targets ] && err "$LINENO: decoder: $i iw: $iw targets: $nr_targets" > - [ $ig -ne $r_ig] && err "$LINENO: decoder: $i ig: $ig root ig: $r_ig" > + [ $ig -ne $r_ig ] && err "$LINENO: decoder: $i ig: $ig root ig: $r_ig" > > sz=$(cat /sys/bus/cxl/devices/$i/size) > res=$(cat /sys/bus/cxl/devices/$i/start) > -- > 2.41.0 > >
Li Zhijian wrote: > A space is missing before ']' > > Acked-by: Dan Williams <dan.j.williams@intel.com> You added my Ack without addressing the feedback. "Commentary on the impact of the change is always welcome." Please include a sentence on how this triggered for you and some analysis of the why it has not triggered previously. Otherwise "A space is missing before ']'" does not add any information that can not be determined by reading the patch. A useful changelog for this would be something like: Currently the cxl-region-sysfs.sh test runs to completion and passes, but with syntax errors in the log. It turns out that because the test is checking for a positive condition as a failure, that also happens to mask the syntax errors. Fix the syntax and note that this also happens to unblock a test case that was being hidden by this error.
On 13/12/2023 06:46, Dan Williams wrote: > Li Zhijian wrote: >> A space is missing before ']' >> >> Acked-by: Dan Williams <dan.j.williams@intel.com> > > You added my Ack without addressing the feedback. I'm sorry about that. I thought this fix(syntax error) is too simple to say more. > > "Commentary on the impact of the change is always welcome." > > Please include a sentence on how this triggered for you and some > analysis of the why it has not triggered previously. Otherwise "A space > is missing before ']'" does not add any information that can not be > determined by reading the patch. > > A useful changelog for this would be something like: > > Currently the cxl-region-sysfs.sh test runs to completion and passes, > but with syntax errors in the log. It turns out that because the test is > checking for a positive condition as a failure, that also happens to > mask the syntax errors. Fix the syntax and note that this also happens > to unblock a test case that was being hidden by this error. thanks, it's a pretty good changelog. will update in V3. Thanks Zhijian
On 13/12/2023 04:54, Alison Schofield wrote: > On Tue, Dec 12, 2023 at 03:42:28PM +0800, Li Zhijian wrote: >> A space is missing before ']' > > What's happens when that space is missing? It's a syntax error, so '[ $ig -ne $r_ig]' is always false, it may hide the a real error. > That's partly a request to add an impact statement, but also > for my education as I'm just learning all this shellcheck > stuff too. shellcheck is able to inspect this error. > > BTW - if any of this was found using a tool, rather than > by inspection, please include a note of the tool used. This time, they are all found by eyes :) when i'm checking the full log when i met an error. Thanks Zhijian > > Thanks, > Alison > >> >> Acked-by: Dan Williams <dan.j.williams@intel.com> >> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com> >> --- >> test/cxl-region-sysfs.sh | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/test/cxl-region-sysfs.sh b/test/cxl-region-sysfs.sh >> index 6a5da6d..db1a163 100644 >> --- a/test/cxl-region-sysfs.sh >> +++ b/test/cxl-region-sysfs.sh >> @@ -104,7 +104,7 @@ do >> iw=$(cat /sys/bus/cxl/devices/$i/interleave_ways) >> ig=$(cat /sys/bus/cxl/devices/$i/interleave_granularity) >> [ $iw -ne $nr_targets ] && err "$LINENO: decoder: $i iw: $iw targets: $nr_targets" >> - [ $ig -ne $r_ig] && err "$LINENO: decoder: $i ig: $ig root ig: $r_ig" >> + [ $ig -ne $r_ig ] && err "$LINENO: decoder: $i ig: $ig root ig: $r_ig" >> >> sz=$(cat /sys/bus/cxl/devices/$i/size) >> res=$(cat /sys/bus/cxl/devices/$i/start) >> -- >> 2.41.0 >> >> >
diff --git a/test/cxl-region-sysfs.sh b/test/cxl-region-sysfs.sh index 6a5da6d..db1a163 100644 --- a/test/cxl-region-sysfs.sh +++ b/test/cxl-region-sysfs.sh @@ -104,7 +104,7 @@ do iw=$(cat /sys/bus/cxl/devices/$i/interleave_ways) ig=$(cat /sys/bus/cxl/devices/$i/interleave_granularity) [ $iw -ne $nr_targets ] && err "$LINENO: decoder: $i iw: $iw targets: $nr_targets" - [ $ig -ne $r_ig] && err "$LINENO: decoder: $i ig: $ig root ig: $r_ig" + [ $ig -ne $r_ig ] && err "$LINENO: decoder: $i ig: $ig root ig: $r_ig" sz=$(cat /sys/bus/cxl/devices/$i/size) res=$(cat /sys/bus/cxl/devices/$i/start)