Re: [Openvpn-devel] [PATCH] bash: substitute legacy `` with modern $()

[Antonio Quartulli]

On 25/08/17 04:21, David Sommerseth wrote:
> On 24/08/17 21:18, Gert Doering wrote:
>> (gen-release-tarballs.sh only needs to work on FreeBSD and Linux, and
>> FreeBSD's /bin/sh is sufficiently modern so so it's likely to work
>>  - but the test scripts need to run robustly everywhere a user builds,
>> so never assume "because bash says so!" is a way anywhere but into worlds
>> of pain.  And yes, we've been there before :-) )
> 
> And to avoid style confusions ... as long as we can avoid having "one
> style" on the test scripts and "another style" on the dev-tools scripts,
> that will be easier to review and maintain.
> 
> We can have more slack in dev-tools, but if we deviate, then we need to
> properly document it so we won't forget why.

I also think it is a bad idea to have different styles. If we have to
agree on something, it should be valid for every file of that type.

Let's postpone this for when we'll have tested various platforms.


Cheers,

-- 
Antonio Quartulli



signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] bash: substitute legacy `` with modern $()

[David Sommerseth]

On 24/08/17 21:18, Gert Doering wrote:
> (gen-release-tarballs.sh only needs to work on FreeBSD and Linux, and
> FreeBSD's /bin/sh is sufficiently modern so so it's likely to work
>  - but the test scripts need to run robustly everywhere a user builds,
> so never assume "because bash says so!" is a way anywhere but into worlds
> of pain.  And yes, we've been there before :-) )

And to avoid style confusions ... as long as we can avoid having "one
style" on the test scripts and "another style" on the dev-tools scripts,
that will be easier to review and maintain.

We can have more slack in dev-tools, but if we deviate, then we need to
properly document it so we won't forget why.


-- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc




signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH v2] bash: substitute legacy `` with modern $()

[Gert Doering]

Hi,

On Thu, Aug 24, 2017 at 09:38:45PM +0800, Antonio Quartulli wrote:
> The backquotes for command substitution in bash are
> considered old-style in favour of the more modern $() [1].
> Substitute them.
> 
> [1]https://www.gnu.org/software/bash/manual/html_node/Command-Substitution.html#Command-Substitution
> 
> Signed-off-by: Antonio Quartulli 
> ---
> 
> v2: added overlooked occurrence

Still NAK.

> note: I did not really know how to test this patch.

More reason for NAK.

gert

-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


signature.asc
Description: PGP signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] bash: substitute legacy `` with modern $()

[Gert Doering]

Hi,

On Thu, Aug 24, 2017 at 09:33:05PM +0800, Antonio Quartulli wrote:
> The backquotes for command substitution in bash are
> considered old-style in favour of the more modern $() [1].
> Substitute them.
> 
> [1]https://www.gnu.org/software/bash/manual/html_node/Command-Substitution.html#Command-Substitution

We do, pointedly, *not* care about *bash* compatibility, but about
"it works on all the oddball shells you happen to find on any of the
supported platforms".

So, NAK, unless you have tested with ksh, and the system /bin/sh on at 
least Solaris and AIX.

Please do not change working shell script stuff just because *bash* says
"this is the more modern way!".

(gen-release-tarballs.sh only needs to work on FreeBSD and Linux, and
FreeBSD's /bin/sh is sufficiently modern so so it's likely to work
 - but the test scripts need to run robustly everywhere a user builds,
so never assume "because bash says so!" is a way anywhere but into worlds
of pain.  And yes, we've been there before :-) )

gert
-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


signature.asc
Description: PGP signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] avoid useless assignment

[Christian Hesse]

David Sommerseth  on Thu, 2017/08/24 20:16:
> On 24/08/17 09:57, Antonio Quartulli wrote:
> > My effort in writing the commit message has been quite poor.
> > 
> > The assignment is useless because 'ret' is re-assigned a few lines later
> > without ever being read.  
> 
> Hmmm.  I'm not convinced of this change.  But I'm also weird in these
> cases :)
> 
> I think it is good defensive programming to predefine the state of
> variables.  When that is not done, it is up the the compiler to decide
> what to do - which most of the times does a sane job these days.  But
> you're at the mercy of the compiler.
> 
> In this case,  I would expect the compiler to optimize this out anyway,
> regardless of the approaches used.  The compiler doesn't necessarily set
> the value first to true and then to change it to the output of
> multi_process_post().  It might just as well postpone the declaration.
> 
> So I think a better approach would be to completely move the "bool ret"
> down.  So it will become:
> 
>bool ret = multi_process_post(m, mi, mpp_flags);
> 
> Which I think is also closer to what the compiler would end up with anyway.

