Re: [Openvpn-devel] [PATCH v4] push-peer-info: rearrange function generating peer info

[Gert Doering]

Hi,

On Mon, Sep 26, 2022 at 08:46:50AM +0200, Antonio Quartulli wrote:
> On 26/09/2022 08:39, Gert Doering wrote:
> > It might seem elegant, to do this with a fall-through switch/case, but
> > it turns out to be not very elegant due to the restrictions on local
> > variables.   Also, if someone ends up setting push-peer-info to 4,
> > they will get "nothing at all" now, instead of "everything".
> 
> This is set programmatically, not by the user. So right now you can't 
> set a value higher than 3.

Good point.

> > To keep to your idea of doing this in blocks "3", "2+3", "1+2+3" one
> > could do
> > 
> > int detail = session->opt->push_peer_info_detail;
> > if (detail >= 3)
> > {
> > ...
> > }
> > if (detail > 2)
> > {
> > ...
> > }
> > if (detail > 1)
> > {
> > ...
> > }
> > 
> > so it has less twisted conditions.
> 
> Since >3 is not possible, this is exactly the same as a switch/case, 
> (just reimplemented with ifs).

True, but less ugly.  I know you really like the switch/case thing, but I
find it much less compelling for a flow with "everything is a fall-through".

> I am not sure why this switch/case is different from the others?
> We also have {} in other case blocks for the very same reason.
> 
> If we feel having the {} is ugly, we should agree on what to do 
> globally, no? Not just change this hunk.

You are introducing something here, which is intended to make the result
more readable.  If that doesn't work, pointing to other ugly parts of 
openvpn code is not a very strong argument...

I'm sure we all agree that for every possible C style sin, you could
find a place where someone clever did this in OpenVPN (I specially like
#ifdef right in the middle of a complex if/else expression) - but for
new code, this should not be the standard to look at.


Anyway, I'm not insisting on this, just stating my thoughts.

What we should - indeed - not do is have inconsistent constructs, with
"some case: having {}, some not" - so in that case, better bring the
local variables before the switch, so we can have all case: without
brackets?

gert
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
 Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany g...@greenie.muc.de


signature.asc
Description: PGP signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH v4] push-peer-info: rearrange function generating peer info

[Antonio Quartulli]

Hi,

On 26/09/2022 08:39, Gert Doering wrote:

Hi,

On Mon, Sep 26, 2022 at 12:13:57AM +0200, Antonio Quartulli wrote:

For now I will just remove the brackets from case 2, where they are not
needed.


TBH, I think we should just not use switch/case here.

It might seem elegant, to do this with a fall-through switch/case, but
it turns out to be not very elegant due to the restrictions on local
variables.   Also, if someone ends up setting push-peer-info to 4,
they will get "nothing at all" now, instead of "everything".


This is set programmatically, not by the user. So right now you can't 
set a value higher than 3.


Actually it may make sense to switch to an enum instead of a plain int, 
so that the compiler will warn us if the switch/case is not handling all 
the values (an 'if' won't do that).




To keep to your idea of doing this in blocks "3", "2+3", "1+2+3" one
could do

int detail = session->opt->push_peer_info_detail;
if (detail >= 3)
{
...
}
if (detail > 2)
{
...
}
if (detail > 1)
{
...
}

so it has less twisted conditions.


Since >3 is not possible, this is exactly the same as a switch/case, 
(just reimplemented with ifs).


I am not sure why this switch/case is different from the others?
We also have {} in other case blocks for the very same reason.

If we feel having the {} is ugly, we should agree on what to do 
globally, no? Not just change this hunk.


Cheers,

--
Antonio Quartulli


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH v4] push-peer-info: rearrange function generating peer info

[Gert Doering]

Hi,

On Mon, Sep 26, 2022 at 12:13:57AM +0200, Antonio Quartulli wrote:
> For now I will just remove the brackets from case 2, where they are not 
> needed.

TBH, I think we should just not use switch/case here.

It might seem elegant, to do this with a fall-through switch/case, but
it turns out to be not very elegant due to the restrictions on local
variables.   Also, if someone ends up setting push-peer-info to 4,
they will get "nothing at all" now, instead of "everything".

