Sure Ben. As you said that would be better and I thought you meant you were fine by dropping local too. Sorry for the confusion. I have revised v4 with local and have sent it out @https://patchwork.ozlabs.org/patch/906237/ .
Also tested the same and works fine. Please let me know if you need any more corrections. Regards, Aliasgar On Sat, Apr 28, 2018 at 6:32 PM, Ben Pfaff <[email protected]> wrote: > Thanks for the revision. > > As I said before, I think that we should separate "local" from "eval" > rather than dropping it, in other words change all instances of > eval local x=... > to > local x; eval x=... > > Thanks, > > Ben. > > On Sat, Apr 28, 2018 at 06:21:31PM -0700, aginwala wrote: > > Thanks Ben: > > > > Sounds good. As suggested, I have included other variables as it > completely > > makes sense to tackle them too. Have sent the revised v3 @ > > https://patchwork.ozlabs.org/patch/906235/. > > > > Have tested the same and it works as expected. > > > > > > > > Regards, > > Aliasgar > > > > On Sat, Apr 28, 2018 at 3:21 PM, Ben Pfaff <[email protected]> wrote: > > > > > I think that just drops the "local" instead of separating it. I think > > > that would be better. I also think that we should do it for each > > > "local" rather than just for a single one, because it's hard to guess > > > whether some other variable might have a space in it. > > > > > > Thanks, > > > > > > Ben. > > > > > > On Sat, Apr 28, 2018 at 12:00:53PM -0700, aginwala wrote: > > > > Yes: > > > > > > > > I already have sent the patch with this changes in v2 @ > > > > https://patchwork.ozlabs.org/patch/904894/ > > > > > > > > Please help review and merge the same. :) > > > > > > > > > > > > Regards, > > > > Aliasgar > > > > > > > > On Sat, Apr 28, 2018 at 11:57 AM, Ben Pfaff <[email protected]> wrote: > > > > > > > > > OK, after experimenting, it looks to me like a good way to avoid > this > > > > > problem is to change all instances of > > > > > > > > > > eval local x=... > > > > > to > > > > > local x; eval x=... > > > > > That seems to work with both bash and dash, and avoids expanding > the > > > > > scope of the local variables. > > > > > > > > > > Does that solve the problem for you, and are you willing to make > the > > > > > change in this form? > > > > > > > > > > Thanks, > > > > > > > > > > Ben. > > > > > > > > > > On Wed, Apr 25, 2018 at 05:51:01PM -0700, aginwala wrote: > > > > > > The root cause is ovn-ctl is using sh instead of bash. Parsing > works > > > fine > > > > > > if we use bash > > > > > > > > > > > > e.g below is the comparision: > > > > > > root@test3:~# cat test.sh > > > > > > #!/bin/bash > > > > > > aaa="-vconsole:off -vfile:info" > > > > > > test(){ > > > > > > c="a" > > > > > > eval local x=\$a${c}a > > > > > > echo 'log values are '$x > > > > > > } > > > > > > root@test3:~# ./test.sh > > > > > > log values are -vconsole:off -vfile:info > > > > > > > > > > > > #switching to sh > > > > > > root@test3:~# cat test.sh > > > > > > #!/bin/sh > > > > > > aaa="-vconsole:off -vfile:info" > > > > > > test(){ > > > > > > c="a" > > > > > > eval local x=\$a${c}a > > > > > > echo 'log values are'$x > > > > > > } > > > > > > root@test3:~# ./test.sh > > > > > > ./test.sh: 1: local: -vfile:info: bad variable name > > > > > > > > > > > > Shortest way is to skip bash for log and use direct var > assignment or > > > > > will > > > > > > try to figure out something that works for both sh and bash using > > > eval. > > > > > Let > > > > > > me know your thoughts on that! > > > > > > > > > > > > On Wed, Apr 25, 2018 at 2:06 PM, aginwala <[email protected]> > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Apr 25, 2018 at 12:54 PM, Ben Pfaff <[email protected]> > wrote: > > > > > > > > > > > > > >> On Thu, Apr 12, 2018 at 03:40:33PM -0700, aginwala wrote: > > > > > > >> > eval doesn't understand white spaces which was introduced in > > > commit > > > > > > >> > 79c7961b8b3c4b7ea0251dea2ffacfa84c84fecb for starting > clustered > > > > > ovn dbs > > > > > > >> > > > > > > > >> > Hence, we need to explicitely handle it. > > > > > > >> > e.g. /usr/share/openvswitch/scripts/ovn-ctl > > > > > > >> --db-nb-addr=192.168.220.101 --db-nb-create-insecure-remote=yes > \ > > > > > > >> > --db-sb-addr=192.168.220.101 --db-sb-create-insecure- > > > remote=yes > > > > > \ > > > > > > >> > --db-nb-cluster-local-addr=192.168.220.101 \ > > > > > > >> > --db-sb-cluster-local-addr=192.168.220.101 \ > > > > > > >> > --ovn-northd-nb-db=tcp:192.168.220.101:6641 > ,tcp:192.168.220 > > > > > > >> .102:6641,tcp:192.168.220.103:6641 \ > > > > > > >> > --ovn-northd-sb-db=tcp:192.168.220.101:6642 > ,tcp:192.168.220 > > > > > > >> .102:6642,tcp:192.168.220.103:6642 \ > > > > > > >> > start_northd > > > > > > >> > > > > > > > >> > gives error: /usr/share/openvswitch/scripts/ovn-ctl: 1: > local: > > > > > > >> -vfile:info: bad variable name > > > > > > >> > As a result ovsdb failes to even initialize and start. This > > > commit > > > > > > >> fixes the same. > > > > > > >> > > > > > > > >> > Signed-off-by: aginwala <[email protected]> > > > > > > >> > --- > > > > > > >> > ovn/utilities/ovn-ctl | 4 ++-- > > > > > > >> > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > >> > > > > > > > >> > diff --git a/ovn/utilities/ovn-ctl b/ovn/utilities/ovn-ctl > > > > > > >> > index 25dda52..9a1ad75 100755 > > > > > > >> > --- a/ovn/utilities/ovn-ctl > > > > > > >> > +++ b/ovn/utilities/ovn-ctl > > > > > > >> > @@ -409,8 +409,8 @@ set_defaults () { > > > > > > >> > OVN_CONTROLLER_LOG="-vconsole:emer -vsyslog:err > > > -vfile:info" > > > > > > >> > OVN_NORTHD_LOG="-vconsole:emer -vsyslog:err > -vfile:info" > > > > > > >> > OVN_NORTHD_LOGFILE="" > > > > > > >> > - OVN_NB_LOG="-vconsole:off -vfile:info" > > > > > > >> > - OVN_SB_LOG="-vconsole:off -vfile:info" > > > > > > >> > + OVN_NB_LOG='"-vconsole:off' '-vfile:info"' > > > > > > >> > + OVN_SB_LOG='"-vconsole:off' '-vfile:info"' > > > > > > >> > OVN_NB_LOGFILE="$logdir/ovsdb-server-nb.log" > > > > > > >> > OVN_SB_LOGFILE="$logdir/ovsdb-server-sb.log" > > > > > > >> > > > > > > >> This doesn't make sense to me. The line > > > > > > >> > > > > > > >> eval local log=\$OVN_${DB}_LOG > > > > > > >> > > > > > > >> should parameter-expand to: > > > > > > >> > > > > > > >> eval local log=$OVN_SB_LOG > > > > > > >> > > > > > > >> which should be executed as: > > > > > > >> > > > > > > >> local log=$OVN_SB_LOG > > > > > > >> > > > > > > >> which should assign "-vconsole:off -vfile:info", without the > > > double > > > > > > >> quotes, to $log (since variable assignment in shell doesn't do > > > word > > > > > > >> splitting). > > > > > > >> > > > > > > >> Then, later, > > > > > > >> > > > > > > >> set "$@" $log --log-file=$logfile > > > > > > >> > > > > > > >> should do word splitting on $log. > > > > > > >> > > > > > > >> Do you understand what is going on here? > > > > > > >> > > > > > > > > > > > > > > >>> > > > > > > > Hi : > > > > > > > > > > > > > > Yes for sure. But eval do not understand white spaces in the > string > > > > > "-vconsole:off > > > > > > > -vfile:info" which breaks on line local log=$OVN_SB_LOG. set > "$@" > > > > > $log > > > > > > > --log-file=$logfile > > > > > > > is not even there yet. Hope you got the idea what I mean. > Easily > > > > > > > reproduced on ubuntu box as I am using ubuntu. > > > > > > > > > > > > > > > > > > > > > > > > > > > >> Thanks, > > > > > > >> > > > > > > >> Ben. > > > > > > >> _______________________________________________ > > > > > > >> dev mailing list > > > > > > >> [email protected] > > > > > > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