ISO C90 forbids mixed declarations and code in C. Probably compilers will
start to complain.
-- 
main(a){char*c=/*Schoene Gruesse */"B?IJj;MEH"
"CX:;",b;for(a/*Best regards my address:*/=0;b=c[a++];)
putchar(b-1/(/*Chriscc -ox -xc - && ./x*/b/42*2-3)*42);}


pgp9YMP02N81Q.pgp
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] avoid useless assignment

[Christian Hesse]

David Sommerseth  on Thu, 2017/08/24 20:51:
> On 24/08/17 20:40, Antonio Quartulli wrote:
> > 
> > 
> > On 25/08/17 02:40, Christian Hesse wrote:  
> >> David Sommerseth  on Thu, 2017/08/24
> >> 20:16:  
> >>> On 24/08/17 09:57, Antonio Quartulli wrote:  
>  My effort in writing the commit message has been quite poor.
> 
>  The assignment is useless because 'ret' is re-assigned a few lines
>  later without ever being read.
> >>>
> >>> Hmmm.  I'm not convinced of this change.  But I'm also weird in these
> >>> cases :)
> >>>
> >>> I think it is good defensive programming to predefine the state of
> >>> variables.  When that is not done, it is up the the compiler to decide
> >>> what to do - which most of the times does a sane job these days.  But
> >>> you're at the mercy of the compiler.
> >>>
> >>> In this case,  I would expect the compiler to optimize this out anyway,
> >>> regardless of the approaches used.  The compiler doesn't necessarily set
> >>> the value first to true and then to change it to the output of
> >>> multi_process_post().  It might just as well postpone the declaration.
> >>>
> >>> So I think a better approach would be to completely move the "bool ret"
> >>> down.  So it will become:
> >>>
> >>>bool ret = multi_process_post(m, mi, mpp_flags);
> >>>
> >>> Which I think is also closer to what the compiler would end up with
> >>> anyway.  
> >>
> >> ISO C90 forbids mixed declarations and code in C. Probably compilers will
> >> start to complain.  
> > 
> > We try to stick to C99. I think it allows such mix, no?  
> 
> That is correct.  We set -std=c99 unless CFLAGS already contains -std=.
> But we expect OpenVPN to be C99 compliant.
> 
> And C99 allows this.

You are right... So scratch my concern.
-- 
main(a){char*c=/*Schoene Gruesse */"B?IJj;MEH"
"CX:;",b;for(a/*Best regards my address:*/=0;b=c[a++];)
putchar(b-1/(/*Chriscc -ox -xc - && ./x*/b/42*2-3)*42);}


pgpEGrDLbx46m.pgp
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] bash: substitute legacy `` with modern $()

[Antonio Quartulli]

On 25/08/17 02:06, David Sommerseth wrote:
> On 24/08/17 16:42, Antonio Quartulli wrote:
>> dev-tools/gen-release-tarballs.sh is only for devs, while
>> tests/t_cltsrv.sh is for running some tests, but I am not sure sure how
>> the latter would interact with non-linux systems.
>>
>> Maybe Gert knows(?)
> 
> Tried running them through ksh or dash?  Those are the most feature
> restrictive shells I can think of right now.  Dash is supposed to be the
> most POSIX compliant shell, iirc.

A quick test with dash on ubuntu shown that the line:

echo "   remote-name  -- valid remotes: $(git remote | tr \\\n ' ')"

is interpreted correctly.

Cheers,


-- 
Antonio Quartulli



signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] avoid useless assignment

[David Sommerseth]

On 24/08/17 20:40, Antonio Quartulli wrote:
> 
> 
> On 25/08/17 02:40, Christian Hesse wrote:
>> David Sommerseth  on Thu, 2017/08/24 
>> 20:16:
>>> On 24/08/17 09:57, Antonio Quartulli wrote:
 My effort in writing the commit message has been quite poor.

 The assignment is useless because 'ret' is re-assigned a few lines later
 without ever being read.  
>>>
>>> Hmmm.  I'm not convinced of this change.  But I'm also weird in these
>>> cases :)
>>>
>>> I think it is good defensive programming to predefine the state of
>>> variables.  When that is not done, it is up the the compiler to decide
>>> what to do - which most of the times does a sane job these days.  But
>>> you're at the mercy of the compiler.
>>>
>>> In this case,  I would expect the compiler to optimize this out anyway,
>>> regardless of the approaches used.  The compiler doesn't necessarily set
>>> the value first to true and then to change it to the output of
>>> multi_process_post().  It might just as well postpone the declaration.
>>>
>>> So I think a better approach would be to completely move the "bool ret"
>>> down.  So it will become:
>>>
>>>bool ret = multi_process_post(m, mi, mpp_flags);
>>>
>>> Which I think is also closer to what the compiler would end up with anyway.
>>
>> ISO C90 forbids mixed declarations and code in C. Probably compilers will
>> start to complain.
> 
> We try to stick to C99. I think it allows such mix, no?

That is correct.  We set -std=c99 unless CFLAGS already contains -std=.
But we expect OpenVPN to be C99 compliant.

And C99 allows this.


-- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc




signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] avoid useless assignment

[Antonio Quartulli]

On 25/08/17 02:40, Christian Hesse wrote:
> David Sommerseth  on Thu, 2017/08/24 20:16:
>> On 24/08/17 09:57, Antonio Quartulli wrote:
>>> My effort in writing the commit message has been quite poor.
>>>
>>> The assignment is useless because 'ret' is re-assigned a few lines later
>>> without ever being read.  
>>
>> Hmmm.  I'm not convinced of this change.  But I'm also weird in these
>> cases :)
>>
>> I think it is good defensive programming to predefine the state of
>> variables.  When that is not done, it is up the the compiler to decide
>> what to do - which most of the times does a sane job these days.  But
>> you're at the mercy of the compiler.
>>
>> In this case,  I would expect the compiler to optimize this out anyway,
>> regardless of the approaches used.  The compiler doesn't necessarily set
>> the value first to true and then to change it to the output of
>> multi_process_post().  It might just as well postpone the declaration.
>>
>> So I think a better approach would be to completely move the "bool ret"
>> down.  So it will become:
>>
>>bool ret = multi_process_post(m, mi, mpp_flags);
>>
>> Which I think is also closer to what the compiler would end up with anyway.
> 
> ISO C90 forbids mixed declarations and code in C. Probably compilers will
> start to complain.

We try to stick to C99. I think it allows such mix, no?

Cheers,


-- 
Antonio Quartulli



signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] avoid useless assignment

[David Sommerseth]

On 24/08/17 09:57, Antonio Quartulli wrote:
> My effort in writing the commit message has been quite poor.
> 
> The assignment is useless because 'ret' is re-assigned a few lines later
> without ever being read.

Hmmm.  I'm not convinced of this change.  But I'm also weird in these
cases :)

I think it is good defensive programming to predefine the state of
variables.  When that is not done, it is up the the compiler to decide
what to do - which most of the times does a sane job these days.  But
you're at the mercy of the compiler.

In this case,  I would expect the compiler to optimize this out anyway,
regardless of the approaches used.  The compiler doesn't necessarily set
the value first to true and then to change it to the output of
multi_process_post().  It might just as well postpone the declaration.

So I think a better approach would be to completely move the "bool ret"
down.  So it will become:

   bool ret = multi_process_post(m, mi, mpp_flags);

Which I think is also closer to what the compiler would end up with anyway.


-- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc



> On 24/08/17 15:53, Antonio Quartulli wrote:
>> Signed-off-by: Antonio Quartulli 
>> ---
>>  src/openvpn/multi.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
>> index 5892ac07..6cdb0110 100644
>> --- a/src/openvpn/multi.h
>> +++ b/src/openvpn/multi.h
>> @@ -633,7 +633,7 @@ multi_process_outgoing_tun(struct multi_context *m, 
>> const unsigned int mpp_flags
>>  static inline bool
>>  multi_process_outgoing_link_dowork(struct multi_context *m, struct 
>> multi_instance *mi, const unsigned int mpp_flags)
>>  {
>> -bool ret = true;
>> +bool ret;
>>  set_prefix(mi);
>>  process_outgoing_link(>context);
>>  ret = multi_process_post(m, mi, mpp_flags);
>>



signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] bash: substitute legacy `` with modern $()

[David Sommerseth]

On 24/08/17 16:42, Antonio Quartulli wrote:
> dev-tools/gen-release-tarballs.sh is only for devs, while
> tests/t_cltsrv.sh is for running some tests, but I am not sure sure how
> the latter would interact with non-linux systems.
> 
> Maybe Gert knows(?)

Tried running them through ksh or dash?  Those are the most feature
restrictive shells I can think of right now.  Dash is supposed to be the
most POSIX compliant shell, iirc.


-- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc



> On 24/08/17 22:37, Илья Шипицин wrote:
>> openvpn is also built on many non bash systems. what about them?
>>
>> 24 авг. 2017 г. 18:34 пользователь "Antonio Quartulli" 
>> написал:
>>
>> The backquotes for command substitution in bash are
>> considered old-style in favour of the more modern $() [1].
>> Substitute them.
>>
>> [1]https://www.gnu.org/software/bash/manual/html_node/Command-Substitution.
>> html#Command-Substitution
>>
>> Signed-off-by: Antonio Quartulli 
>> ---
>>
>> note: I did not really know how to test this patch.
>>
>>  dev-tools/gen-release-tarballs.sh | 10 +-
>>  tests/t_cltsrv.sh |  6 +++---
>>  2 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/dev-tools/gen-release-tarballs.sh b/dev-tools/gen-release-
>> tarballs.sh
>> index f9c620e3..550e5cd2 100755
>> --- a/dev-tools/gen-release-tarballs.sh
>> +++ b/dev-tools/gen-release-tarballs.sh
>> @@ -49,7 +49,7 @@ if [ $? -ne 0 ]; then
>>  fi
>>
>>  # Extract the git URL
>> -giturl="`git remote get-url $arg_remote_name 2>/dev/null`"
>> +giturl="$(git remote get-url $arg_remote_name 2>/dev/null)"
>>  if [ $? -ne 0 ]; then
>>  echo "** ERROR ** Invalid git remote name: $arg_remote_name"
>>  exit 2
>> @@ -71,7 +71,7 @@ get_filename()
>>  {
>>  local wildcard="$1"
>>
>> -res="`find . -maxdepth 1 -type f -name \"$wildcard\" | head -n1 | cut
>> -d/ -f2-`"
>> +res="$(find . -maxdepth 1 -type f -name \"$wildcard\" | head -n1 | cut
>> -d/ -f2-)"
>>  if [ $? -ne 0 ]; then
>>  echo "-- 'find' failed."
>>  exit 5
>> @@ -88,7 +88,7 @@ copy_files()
>>  local fileext="$1"
>>  local dest="$2"
>>
>> -file="`get_filename openvpn-*.*.*.$fileext`"
>> +file="$(get_filename openvpn-*.*.*.$fileext)"
>>  if [ -z "$file" ]; then
>>  echo "** ERROR Failed to find source file"
>>  exit 5
>> @@ -106,7 +106,7 @@ sign_file()
>>  local signkey="$1"
>>  local srchfile="$2"
>>  local signtype="$3"
>> -local file="`get_filename $srchfile`"
>> +local file="$(get_filename $srchfile)"
>>
>>  echo "-- Signing $file ..."
>>  case "$signtype" in
>> @@ -169,7 +169,7 @@ fi
>>  #
>>
>>  # Clone the remote repository
>> -workdir="`mktemp -d -p /var/tmp openvpn-build-release-XX`"
>> +workdir="$(mktemp -d -p /var/tmp openvpn-build-release-XX)"
>>  cd $workdir
>>  echo "-- Working directory: $workdir"
>>  echo "-- git clone $giturl"
>> diff --git a/tests/t_cltsrv.sh b/tests/t_cltsrv.sh
>> index 752251e4..1ab3db3e 100755
>> --- a/tests/t_cltsrv.sh
>> +++ b/tests/t_cltsrv.sh
>> @@ -25,14 +25,14 @@ top_builddir="${top_builddir:-..}"
>>  trap "rm -f log.$$ log.$$.signal ; trap 0 ; exit 77" 1 2 15
>>  trap "rm -f log.$$ log.$$.signal ; exit 1" 0 3
>>  addopts=
>> -case `uname -s` in
>> +case $(uname -s) in
>>  FreeBSD)
>>  # FreeBSD jails map the outgoing IP to the jail IP - we need to
>>  # allow the real IP unless we want the test to run forever.
>> -if test "`sysctl 2>/dev/null -n security.jail.jailed`" = 1 \
>> +if test "$(sysctl 2>/dev/null -n security.jail.jailed)" = 1 \
>>  || ps -ostate= -p $$ | grep -q J; then
>> addopts="--float"
>> -   if test "x`ifconfig | grep inet`" = x ; then
>> +   if test "x$(ifconfig | grep inet)" = x ; then
>> echo "###"
>> echo "### To run the test in a FreeBSD jail, you MUST add an IP
>> alias for the jail's IP."
>> echo "###"
>> --
>> 2.14.1



signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] bash: substitute legacy `` with modern $()

[Antonio Quartulli]

dev-tools/gen-release-tarballs.sh is only for devs, while
tests/t_cltsrv.sh is for running some tests, but I am not sure sure how
the latter would interact with non-linux systems.

Maybe Gert knows(?)

Cheers,

On 24/08/17 22:37, Илья Шипицин wrote:
> openvpn is also built on many non bash systems. what about them?
> 
> 24 авг. 2017 г. 18:34 пользователь "Antonio Quartulli" 
> написал:
> 
> The backquotes for command substitution in bash are
> considered old-style in favour of the more modern $() [1].
> Substitute them.
> 
> [1]https://www.gnu.org/software/bash/manual/html_node/Command-Substitution.
> html#Command-Substitution
> 
> Signed-off-by: Antonio Quartulli 
> ---
> 
> note: I did not really know how to test this patch.
> 
>  dev-tools/gen-release-tarballs.sh | 10 +-
>  tests/t_cltsrv.sh |  6 +++---
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/dev-tools/gen-release-tarballs.sh b/dev-tools/gen-release-
> tarballs.sh
> index f9c620e3..550e5cd2 100755
> --- a/dev-tools/gen-release-tarballs.sh
> +++ b/dev-tools/gen-release-tarballs.sh
> @@ -49,7 +49,7 @@ if [ $? -ne 0 ]; then
>  fi
> 
>  # Extract the git URL
> -giturl="`git remote get-url $arg_remote_name 2>/dev/null`"
> +giturl="$(git remote get-url $arg_remote_name 2>/dev/null)"
>  if [ $? -ne 0 ]; then
>  echo "** ERROR ** Invalid git remote name: $arg_remote_name"
>  exit 2
> @@ -71,7 +71,7 @@ get_filename()
>  {
>  local wildcard="$1"
> 
> -res="`find . -maxdepth 1 -type f -name \"$wildcard\" | head -n1 | cut
> -d/ -f2-`"
> +res="$(find . -maxdepth 1 -type f -name \"$wildcard\" | head -n1 | cut
> -d/ -f2-)"
>  if [ $? -ne 0 ]; then
>  echo "-- 'find' failed."
>  exit 5
> @@ -88,7 +88,7 @@ copy_files()
>  local fileext="$1"
>  local dest="$2"
> 
> -file="`get_filename openvpn-*.*.*.$fileext`"
> +file="$(get_filename openvpn-*.*.*.$fileext)"
>  if [ -z "$file" ]; then
>  echo "** ERROR Failed to find source file"
>  exit 5
> @@ -106,7 +106,7 @@ sign_file()
>  local signkey="$1"
>  local srchfile="$2"
>  local signtype="$3"
> -local file="`get_filename $srchfile`"
> +local file="$(get_filename $srchfile)"
> 
>  echo "-- Signing $file ..."
>  case "$signtype" in
> @@ -169,7 +169,7 @@ fi
>  #
> 
>  # Clone the remote repository
> -workdir="`mktemp -d -p /var/tmp openvpn-build-release-XX`"
> +workdir="$(mktemp -d -p /var/tmp openvpn-build-release-XX)"
>  cd $workdir
>  echo "-- Working directory: $workdir"
>  echo "-- git clone $giturl"
> diff --git a/tests/t_cltsrv.sh b/tests/t_cltsrv.sh
> index 752251e4..1ab3db3e 100755
> --- a/tests/t_cltsrv.sh
> +++ b/tests/t_cltsrv.sh
> @@ -25,14 +25,14 @@ top_builddir="${top_builddir:-..}"
>  trap "rm -f log.$$ log.$$.signal ; trap 0 ; exit 77" 1 2 15
>  trap "rm -f log.$$ log.$$.signal ; exit 1" 0 3
>  addopts=
> -case `uname -s` in
> +case $(uname -s) in
>  FreeBSD)
>  # FreeBSD jails map the outgoing IP to the jail IP - we need to
>  # allow the real IP unless we want the test to run forever.
> -if test "`sysctl 2>/dev/null -n security.jail.jailed`" = 1 \
> +if test "$(sysctl 2>/dev/null -n security.jail.jailed)" = 1 \
>  || ps -ostate= -p $$ | grep -q J; then
> addopts="--float"
> -   if test "x`ifconfig | grep inet`" = x ; then
> +   if test "x$(ifconfig | grep inet)" = x ; then
> echo "###"
> echo "### To run the test in a FreeBSD jail, you MUST add an IP
> alias for the jail's IP."
> echo "###"
> --
> 2.14.1
> 
> 
> 
> --
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
> 

-- 
Antonio Quartulli



signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] bash: substitute legacy `` with modern $()

[Илья Шипицин]

openvpn is also built on many non bash systems. what about them?

24 авг. 2017 г. 18:34 пользователь "Antonio Quartulli" 
написал:

The backquotes for command substitution in bash are
considered old-style in favour of the more modern $() [1].
Substitute them.

[1]https://www.gnu.org/software/bash/manual/html_node/Command-Substitution.
html#Command-Substitution

Signed-off-by: Antonio Quartulli 
---

note: I did not really know how to test this patch.

 dev-tools/gen-release-tarballs.sh | 10 +-
 tests/t_cltsrv.sh |  6 +++---
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/dev-tools/gen-release-tarballs.sh b/dev-tools/gen-release-
tarballs.sh
index f9c620e3..550e5cd2 100755
--- a/dev-tools/gen-release-tarballs.sh
+++ b/dev-tools/gen-release-tarballs.sh
@@ -49,7 +49,7 @@ if [ $? -ne 0 ]; then
 fi

 # Extract the git URL
-giturl="`git remote get-url $arg_remote_name 2>/dev/null`"
+giturl="$(git remote get-url $arg_remote_name 2>/dev/null)"
 if [ $? -ne 0 ]; then
 echo "** ERROR ** Invalid git remote name: $arg_remote_name"
 exit 2
@@ -71,7 +71,7 @@ get_filename()
 {
 local wildcard="$1"

-res="`find . -maxdepth 1 -type f -name \"$wildcard\" | head -n1 | cut
-d/ -f2-`"
+res="$(find . -maxdepth 1 -type f -name \"$wildcard\" | head -n1 | cut
-d/ -f2-)"
 if [ $? -ne 0 ]; then
 echo "-- 'find' failed."
 exit 5
@@ -88,7 +88,7 @@ copy_files()
 local fileext="$1"
 local dest="$2"

-file="`get_filename openvpn-*.*.*.$fileext`"
+file="$(get_filename openvpn-*.*.*.$fileext)"
 if [ -z "$file" ]; then
 echo "** ERROR Failed to find source file"
 exit 5
@@ -106,7 +106,7 @@ sign_file()
 local signkey="$1"
 local srchfile="$2"
 local signtype="$3"
-local file="`get_filename $srchfile`"
+local file="$(get_filename $srchfile)"

 echo "-- Signing $file ..."
 case "$signtype" in
@@ -169,7 +169,7 @@ fi
 #

 # Clone the remote repository
-workdir="`mktemp -d -p /var/tmp openvpn-build-release-XX`"
+workdir="$(mktemp -d -p /var/tmp openvpn-build-release-XX)"
 cd $workdir
 echo "-- Working directory: $workdir"
 echo "-- git clone $giturl"
diff --git a/tests/t_cltsrv.sh b/tests/t_cltsrv.sh
index 752251e4..1ab3db3e 100755
--- a/tests/t_cltsrv.sh
+++ b/tests/t_cltsrv.sh
@@ -25,14 +25,14 @@ top_builddir="${top_builddir:-..}"
 trap "rm -f log.$$ log.$$.signal ; trap 0 ; exit 77" 1 2 15
 trap "rm -f log.$$ log.$$.signal ; exit 1" 0 3
 addopts=
-case `uname -s` in
+case $(uname -s) in
 FreeBSD)
 # FreeBSD jails map the outgoing IP to the jail IP - we need to
 # allow the real IP unless we want the test to run forever.
-if test "`sysctl 2>/dev/null -n security.jail.jailed`" = 1 \
+if test "$(sysctl 2>/dev/null -n security.jail.jailed)" = 1 \
 || ps -ostate= -p $$ | grep -q J; then
addopts="--float"
-   if test "x`ifconfig | grep inet`" = x ; then
+   if test "x$(ifconfig | grep inet)" = x ; then
echo "###"
echo "### To run the test in a FreeBSD jail, you MUST add an IP
alias for the jail's IP."
echo "###"
--
2.14.1



--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH v2] bash: substitute legacy `` with modern $()

[Antonio Quartulli]

The backquotes for command substitution in bash are
considered old-style in favour of the more modern $() [1].
Substitute them.

[1]https://www.gnu.org/software/bash/manual/html_node/Command-Substitution.html#Command-Substitution

Signed-off-by: Antonio Quartulli 
---

v2: added overlooked occurrence

note: I did not really know how to test this patch.

 dev-tools/gen-release-tarballs.sh | 12 ++--
 tests/t_cltsrv.sh |  6 +++---
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/dev-tools/gen-release-tarballs.sh 
b/dev-tools/gen-release-tarballs.sh
index f9c620e3..b4f5edc0 100755
--- a/dev-tools/gen-release-tarballs.sh
+++ b/dev-tools/gen-release-tarballs.sh
@@ -22,7 +22,7 @@ set -u
 if [ $# -ne 4 ]; then
 echo "Usage: $0"
 echo ""
-echo "   remote-name  -- valid remotes: `git remote | tr \\\n ' '`"
+echo "   remote-name  -- valid remotes: $(git remote | tr \\\n ' ')"
 echo "   tag-name -- An existing release tag"
 echo "   sign-key -- PGP key used to sign all files"
 echo "   dest-dir -- Where to put the complete set of release tarballs"
@@ -49,7 +49,7 @@ if [ $? -ne 0 ]; then
 fi
 
 # Extract the git URL
-giturl="`git remote get-url $arg_remote_name 2>/dev/null`"
+giturl="$(git remote get-url $arg_remote_name 2>/dev/null)"
 if [ $? -ne 0 ]; then
 echo "** ERROR ** Invalid git remote name: $arg_remote_name"
 exit 2
@@ -71,7 +71,7 @@ get_filename()
 {
 local wildcard="$1"
 
-res="`find . -maxdepth 1 -type f -name \"$wildcard\" | head -n1 | cut -d/ 
-f2-`"
+res="$(find . -maxdepth 1 -type f -name \"$wildcard\" | head -n1 | cut -d/ 
-f2-)"
 if [ $? -ne 0 ]; then
 echo "-- 'find' failed."
 exit 5
@@ -88,7 +88,7 @@ copy_files()
 local fileext="$1"
 local dest="$2"
 
-file="`get_filename openvpn-*.*.*.$fileext`"
+file="$(get_filename openvpn-*.*.*.$fileext)"
 if [ -z "$file" ]; then
 echo "** ERROR Failed to find source file"
 exit 5
@@ -106,7 +106,7 @@ sign_file()
 local signkey="$1"
 local srchfile="$2"
 local signtype="$3"
-local file="`get_filename $srchfile`"
+local file="$(get_filename $srchfile)"
 
 echo "-- Signing $file ..."
 case "$signtype" in
@@ -169,7 +169,7 @@ fi
 #
 
 # Clone the remote repository
-workdir="`mktemp -d -p /var/tmp openvpn-build-release-XX`"
+workdir="$(mktemp -d -p /var/tmp openvpn-build-release-XX)"
 cd $workdir
 echo "-- Working directory: $workdir"
 echo "-- git clone $giturl"
diff --git a/tests/t_cltsrv.sh b/tests/t_cltsrv.sh
index 752251e4..1ab3db3e 100755
--- a/tests/t_cltsrv.sh
+++ b/tests/t_cltsrv.sh
@@ -25,14 +25,14 @@ top_builddir="${top_builddir:-..}"
 trap "rm -f log.$$ log.$$.signal ; trap 0 ; exit 77" 1 2 15
 trap "rm -f log.$$ log.$$.signal ; exit 1" 0 3
 addopts=
-case `uname -s` in
+case $(uname -s) in
 FreeBSD)
 # FreeBSD jails map the outgoing IP to the jail IP - we need to
 # allow the real IP unless we want the test to run forever.
-if test "`sysctl 2>/dev/null -n security.jail.jailed`" = 1 \
+if test "$(sysctl 2>/dev/null -n security.jail.jailed)" = 1 \
 || ps -ostate= -p $$ | grep -q J; then
addopts="--float"
-   if test "x`ifconfig | grep inet`" = x ; then
+   if test "x$(ifconfig | grep inet)" = x ; then
echo "###"
echo "### To run the test in a FreeBSD jail, you MUST add an IP 
alias for the jail's IP."
echo "###"
-- 
2.14.1


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH] bash: substitute legacy `` with modern $()

[Antonio Quartulli]

The backquotes for command substitution in bash are
considered old-style in favour of the more modern $() [1].
Substitute them.

[1]https://www.gnu.org/software/bash/manual/html_node/Command-Substitution.html#Command-Substitution

Signed-off-by: Antonio Quartulli 
---

note: I did not really know how to test this patch.

 dev-tools/gen-release-tarballs.sh | 10 +-
 tests/t_cltsrv.sh |  6 +++---
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/dev-tools/gen-release-tarballs.sh 
b/dev-tools/gen-release-tarballs.sh
index f9c620e3..550e5cd2 100755
--- a/dev-tools/gen-release-tarballs.sh
+++ b/dev-tools/gen-release-tarballs.sh
@@ -49,7 +49,7 @@ if [ $? -ne 0 ]; then
 fi
 
 # Extract the git URL
-giturl="`git remote get-url $arg_remote_name 2>/dev/null`"
+giturl="$(git remote get-url $arg_remote_name 2>/dev/null)"
 if [ $? -ne 0 ]; then
 echo "** ERROR ** Invalid git remote name: $arg_remote_name"
 exit 2
@@ -71,7 +71,7 @@ get_filename()
 {
 local wildcard="$1"
 
-res="`find . -maxdepth 1 -type f -name \"$wildcard\" | head -n1 | cut -d/ 
-f2-`"
+res="$(find . -maxdepth 1 -type f -name \"$wildcard\" | head -n1 | cut -d/ 
-f2-)"
 if [ $? -ne 0 ]; then
 echo "-- 'find' failed."
 exit 5
@@ -88,7 +88,7 @@ copy_files()
 local fileext="$1"
 local dest="$2"
 
-file="`get_filename openvpn-*.*.*.$fileext`"
+file="$(get_filename openvpn-*.*.*.$fileext)"
 if [ -z "$file" ]; then
 echo "** ERROR Failed to find source file"
 exit 5
@@ -106,7 +106,7 @@ sign_file()
 local signkey="$1"
 local srchfile="$2"
 local signtype="$3"
-local file="`get_filename $srchfile`"
+local file="$(get_filename $srchfile)"
 
 echo "-- Signing $file ..."
 case "$signtype" in
@@ -169,7 +169,7 @@ fi
 #
 
 # Clone the remote repository
-workdir="`mktemp -d -p /var/tmp openvpn-build-release-XX`"
+workdir="$(mktemp -d -p /var/tmp openvpn-build-release-XX)"
 cd $workdir
 echo "-- Working directory: $workdir"
 echo "-- git clone $giturl"
diff --git a/tests/t_cltsrv.sh b/tests/t_cltsrv.sh
index 752251e4..1ab3db3e 100755
--- a/tests/t_cltsrv.sh
+++ b/tests/t_cltsrv.sh
@@ -25,14 +25,14 @@ top_builddir="${top_builddir:-..}"
 trap "rm -f log.$$ log.$$.signal ; trap 0 ; exit 77" 1 2 15
 trap "rm -f log.$$ log.$$.signal ; exit 1" 0 3
 addopts=
-case `uname -s` in
+case $(uname -s) in
 FreeBSD)
 # FreeBSD jails map the outgoing IP to the jail IP - we need to
 # allow the real IP unless we want the test to run forever.
-if test "`sysctl 2>/dev/null -n security.jail.jailed`" = 1 \
+if test "$(sysctl 2>/dev/null -n security.jail.jailed)" = 1 \
 || ps -ostate= -p $$ | grep -q J; then
addopts="--float"
-   if test "x`ifconfig | grep inet`" = x ; then
+   if test "x$(ifconfig | grep inet)" = x ; then
echo "###"
echo "### To run the test in a FreeBSD jail, you MUST add an IP 
alias for the jail's IP."
echo "###"
-- 
2.14.1


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] avoid useless assignment

[Antonio Quartulli]

My effort in writing the commit message has been quite poor.

The assignment is useless because 'ret' is re-assigned a few lines later
without ever being read.

Cheers,

On 24/08/17 15:53, Antonio Quartulli wrote:
> Signed-off-by: Antonio Quartulli 
> ---
>  src/openvpn/multi.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
> index 5892ac07..6cdb0110 100644
> --- a/src/openvpn/multi.h
> +++ b/src/openvpn/multi.h
> @@ -633,7 +633,7 @@ multi_process_outgoing_tun(struct multi_context *m, const 
> unsigned int mpp_flags
>  static inline bool
>  multi_process_outgoing_link_dowork(struct multi_context *m, struct 
> multi_instance *mi, const unsigned int mpp_flags)
>  {
> -bool ret = true;
> +bool ret;
>  set_prefix(mi);
>  process_outgoing_link(>context);
>  ret = multi_process_post(m, mi, mpp_flags);
> 

-- 
Antonio Quartulli



signature.asc
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH] fragment.c: simplify boolean expression

[Antonio Quartulli]

!A || (A && B) is equivalent to the simpler !A || B
therefore it is preferable to use the second version as
it is simpler to parse while reading the code.

Signed-off-by: Antonio Quartulli 
---
 src/openvpn/fragment.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/openvpn/fragment.c b/src/openvpn/fragment.c
index 25ffc4cf..36588060 100644
--- a/src/openvpn/fragment.c
+++ b/src/openvpn/fragment.c
@@ -209,7 +209,7 @@ fragment_incoming(struct fragment_master *f, struct buffer 
*buf,
 }
 
 /* is this the first fragment for our sequence number? */
-if (!frag->defined || (frag->defined && frag->max_frag_size != 
size))
+if (!frag->defined || frag->max_frag_size != size)
 {
 frag->defined = true;
 frag->max_frag_size = size;
-- 
2.14.1


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH] avoid useless assignment

[Antonio Quartulli]

Signed-off-by: Antonio Quartulli 
---
 src/openvpn/multi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
index 5892ac07..6cdb0110 100644
--- a/src/openvpn/multi.h
+++ b/src/openvpn/multi.h
@@ -633,7 +633,7 @@ multi_process_outgoing_tun(struct multi_context *m, const 
unsigned int mpp_flags
 static inline bool
 multi_process_outgoing_link_dowork(struct multi_context *m, struct 
multi_instance *mi, const unsigned int mpp_flags)
 {
-bool ret = true;
+bool ret;
 set_prefix(mi);
 process_outgoing_link(>context);
 ret = multi_process_post(m, mi, mpp_flags);
-- 
2.14.1


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel