Re: [Openvpn-devel] [PATCH] bash->bourne script cleanup

[David Sommerseth]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 22/04/10 10:15, Davide Brini wrote:
> On Thursday 22 Apr 2010 09:02:23 David Sommerseth wrote:
> 
>> For future patches, would you mind adding a little bit more descriptive
>> text which can be used as commit log messages.  I do write those commit
>> logs when I find it is needed, but adding a little bit more descriptions
>> of what the patch does is a nice and good habit - no matter how easy the
>> patch looks like ... and it makes my maintenance job a lot easier and
>> quicker.
> 
> Sure, no problem. But just to make it clear (please forgive my ignorance), my 
> original message for the patch contained quite a detailed explanation of what 
> it does. 

Sorry, I was a bit unclear ... the initial patch had good info!  And as
you can see in the commit log, I've used a lot of that text.

> I assume that was too long, and you need a 1-2 lines summary to use 
> for the commit? 

It doesn't need to be a novel, but I do prefer longer ones and not just
1-2 lines.  So not too short, please :)

> Of course this is not to say that the detailed explanation 
> should be missing, only that you need the brief summary as well. Is my 
> understanding correct?

Ideally a commit log should state the problem the patch should fix and
give the basic ideas of how you solved it - without being too technical,
as the patch will describe the technical details.  But things which
might be difficult to understand in the code, should be explained in
"plain English" in the commit log.

If applicable, also add references to mail discussions, chat logs, etc,
if that is relevant and can give more background information.  This is
important when someone wants to investigate the patches later on, maybe
even someone who is fresh to the OpenVPN code.

And if your text can be used as a "copy-paste" into the git commit,
that's what I'd prefer :)

Another thing I've began to add to commit logs are those
"Signed-off-by:" lines for those submitting the patches.  So, please add
"Signed-off-by:" lines as well.




To explain the Signed-off-by and Acked-by process a bit further.  I do
now separate commit Author and sender.  The difference is that the
author field should be a reference to the one who *wrote* the patch.
This person sends it to someone, and would add the first "Signed-off-by"
message as a "signature" of the patch.  This indicates that the author
finds the patch ready to be sent further.

Then the one who receives the patch and finds it good to go further,
will add his/hers "Signed-off-by" reference.  That way we can track who
have been looking at the patch.  Before it goes into the tree, a final
"Acked-by:" note is added, indicating who accepted it for inclusion into
the tree.

So, if John Doe writes a patch, sends it to Jane Doe and she sends it
Bob Boss for inclusion the process would be:

John Doe:
Author: John Doe 
Patch fixing x.y.z

explaining the patch

Signed-off-by: John Doe 



Jane Doe will send it further as:
Author: John Doe 
Patch fixing x.y.z

explaining the patch

Signed-off-by: John Doe 
Signed-off-by: Jane Doe   



And when Bob Boss ACK's the patch, the final commit to be found in the
tree will be:
Author: John Doe 
Patch fixing x.y.z

explaining the patch

Signed-off-by: John Doe 
Signed-off-by: Jane Doe   
Signed-off-by: David Sommerseth 
Acked-by: Bob Boss 



The Acked-by: can also be someone who don't do the final commit[1].  Or
it can be my patches which I've sent for review before including
them[2].  And it can of course be ACKed by the same person who do the
final commit.  The last Signed-off-by should normally be the person the
one who does the final commit.  This is the person  who "touched" the
patch by adding the Acked-by line(s).

However, you should not see any commits which is written by (author) and
Acked-by by the same person.  Then the process have failed to give a
qualified review.

You may ask why we have this "bureaucracy" (which is a fair question!)
It is simply to keep all who send in patches, those who reviews them and
finally accepts them into the source tree more accountable for their
work.  And to document that each accepted patch has been through a
certain set of reviews.

As these patches will go back into SVN, it's not possible to, AFAIK, to
track authors and committers explicitly - so we track it in the commit
log as well.  Just to keep this project as transparent as possible.

For now, the only patches which which "breaks" this in the git tree are
the patches coming in from James' SVN BETA21 branch.  But I trust that
James tracks the patches he pulls in according to the standards he

Re: [Openvpn-devel] [PATCH] bash->bourne script cleanup

[Davide Brini]

On Thursday 22 Apr 2010 09:02:23 David Sommerseth wrote:

> For future patches, would you mind adding a little bit more descriptive
> text which can be used as commit log messages.  I do write those commit
> logs when I find it is needed, but adding a little bit more descriptions
> of what the patch does is a nice and good habit - no matter how easy the
> patch looks like ... and it makes my maintenance job a lot easier and
> quicker.

Sure, no problem. But just to make it clear (please forgive my ignorance), my 
original message for the patch contained quite a detailed explanation of what 
it does. I assume that was too long, and you need a 1-2 lines summary to use 
for the commit? Of course this is not to say that the detailed explanation 
should be missing, only that you need the brief summary as well. Is my 
understanding correct?

Thanks.

-- 
D.

Re: [Openvpn-devel] [PATCH] bash->bourne script cleanup

[David Sommerseth]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 19/04/10 23:37, Davide Brini wrote:
> On Monday 19 April 2010, David Sommerseth wrote:
> 
>> I've done a quick test on one of my connections on Fedora 12 without any
>> resolvconf package (meaning it invokes the simple cp approach), and it
>> worked like a charm.
>>
>> Applied to bugfix2.1 and merged into allmerged.
>> Commit a9c9a89e96dc1e4e843e05ecadc4349b81606b06
> 
> Sorry, I just realized that I didn't change the sha-bang line in client.down. 
> Apologies. Fix attached.

No problem!  And thanks a lot for fixing it!  I should have caught that
one during my review as well.  I looked at "everything", but managed to
not notice this one as well.  Anyway, I gave it an ACK and it's now in
the bugfix2.1 branch.  As this is a non-critical fix, I'll delay pushing
it to allmerged until more patches comes in.  You'll find it as commit
2d9c14b435ae4083aa3f6bce803ede27cce2b6cb

For future patches, would you mind adding a little bit more descriptive
text which can be used as commit log messages.  I do write those commit
logs when I find it is needed, but adding a little bit more descriptions
of what the patch does is a nice and good habit - no matter how easy the
patch looks like ... and it makes my maintenance job a lot easier and
quicker.

I won't reject good patches due to missing commit messages, but it I'm
not picking these patches first for inclusion - as they require more
work.  So the easier the patches are for me to work with, the quicker
they go into the tree after the required review.


kind regards,

David Sommerseth
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkvQAowACgkQDC186MBRfrrhCQCfdrZTU2ilBNQ+K8FC8iDguloq
X04An0/BxnoqesB1CqCS+BzYdreb8rrh
=q4xW
-END PGP SIGNATURE-

Re: [Openvpn-devel] [PATCH] bash->bourne script cleanup

[Davide Brini]

On Monday 19 April 2010, David Sommerseth wrote:

> I've done a quick test on one of my connections on Fedora 12 without any
> resolvconf package (meaning it invokes the simple cp approach), and it
> worked like a charm.
> 
> Applied to bugfix2.1 and merged into allmerged.
> Commit a9c9a89e96dc1e4e843e05ecadc4349b81606b06

Sorry, I just realized that I didn't change the sha-bang line in client.down. 
Apologies. Fix attached.

-- 
D.
--- openvpn-2.1.1/contrib/pull-resolv-conf/client.down  2010-03-11 21:32:09.0 +
+++ openvpn-2.1.1-a/contrib/pull-resolv-conf/client.down2010-04-19 22:33:53.0 +0100
@@ -1,4 +1,4 @@
-#!/bin/bash
+#!/bin/sh

 # Copyright (c) 2005-2009 OpenVPN Technologies, Inc.
 # Licensed under the GPL version 2

Re: [Openvpn-devel] [PATCH] bash->bourne script cleanup

[David Sommerseth]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 11/03/10 22:41, Davide Brini wrote:
> On Wednesday 10 March 2010, David Sommerseth wrote:
> 
>>> Well, I was actually going to write a patch, but shortly after starting I
>>> found out that it would end up being essentially the same as Gentoo's
>>> scripts. Would it be worth separately maintaining something that has
>>> already been written somewhere else?
>>
>> I would say that if there are things which are distro related, they
>> should either be found only in that distribution or we can consider (if
>> it is considered important by more people) to put distro specific stuff
>> into a separate folder in the OpenVPN source tree.
>>
>> If it is possible to get some up/down scripts which are generic for the
>> vast majority of POSIX sh based distributions, that would be the
>> preferred approach.  If not, then we are back to where we started :)
> 
> Ok, here it goes (it's against 2.1.1). As said, it's basically a complete 
> rewrite that draws many ideas from the Gentoo scripts. These are the main 
> differences from the "old" client.{up,down} scripts:
> 
> - No more bashisms (AFAICT). Should work with any POSIX-compatible shell 
> (which means "almost all reasonably recent shells"), though I've only tested 
> with bash and dash.
> 
> - Unnecessary calls to external tools (sed) removed 
> 
> - Manages multiple DNS and DOMAIN options. Each DNS option becomes a 
> "nameserver" line in the new resolv.conf (up to a maximum of 3). If there's a 
> single DOMAIN option, it becomes a "domain" line in resolv.conf; otherwise, 
> all the domains are listed in a "search" line in resolv.conf (eg "search 
> foo.com example.net").
> 
> - Client.up renames the existing resolv.conf and creates a brand new one; 
> client.down restores it from the saved copy when the VPN terminates (the 
> usual 
> rules about running as root apply). This is how Gentoo does that; the old 
> scripts instead added/removed some lines at the beginning of the file, which 
> looks a less clean approach to me. The rename approach also dramatically 
> simplifies and shortens client.down, as you'll see.
> 
> - Uses resolvconf if it's available (detected by the presence of 
> /sbin/resolvconf) rather than writing to resolv.conf directly. Not sure 
> whether this is a Linux-only thing or other systems use it though.
> 
> A doubt I have is: should the script output its errors as it does now? If 
> yes, 
> is it possible to somehow send them to the main OpenVPN log so they appear 
> among the other normal messages?
> 
> Let me know what you think.

ACK!

I've done a quick test on one of my connections on Fedora 12 without any
resolvconf package (meaning it invokes the simple cp approach), and it
worked like a charm.

Applied to bugfix2.1 and merged into allmerged.
Commit a9c9a89e96dc1e4e843e05ecadc4349b81606b06


kind regards,

David Sommerseth
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkvMwe0ACgkQDC186MBRfroWOwCeMFu3NO/s6UDeTSjGkmde/DpQ
MtsAn0rqsF7B5/4RIjcF4k7zyoryvhsw
=RuVf
-END PGP SIGNATURE-

Re: [Openvpn-devel] [PATCH] bash->bourne script cleanup

[Davide Brini]

On Wednesday 10 March 2010, David Sommerseth wrote:

> > Well, I was actually going to write a patch, but shortly after starting I
> > found out that it would end up being essentially the same as Gentoo's
> > scripts. Would it be worth separately maintaining something that has
> > already been written somewhere else?
> 
> I would say that if there are things which are distro related, they
> should either be found only in that distribution or we can consider (if
> it is considered important by more people) to put distro specific stuff
> into a separate folder in the OpenVPN source tree.
> 
> If it is possible to get some up/down scripts which are generic for the
> vast majority of POSIX sh based distributions, that would be the
> preferred approach.  If not, then we are back to where we started :)

Ok, here it goes (it's against 2.1.1). As said, it's basically a complete 
rewrite that draws many ideas from the Gentoo scripts. These are the main 
differences from the "old" client.{up,down} scripts:

- No more bashisms (AFAICT). Should work with any POSIX-compatible shell 
(which means "almost all reasonably recent shells"), though I've only tested 
with bash and dash.

- Unnecessary calls to external tools (sed) removed 

- Manages multiple DNS and DOMAIN options. Each DNS option becomes a 
"nameserver" line in the new resolv.conf (up to a maximum of 3). If there's a 
single DOMAIN option, it becomes a "domain" line in resolv.conf; otherwise, 
all the domains are listed in a "search" line in resolv.conf (eg "search 
foo.com example.net").

- Client.up renames the existing resolv.conf and creates a brand new one; 
client.down restores it from the saved copy when the VPN terminates (the usual 
rules about running as root apply). This is how Gentoo does that; the old 
scripts instead added/removed some lines at the beginning of the file, which 
looks a less clean approach to me. The rename approach also dramatically 
simplifies and shortens client.down, as you'll see.

- Uses resolvconf if it's available (detected by the presence of 
/sbin/resolvconf) rather than writing to resolv.conf directly. Not sure 
whether this is a Linux-only thing or other systems use it though.

A doubt I have is: should the script output its errors as it does now? If yes, 
is it possible to somehow send them to the main OpenVPN log so they appear 
among the other normal messages?

Let me know what you think.

In case lines wrap, which is likely, I'm also attaching the file.

diff -burp openvpn-2.1.1/contrib/pull-resolv-conf/client.up openvpn-2.1.1-
a/contrib/pull-resolv-conf/client.up > clientup.patch
--- openvpn-2.1.1/contrib/pull-resolv-conf/client.up2009-10-01 
19:02:17.0 +0100
+++ openvpn-2.1.1-a/contrib/pull-resolv-conf/client.up  2010-03-11 
21:32:03.0 +
@@ -1,4 +1,4 @@
-#!/bin/bash
+#!/bin/sh

 # Copyright (c) 2005-2009 OpenVPN Technologies, Inc.
 # Licensed under the GPL version 2
@@ -14,7 +14,6 @@
 # Place this in /etc/openvpn/client.up
 # Then, add the following to your /etc/openvpn/.conf:
 #   client
-#   pull dhcp-options
 #   up /etc/openvpn/client.up
 # Next, "chmod a+x /etc/openvpn/client.up"

@@ -22,8 +21,8 @@
 # Note that this script is best served with the companion "client.down"
 # script.

-# Only tested on Gentoo Linux 2005.0 with OpenVPN 2.0
-# It should work with any GNU/Linux with /etc/resolv.conf
+# Tested under Debian lenny with OpenVPN 2.1_rc11
+# It should work with any UNIX with a POSIX sh, /etc/resolv.conf or 
resolvconf

 # This runs with the context of the OpenVPN UID/GID
 # at the time of execution. This generally means that
@@ -38,38 +37,64 @@
 # init variables

 i=1
-j=1
-unset fopt
-unset dns
-unset opt
-
-# Convert ENVs to an array
-
-while fopt=foreign_option_$i; [ -n "${!fopt}" ]; do
-{
-   opt[i-1]=${!fopt}
-   case ${opt[i-1]} in
-   *DOMAIN* ) domain=`echo ${opt[i-1]} | \
-   sed -e 's/dhcp-option DOMAIN //g'` ;;
-   *DNS*) dns[j-1]=`echo ${opt[i-1]} | \
-   sed -e 's/dhcp-option DNS //g'`
-  let j++ ;;
+domains=
+fopt=
+ndoms=0
+nns=0
+nl='
+'
+
+# $foreign_option_ is something like
+# "dhcp-option DOMAIN example.com" (multiple allowed)
+# or
+# "dhcp-option DNS 10.10.10.10" (multiple allowed)
+
+# each DNS option becomes a "nameserver" option in resolv.con
+# if we get one DOMAIN, that becomes "domain" in resolv.conf
+# if we get multiple DOMAINS, those become "search" lines in resolv.conf
+
+while true; do
+  eval fopt=\$foreign_option_${i}
+  [ -z "${fopt}" ] && break
+
+  case ${fopt} in
+   dhcp-option\ DOMAIN\ *)
+   ndoms=$((ndoms + 1))
+   domains="${domains} ${fopt#dhcp-option DOMAIN }"
+   ;;
+   dhcp-option\ DNS\ *)
+   nns=$((nns + 1))
+   if [ $nns -le 3 ]; then
+ dns="${dns}${dns:+$nl}nameserver ${fopt#dhcp-option DNS }"
+   else
+ printf "%s\n" "Too many nameservers - ignoring after third" >&2
+   fi
+   ;;

Re: [Openvpn-devel] [PATCH] bash->bourne script cleanup

[David Sommerseth]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 10/03/10 17:34, Davide Brini wrote:
>> > >>The only scripts that actually require bash are
>> > >> contrib/pull-resolv-conf/client.{up,down} ; they use the ${!var}
>> > >> variable indirection feature.
>>>
>>> > > Not only that, they also use arrays, which are not part of standard sh.
>>> > >
>>> > > For a POSIX-sh compliant solution thad does mostly the same thing (and
>>> > > handles multiple DOMAIN options as well, which client.{up,down} doesn't
>>> > > support), I suggest  you look into the Gentoo-provided {up,down}.sh
>>> > > scripts (though they contain some Gentoo-specific parts, but those are
>>> > > easily removed).
>> > 
>> > Agreed!  If you have time to look at a patch for it, I'd appreciate
>> > that.  It will go in as a separate patch, to keep the patch flow as
>> > simple as possible.
>> >
> Well, I was actually going to write a patch, but shortly after starting I 
> found out that it would end up being essentially the same as Gentoo's scripts.
> Would it be worth separately maintaining something that has already been 
> written somewhere else?

I would say that if there are things which are distro related, they
should either be found only in that distribution or we can consider (if
it is considered important by more people) to put distro specific stuff
into a separate folder in the OpenVPN source tree.

If it is possible to get some up/down scripts which are generic for the
vast majority of POSIX sh based distributions, that would be the
preferred approach.  If not, then we are back to where we started :)

Did that answer your question?


kind regards,

David Sommerseth
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkuYF54ACgkQDC186MBRfrqHwACgpMmlJ9ZQU/CvppJ+3pq1s/ql
IqwAoJEk28vBEMeJn44K8sLoJ33h/rqs
=eOoy
-END PGP SIGNATURE-

Re: [Openvpn-devel] [PATCH] bash->bourne script cleanup

[Davide Brini]

On Wednesday 10 Mar 2010 15:45:32 David Sommerseth wrote:
> On 01/03/10 00:26, Davide Brini wrote:
> > On Sunday 28 February 2010, David Sommerseth wrote:
> >> From: Dan Nelson 
> >>
> >> Many of the scripts in the openvpn source have their shell set to
> >> /bin/bash, but only two use bash features. The attached patch (against
> >> openvpn-2.1_rc9) sets the shell on the rest of the scripts to /bin/sh
> >> for better portability. The only scripts that actually require bash are
> >> contrib/pull-resolv-conf/client.{up,down} ; they use the ${!var}
> >> variable indirection feature.
> >
> > Not only that, they also use arrays, which are not part of standard sh.
> >
> > For a POSIX-sh compliant solution thad does mostly the same thing (and
> > handles multiple DOMAIN options as well, which client.{up,down} doesn't
> > support), I suggest  you look into the Gentoo-provided {up,down}.sh
> > scripts (though they contain some Gentoo-specific parts, but those are
> > easily removed).
> 
> Agreed!  If you have time to look at a patch for it, I'd appreciate
> that.  It will go in as a separate patch, to keep the patch flow as
> simple as possible.

Well, I was actually going to write a patch, but shortly after starting I 
found out that it would end up being essentially the same as Gentoo's scripts.
Would it be worth separately maintaining something that has already been 
written somewhere else?

Note I'm not being argumentative, I'm just trying to understand what pople 
think, what the policy is, and what would be the best thing to do.
It's true that these are just two short scripts, and maintaining them would be 
no big deal anyway.

Thanks!

-- 
D.

Re: [Openvpn-devel] [PATCH] bash->bourne script cleanup

[David Sommerseth]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 01/03/10 00:26, Davide Brini wrote:
> On Sunday 28 February 2010, David Sommerseth wrote:
>> From: Dan Nelson 
>>
>> Many of the scripts in the openvpn source have their shell set to
>> /bin/bash, but only two use bash features. The attached patch (against
>> openvpn-2.1_rc9) sets the shell on the rest of the scripts to /bin/sh for
>> better portability. The only scripts that actually require bash are
>> contrib/pull-resolv-conf/client.{up,down} ; they use the ${!var} variable
>> indirection feature.
> 
> Not only that, they also use arrays, which are not part of standard sh.
> 
> For a POSIX-sh compliant solution thad does mostly the same thing (and 
> handles 
> multiple DOMAIN options as well, which client.{up,down} doesn't support), I 
> suggest  you look into the Gentoo-provided {up,down}.sh scripts (though they 
> contain some Gentoo-specific parts, but those are easily removed).
> 

Agreed!  If you have time to look at a patch for it, I'd appreciate
that.  It will go in as a separate patch, to keep the patch flow as
simple as possible.


kind regards,

David Sommerseth

-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkuXvpwACgkQDC186MBRfrrvAACdEkubTxrMsJUPLErrp2WVckDP
vt8AoJx8VGtZ37YMiLI8AR+wXUnEHN8g
=W9YB
-END PGP SIGNATURE-

Re: [Openvpn-devel] [PATCH] bash->bourne script cleanup

[David Sommerseth]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 28/02/10 22:12, David Sommerseth wrote:
> From: Dan Nelson 
> 
> Many of the scripts in the openvpn source have their shell set to
> /bin/bash, but only two use bash features. The attached patch (against
> openvpn-2.1_rc9) sets the shell on the rest of the scripts to /bin/sh for
> better portability. The only scripts that actually require bash are
> contrib/pull-resolv-conf/client.{up,down} ; they use the ${!var} variable
> indirection feature.
> 
> sf.net tracker:
> 
> 
> Signed-off-by: David Sommerseth 
> ---
>  easy-rsa/2.0/build-ca   |2 +-
>  easy-rsa/2.0/build-dh   |2 +-
>  easy-rsa/2.0/build-inter|2 +-
>  easy-rsa/2.0/build-key  |2 +-
>  easy-rsa/2.0/build-key-pass |2 +-
>  easy-rsa/2.0/build-key-pkcs12   |2 +-
>  easy-rsa/2.0/build-key-server   |2 +-
>  easy-rsa/2.0/build-req  |2 +-
>  easy-rsa/2.0/build-req-pass |2 +-
>  easy-rsa/2.0/clean-all  |2 +-
>  easy-rsa/2.0/inherit-inter  |2 +-
>  easy-rsa/2.0/list-crl   |2 +-
>  easy-rsa/2.0/revoke-full|2 +-
>  easy-rsa/2.0/sign-req   |2 +-
>  sample-config-files/firewall.sh |2 +-
>  sample-scripts/bridge-start |2 +-
>  sample-scripts/bridge-stop  |2 +-
>  17 files changed, 17 insertions(+), 17 deletions(-)
> 

Patch applied to bugfix2.1 branch, merged into allmerged.
Commit b75eb4c5a1cd31effcad9b6fa686dbd43527730a


kind regards,

David Sommerseth


-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkuXvlIACgkQDC186MBRfrq19wCggOAdo60oWyurnzc76hLwL53k
yDIAn2rRw6vNPz++dLnyjjZ6UvNp+vIs
=mGYY
-END PGP SIGNATURE-