diff mbox series

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

Message ID 1523572833-3808-1-git-send-email-aginwala@ebay.com
State Superseded
Headers show
Series [ovs-dev] ovn-ctl: Handle whitespaces when using eval for start_ovsdb: | expand

Commit Message

aginwala aginwala April 12, 2018, 10:40 p.m. UTC 
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@ebay.com>
---
 ovn/utilities/ovn-ctl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Ben Pfaff April 25, 2018, 7:54 p.m. UTC | #1 
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@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?

Thanks,

Ben.
aginwala April 25, 2018, 9:06 p.m. UTC | #2 
On Wed, Apr 25, 2018 at 12:54 PM, Ben Pfaff <blp@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@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@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
aginwala April 26, 2018, 12:51 a.m. UTC | #3 
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 <aginwala@asu.edu> wrote:

>
>
> On Wed, Apr 25, 2018 at 12:54 PM, Ben Pfaff <blp@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@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@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
>
Ben Pfaff April 26, 2018, 1:24 a.m. UTC | #4 
On April 25, 2018 8:51:01 PM EDT, aginwala <aginwala@asu.edu> 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 <aginwala@asu.edu> wrote:
>
>>
>>
>> On Wed, Apr 25, 2018 at 12:54 PM, Ben Pfaff <blp@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@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@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>>
>>

What shell is sh on your system?
aginwala April 26, 2018, 1:36 a.m. UTC | #5 
It's dash

root@test3:~# ls -l /bin/sh
lrwxrwxrwx 1 root root 4 Feb 17  2016 /bin/sh -> dash


Regards,

On Wed, Apr 25, 2018 at 6:24 PM, Ben Pfaff <blp@ovn.org> wrote:

> On April 25, 2018 8:51:01 PM EDT, aginwala <aginwala@asu.edu> 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 <aginwala@asu.edu> wrote:
>>
>>>
>>>
>>> On Wed, Apr 25, 2018 at 12:54 PM, Ben Pfaff <blp@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@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@openvswitch.org
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>>
>>>
>>>
>>
> What shell is sh on your system?
>
Ben Pfaff April 28, 2018, 6:57 p.m. UTC | #6 
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 <aginwala@asu.edu> wrote:
> 
> >
> >
> > On Wed, Apr 25, 2018 at 12:54 PM, Ben Pfaff <blp@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@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@openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>
> >
> >
aginwala April 28, 2018, 7 p.m. UTC | #7 
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@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@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 <aginwala@asu.edu> wrote:
> >
> > >
> > >
> > > On Wed, Apr 25, 2018 at 12:54 PM, Ben Pfaff <blp@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@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@openvswitch.org
> > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >>
> > >
> > >
>
Ben Pfaff April 28, 2018, 10:21 p.m. UTC | #8 
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@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@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 <aginwala@asu.edu> wrote:
> > >
> > > >
> > > >
> > > > On Wed, Apr 25, 2018 at 12:54 PM, Ben Pfaff <blp@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@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@openvswitch.org
> > > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > >>
> > > >
> > > >
> >
aginwala April 29, 2018, 1:21 a.m. UTC | #9 
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@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@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@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 <aginwala@asu.edu> wrote:
> > > >
> > > > >
> > > > >
> > > > > On Wed, Apr 25, 2018 at 12:54 PM, Ben Pfaff <blp@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@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@openvswitch.org
> > > > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > > >>
> > > > >
> > > > >
> > >
>
Ben Pfaff April 29, 2018, 1:32 a.m. UTC | #10 
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@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@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@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 <aginwala@asu.edu> wrote:
> > > > >
> > > > > >
> > > > > >
> > > > > > On Wed, Apr 25, 2018 at 12:54 PM, Ben Pfaff <blp@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@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@openvswitch.org
> > > > > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > > > >>
> > > > > >
> > > > > >
> > > >
> >
aginwala April 29, 2018, 1:57 a.m. UTC | #11 
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@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@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@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@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 <aginwala@asu.edu>
> wrote:
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On Wed, Apr 25, 2018 at 12:54 PM, Ben Pfaff <blp@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@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@openvswitch.org
> > > > > > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > > > > >>
> > > > > > >
> > > > > > >
> > > > >
> > >
>
diff mbox series

Patch

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"