To keep to your idea of doing this in blocks "3", "2+3", "1+2+3" one
could do

   int detail = session->opt->push_peer_info_detail;
   if (detail >= 3)
   {
   ...
   }
   if (detail > 2)
   {
   ...
   }
   if (detail > 1)
   {
   ...
   }

so it has less twisted conditions.

gert

-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
 Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany g...@greenie.muc.de


signature.asc
Description: PGP signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH v4] push-peer-info: rearrange function generating peer info

[Antonio Quartulli]

Hi,

On 20/09/2022 21:57, Selva Nair wrote:



On Tue, Sep 20, 2022 at 3:26 PM Antonio Quartulli > wrote:


Hi,

On 20/09/2022 18:42, Gert Doering wrote:
 > Hi,
 >
 > On Mon, Sep 19, 2022 at 12:06:18AM +0200, Antonio Quartulli wrote:
 >> +    switch (session->opt->push_peer_info_detail)
 >>       {
 >> -        /* push version */
 >> -        buf_printf(, "IV_VER=%s\n", PACKAGE_VERSION);
 >> +        case 3:
 >> +        {
 > ...
 >> +        }
 >>
 >> +        /* fall through */
 >
 > These {} brackets are not needed by C or our style guide.

they are required when the first instruction after a case label is a
variable declaration. So this is needed for case 3, but could be
avoided
for the other 2.


IMO, we should have a policy of avoiding variable declarations within 
case: clauses.


Whether its the first line after the case: label or not, if the scope is 
not limited using {}, it leads to possible jumping over initialization. 
Its so easy to overlook that. Unfortunately C (unlike C++) complains 
loudly only if the declaration is next to the label.  One could always 
use {},  but that makes code hard to read. I think its much better to 
require that all locals used within a switch block must be defined 
before all case: labels.


I agree with that. To be a bit more strict: I always preferred declaring 
all variables at the beginning of the function.


You may have jumps in a function too (having the same issue as the 
switch/case) or you may also hide another variable (i.e. function 
argument) without noticing.


However, this is a broader topic that should be discussed during a 
community meeting IMHO.


For now I will just remove the brackets from case 2, where they are not 
needed.


Cheers,



Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


--
Antonio Quartulli


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH v4] push-peer-info: rearrange function generating peer info

[Selva Nair]

On Tue, Sep 20, 2022 at 3:26 PM Antonio Quartulli  wrote:

> Hi,
>
> On 20/09/2022 18:42, Gert Doering wrote:
> > Hi,
> >
> > On Mon, Sep 19, 2022 at 12:06:18AM +0200, Antonio Quartulli wrote:
> >> +switch (session->opt->push_peer_info_detail)
> >>   {
> >> -/* push version */
> >> -buf_printf(, "IV_VER=%s\n", PACKAGE_VERSION);
> >> +case 3:
> >> +{
> > ...
> >> +}
> >>
> >> +/* fall through */
> >
> > These {} brackets are not needed by C or our style guide.
>
> they are required when the first instruction after a case label is a
> variable declaration. So this is needed for case 3, but could be avoided
> for the other 2.
>

IMO, we should have a policy of avoiding variable declarations within case:
clauses.

Whether its the first line after the case: label or not, if the scope is
not limited using {}, it leads to possible jumping over initialization. Its
so easy to overlook that. Unfortunately C (unlike C++) complains loudly
only if the declaration is next to the label.  One could always use {},
but that makes code hard to read. I think its much better to require that
all locals used within a switch block must be defined before all case:
labels.

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH v4] push-peer-info: rearrange function generating peer info

[Antonio Quartulli]

Hi,

On 20/09/2022 18:42, Gert Doering wrote:

Hi,

On Mon, Sep 19, 2022 at 12:06:18AM +0200, Antonio Quartulli wrote:

+switch (session->opt->push_peer_info_detail)
  {
-/* push version */
-buf_printf(, "IV_VER=%s\n", PACKAGE_VERSION);
+case 3:
+{

...

+}
  
+/* fall through */


These {} brackets are not needed by C or our style guide.


they are required when the first instruction after a case label is a 
variable declaration. So this is needed for case 3, but could be avoided 
for the other 2.


Cheers,

--
Antonio Quartulli


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel