[ovs-dev] [PATCH] ovn-ctl: Handle whitespaces when using eval for start_ovsdb:

aginwala aginwala at asu.edu
Sun Apr 29 01:57:05 UTC 2018


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 <blp at ovn.org> 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 <blp at ovn.org> 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 <blp at ovn.org> 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 at 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 at test3:~# ./test.sh
> > > > > > log values are -vconsole:off -vfile:info
> > > > > >
> > > > > > #switching to sh
> > > > > > root at 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 at 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 <aginwala at asu.edu>
> wrote:
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On Wed, Apr 25, 2018 at 12:54 PM, Ben Pfaff <blp at ovn.org>
> 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 <aginwala at ebay.com>
> > > > > > >> > ---
> > > > > > >> >  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
> > > > > > >> dev at openvswitch.org
> > > > > > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > > > > >>
> > > > > > >
> > > > > > >
> > > > >
> > >
>


More information about the dev mailing list