| Message ID | 20210804120446.32835-2-aleksei.kodanev@bell-sw.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/3] network/tcp_cc: check sch_netem driver in the lib | expand |
Hi Alexey, > busybox/sysctl expects -e option to be set before 'name=value'. > This can easily be fixed by splitting the string in tst_set_sysctl(), > so that 'rparam' with '-e' option is added in between. Good catch. Reviewed-by: Petr Vorel <pvorel@suse.cz> ... > diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh > index 511fb7eb1..bb30c13ec 100644 > --- a/testcases/lib/tst_net.sh > +++ b/testcases/lib/tst_net.sh > @@ -918,7 +918,7 @@ tst_set_sysctl() > local rparam= > [ "$TST_USE_NETNS" = "yes" ] && rparam="-r '-e'" > - tst_net_run $safe $rparam "sysctl -q -w $name=$value" > + tst_net_run $safe $rparam "sysctl -q -w" "$name=$value" I wonder if it were a bit clearer if we moves all params to second arg: tst_net_run $safe "sysctl" "$name=$value -q -w" Kind regards, Petr
On 20.08.2021 13:43, Petr Vorel wrote: > Hi Alexey, > >> busybox/sysctl expects -e option to be set before 'name=value'. >> This can easily be fixed by splitting the string in tst_set_sysctl(), >> so that 'rparam' with '-e' option is added in between. > Good catch. > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > ... >> diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh >> index 511fb7eb1..bb30c13ec 100644 >> --- a/testcases/lib/tst_net.sh >> +++ b/testcases/lib/tst_net.sh >> @@ -918,7 +918,7 @@ tst_set_sysctl() >> local rparam= >> [ "$TST_USE_NETNS" = "yes" ] && rparam="-r '-e'" > >> - tst_net_run $safe $rparam "sysctl -q -w $name=$value" >> + tst_net_run $safe $rparam "sysctl -q -w" "$name=$value" > > I wonder if it were a bit clearer if we moves all params to second arg: > tst_net_run $safe "sysctl" "$name=$value -q -w" OK, but changed to "sysctl" "-q -w $name=$value" because the other options should also be placed before the key/value pair: Usage: sysctl -p [-enq] [FILE...] / [-enqaw] [KEY[=VALUE]]... Thanks for review Petr! Applied the patches.
Hi Alexey, .. > >> - tst_net_run $safe $rparam "sysctl -q -w $name=$value" > >> + tst_net_run $safe $rparam "sysctl -q -w" "$name=$value" > > I wonder if it were a bit clearer if we moves all params to second arg: > > tst_net_run $safe "sysctl" "$name=$value -q -w" > OK, but changed to "sysctl" "-q -w $name=$value" because the other > options should also be placed before the key/value pair: > Usage: sysctl -p [-enq] [FILE...] / [-enqaw] [KEY[=VALUE]]... Thanks for addressing it! Kind regards, Petr
diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh index 511fb7eb1..bb30c13ec 100644 --- a/testcases/lib/tst_net.sh +++ b/testcases/lib/tst_net.sh @@ -918,7 +918,7 @@ tst_set_sysctl() local rparam= [ "$TST_USE_NETNS" = "yes" ] && rparam="-r '-e'" - tst_net_run $safe $rparam "sysctl -q -w $name=$value" + tst_net_run $safe $rparam "sysctl -q -w" "$name=$value" } tst_cleanup_rhost()
busybox/sysctl expects -e option to be set before 'name=value'. This can easily be fixed by splitting the string in tst_set_sysctl(), so that 'rparam' with '-e' option is added in between. Signed-off-by: Alexey Kodanev <aleksei.kodanev@bell-sw.com> --- testcases/lib/tst_net.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)