Message ID | 20200608142744.274287-1-yangx.jy@cn.fujitsu.com |
---|---|
State | Accepted |
Headers | show |
Series | network/nfs_lib.sh: Use double quotes for grep pattern | expand |
Hi Xiao, Reviewed-by: Petr Vorel <petr.vorel@gmail.com> > +++ b/testcases/network/nfs/nfs_stress/nfs_lib.sh > @@ -58,7 +58,7 @@ nfs_server_udp_enabled() > tst_rhost_run -c "[ -f /etc/nfs.conf ]" || return 0 > config=$(tst_rhost_run -c 'for f in $(grep ^include.*= '/etc/nfs.conf' | cut -d = -f2); do [ -f $f ] && printf "$f "; done') > - tst_rhost_run -c "grep -q '^[# ]*udp *= *y' /etc/nfs.conf $config" > + tst_rhost_run -c "grep -q \"^[# ]*udp *= *y\" /etc/nfs.conf $config" Good catch. But I wonder if we shouldn't fix tst_rhost_run instead, to avoid this error in the future. How about replacing ' with \" in $cmd? Kind regards, Petr
On 2020/6/8 22:58, Petr Vorel wrote: > Hi Xiao, > > Reviewed-by: Petr Vorel<petr.vorel@gmail.com> > >> +++ b/testcases/network/nfs/nfs_stress/nfs_lib.sh >> @@ -58,7 +58,7 @@ nfs_server_udp_enabled() >> tst_rhost_run -c "[ -f /etc/nfs.conf ]" || return 0 >> config=$(tst_rhost_run -c 'for f in $(grep ^include.*= '/etc/nfs.conf' | cut -d = -f2); do [ -f $f ]&& printf "$f "; done') > >> - tst_rhost_run -c "grep -q '^[# ]*udp *= *y' /etc/nfs.conf $config" >> + tst_rhost_run -c "grep -q \"^[# ]*udp *= *y\" /etc/nfs.conf $config" > Good catch. But I wonder if we shouldn't fix tst_rhost_run instead, to avoid > this error in the future. How about replacing ' with \" in $cmd? Hi Petr, It is fine for me to fix the issue in tst_rhost_run() but I didn't find a better fix, could you provide an example about your idea? Best Regards, Xiao Yang > > Kind regards, > Petr > > > . >
On 08.06.2020 17:58, Petr Vorel wrote: > Hi Xiao, > > Reviewed-by: Petr Vorel <petr.vorel@gmail.com> Applied the patch, thanks! > >> +++ b/testcases/network/nfs/nfs_stress/nfs_lib.sh >> @@ -58,7 +58,7 @@ nfs_server_udp_enabled() >> tst_rhost_run -c "[ -f /etc/nfs.conf ]" || return 0 >> config=$(tst_rhost_run -c 'for f in $(grep ^include.*= '/etc/nfs.conf' | cut -d = -f2); do [ -f $f ] && printf "$f "; done') > >> - tst_rhost_run -c "grep -q '^[# ]*udp *= *y' /etc/nfs.conf $config" >> + tst_rhost_run -c "grep -q \"^[# ]*udp *= *y\" /etc/nfs.conf $config" > Good catch. But I wonder if we shouldn't fix tst_rhost_run instead, to avoid > this error in the future. How about replacing ' with \" in $cmd? > Perhaps this: diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh index 59b373e..9c8f163 100644 --- a/testcases/lib/tst_net.sh +++ b/testcases/lib/tst_net.sh @@ -172,8 +172,7 @@ tst_rhost_run() local output= local ret=0 if [ -n "${TST_USE_SSH:-}" ]; then - output=`ssh -n -q $user@$RHOST "sh -c \ - '$pre_cmd $cmd $post_cmd'" $out 2>&1 || echo 'RTERR'` + output=$(ssh -n -q $user@$RHOST "$pre_cmd $cmd $post_cmd" $out 2>&1 || echo 'RTERR') elif [ -n "$TST_USE_NETNS" ]; then output=`$LTP_NETNS sh -c \ "$pre_cmd $cmd $post_cmd" $out 2>&1 || echo 'RTERR'`
Hi Alexey, Xiao, > Perhaps this: > diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh > index 59b373e..9c8f163 100644 > --- a/testcases/lib/tst_net.sh > +++ b/testcases/lib/tst_net.sh > @@ -172,8 +172,7 @@ tst_rhost_run() > local output= > local ret=0 > if [ -n "${TST_USE_SSH:-}" ]; then > - output=`ssh -n -q $user@$RHOST "sh -c \ > - '$pre_cmd $cmd $post_cmd'" $out 2>&1 || echo 'RTERR'` > + output=$(ssh -n -q $user@$RHOST "$pre_cmd $cmd $post_cmd" $out 2>&1 || echo 'RTERR') > elif [ -n "$TST_USE_NETNS" ]; then > output=`$LTP_NETNS sh -c \ > "$pre_cmd $cmd $post_cmd" $out 2>&1 || echo 'RTERR'` Nice! Much simpler than what what I was just going to post (sed replacement). Would it work for rsh as well? i.e. can it work without sh -c? I have no working rsh setup. And removing it from all 3 variants would be great (keeping them to be the same, also it might allow to also use shell functions, which doesn't work with sh -c "..."). Why was sh -c "..." used anyway? BTW I have more tst_net.sh, but I post them after we solve this one (as replacing quotes with sed, which is in my prepared patchset is ugly). Kind regards, Petr
On 17.06.2020 21:43, Petr Vorel wrote: > Hi Alexey, Xiao, > >> Perhaps this: > >> diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh >> index 59b373e..9c8f163 100644 >> --- a/testcases/lib/tst_net.sh >> +++ b/testcases/lib/tst_net.sh >> @@ -172,8 +172,7 @@ tst_rhost_run() >> local output= >> local ret=0 >> if [ -n "${TST_USE_SSH:-}" ]; then >> - output=`ssh -n -q $user@$RHOST "sh -c \ >> - '$pre_cmd $cmd $post_cmd'" $out 2>&1 || echo 'RTERR'` >> + output=$(ssh -n -q $user@$RHOST "$pre_cmd $cmd $post_cmd" $out 2>&1 || echo 'RTERR') >> elif [ -n "$TST_USE_NETNS" ]; then >> output=`$LTP_NETNS sh -c \ >> "$pre_cmd $cmd $post_cmd" $out 2>&1 || echo 'RTERR'` > > Nice! Much simpler than what what I was just going to post (sed replacement). > Would it work for rsh as well? i.e. can it work without sh -c? > I have no working rsh setup. > > And removing it from all 3 variants would be great (keeping them to be the same, > also it might allow to also use shell functions, which doesn't work with sh -c > "..."). Why was sh -c "..." used anyway? Hi Petr, Don't remember, so if all work without it I would remove it. For rsh, I doubt that's is used now days, let's remove it too? The replacement (ssh) in the tst_rhost_run() has been for a long time already. > BTW I have more tst_net.sh, but I post them after we solve this one (as > replacing quotes with sed, which is in my prepared patchset is ugly). > > Kind regards, > Petr >
Hi Alexey, > > And removing it from all 3 variants would be great (keeping them to be the same, > > also it might allow to also use shell functions, which doesn't work with sh -c > > "..."). Why was sh -c "..." used anyway? > Hi Petr, > Don't remember, so if all work without it I would remove it. For rsh, > I doubt that's is used now days, let's remove it too? The replacement (ssh) > in the tst_rhost_run() has been for a long time already. +1 agree, go ahead :). Kind regards, Petr
diff --git a/testcases/network/nfs/nfs_stress/nfs_lib.sh b/testcases/network/nfs/nfs_stress/nfs_lib.sh index f7eb57d66..e236cd485 100644 --- a/testcases/network/nfs/nfs_stress/nfs_lib.sh +++ b/testcases/network/nfs/nfs_stress/nfs_lib.sh @@ -58,7 +58,7 @@ nfs_server_udp_enabled() tst_rhost_run -c "[ -f /etc/nfs.conf ]" || return 0 config=$(tst_rhost_run -c 'for f in $(grep ^include.*= '/etc/nfs.conf' | cut -d = -f2); do [ -f $f ] && printf "$f "; done') - tst_rhost_run -c "grep -q '^[# ]*udp *= *y' /etc/nfs.conf $config" + tst_rhost_run -c "grep -q \"^[# ]*udp *= *y\" /etc/nfs.conf $config" } nfs_setup_server()
sh -c 'grep -q '^[# ]*udp *= *y' ...' in nfs_server_udp_enabled() cannot processe nested single quotes correctly if rsh/ssh is used and gets the error: -------------------------------------------- grep: Unmatched [ or [^ (or grep: Unmatched [, [^, [:, [., or [=) nfs01 1 TCONF: UDP support disabled on NFS server -------------------------------------------- This issue make many NFS tests including UDP become TCONF even if NFS supports UDP so use double quotes inside to fix it. Fixes: 0f1c36c51 ("nfs: Detect disabled UDP") Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com> --- testcases/network/nfs/nfs_stress/nfs_lib.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)