[Openvpn-devel] [PATCH] route: cleanup codestyle and make code more readable
This patch does not introduce any functional change.
The code in route.c seems to have been written in different
periods by different people, without sticking to a clear
codestyle. For this reason the code in this file in not
consistent at all.
Clean it up by:
- removing spaces from function invocations
- cutting line longer than 80 chars (where possible)
- moving function arguments on the same line when there is enough space
- adding empty line between var declarations and code
- adding empty line between code and final return
- adding empty line to make the code less sticky and easier to parse
Signed-off-by: Antonio Quartulli
---
Yes, this is a quite big patch. However, since we are planning a big
restructuring of the route.c file, it is better to take care of the
style in a separate patch (this) so that later we don't need to mixup cleanups
and refactoring.
Note that this patch is based on master plus the following patches:
- ensure function declarations are compiled with their definitions
- fix a couple of typ0s in comments and strings
- route: avoid definition of unused variables in certain configurations
- convert *_inline attributes to bool
- reformatting: fix style in crypto*.{c, h}
- Allow learning iroutes with network made up of all 0s (only if netbits < 8)
- ifconfig-ipv6(-push): allow using hostnames
Applying this patch without the above, might lead to screams,
natural disasters and endless nightmares.
Travis-ci has successfully built the entire matrix without complaining.
I'd really avoid to merge this into release/2.4 as it is quite risky.
src/openvpn/route.c | 1526 +--
1 file changed, 752 insertions(+), 774 deletions(-)
diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index 8c71e6ec..605c367c 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -56,15 +56,21 @@ static bool add_route_service(const struct route_ipv4 *,
const struct tuntap *);
static bool del_route_service(const struct route_ipv4 *, const struct tuntap
*);
-static bool add_route_ipv6_service(const struct route_ipv6 *, const struct
tuntap *);
+static bool add_route_ipv6_service(const struct route_ipv6 *,
+ const struct tuntap *);
-static bool del_route_ipv6_service(const struct route_ipv6 *, const struct
tuntap *);
+static bool del_route_ipv6_service(const struct route_ipv6 *,
+ const struct tuntap *);
#endif
-static void delete_route(struct route_ipv4 *r, const struct tuntap *tt,
unsigned int flags, const struct route_gateway_info *rgi, const struct env_set
*es);
+static void delete_route(struct route_ipv4 *r, const struct tuntap *tt,
+unsigned int flags,
+const struct route_gateway_info *rgi,
+const struct env_set *es);
-static void get_bypass_addresses(struct route_bypass *rb, const unsigned int
flags);
+static void get_bypass_addresses(struct route_bypass *rb,
+const unsigned int flags);
#ifdef ENABLE_DEBUG
@@ -110,8 +116,10 @@ struct route_option_list *
new_route_option_list(struct gc_arena *a)
{
struct route_option_list *ret;
+
ALLOC_OBJ_CLEAR_GC(ret, struct route_option_list, a);
ret->gc = a;
+
return ret;
}
@@ -119,8 +127,10 @@ struct route_ipv6_option_list *
new_route_ipv6_option_list(struct gc_arena *a)
{
struct route_ipv6_option_list *ret;
+
ALLOC_OBJ_CLEAR_GC(ret, struct route_ipv6_option_list, a);
ret->gc = a;
+
return ret;
}
@@ -135,22 +145,28 @@ struct route_option_list *
clone_route_option_list(const struct route_option_list *src, struct gc_arena
*a)
{
struct route_option_list *ret;
+
ALLOC_OBJ_GC(ret, struct route_option_list, a);
*ret = *src;
+
return ret;
}
struct route_ipv6_option_list *
-clone_route_ipv6_option_list(const struct route_ipv6_option_list *src, struct
gc_arena *a)
+clone_route_ipv6_option_list(const struct route_ipv6_option_list *src,
+struct gc_arena *a)
{
struct route_ipv6_option_list *ret;
+
ALLOC_OBJ_GC(ret, struct route_ipv6_option_list, a);
*ret = *src;
+
return ret;
}
void
-copy_route_option_list(struct route_option_list *dest, const struct
route_option_list *src, struct gc_arena *a)
+copy_route_option_list(struct route_option_list *dest,
+ const struct route_option_list *src, struct gc_arena *a)
{
*dest = *src;
dest->gc = a;
@@ -169,15 +185,16 @@ static const char *
route_string(const struct route_ipv4 *r, struct gc_arena *gc)
{
struct buffer out = alloc_buf_gc(256, gc);
+
buf_printf(&out, "ROUTE network %s netmask %s gateway %s",
print_in_addr_t(r->network, 0, gc),
print_in_addr_t(r->netmask, 0, gc),
- print_in_addr_t(r->gateway, 0, gc)
- );
+ print_in_addr_t(r->gateway,
Re: [Openvpn-devel] [PATCH] route: cleanup codestyle and make code more readable
On 23/08/17 07:30, Antonio Quartulli wrote:
> This patch does not introduce any functional change.
>
> The code in route.c seems to have been written in different
> periods by different people, without sticking to a clear
> codestyle. For this reason the code in this file in not
> consistent at all.
>
> Clean it up by:
> - removing spaces from function invocations
> - cutting line longer than 80 chars (where possible)
> - moving function arguments on the same line when there is enough space
> - adding empty line between var declarations and code
> - adding empty line between code and final return
> - adding empty line to make the code less sticky and easier to parse
>
> Signed-off-by: Antonio Quartulli
> ---
>
> Yes, this is a quite big patch. However, since we are planning a big
> restructuring of the route.c file, it is better to take care of the
> style in a separate patch (this) so that later we don't need to mixup cleanups
> and refactoring.
>
> Note that this patch is based on master plus the following patches:
>
> - ensure function declarations are compiled with their definitions
> - fix a couple of typ0s in comments and strings
> - route: avoid definition of unused variables in certain configurations
> - convert *_inline attributes to bool
> - reformatting: fix style in crypto*.{c, h}
> - Allow learning iroutes with network made up of all 0s (only if netbits < 8)
> - ifconfig-ipv6(-push): allow using hostnames
>
>
> Applying this patch without the above, might lead to screams,
> natural disasters and endless nightmares.
I got it applying quite nicely (working my way through more patches
now). And yes, I like that we clean up the coding style further in this
file. But unfortunately, I'll have to say NAK in this round.
- Many places you replace spaces with tabs.
- There are several scenarios where our uncrustify config actually
improves your patch further (see the attachment).
- And the contradictions like the ones below
> -static void delete_route(struct route_ipv4 *r, const struct tuntap *tt,
> unsigned int flags, const struct route_gateway_info *rgi, const struct
> env_set *es);
> +static void delete_route(struct route_ipv4 *r, const struct tuntap *tt,
> + unsigned int flags,
> + const struct route_gateway_info *rgi,
> + const struct env_set *es);
vs
> static void
> -delete_route(struct route_ipv4 *r,
> - const struct tuntap *tt,
> - unsigned int flags,
> - const struct route_gateway_info *rgi,
> - const struct env_set *es)
> +delete_route(struct route_ipv4 *r, const struct tuntap *tt, unsigned int
> flags,
> + const struct route_gateway_info *rgi, const struct env_set *es)
I think the change you do in the former one is also more readable than
squeezing everything into as few lines as possible, especially when
there's lots of arguments.
Our uncrustify config doesn't touch these details of function
declarations, as tun.c and route.c is fairly extreme in variations here.
So we let that pass on the reformatting patch before the v2.4 release,
to take care of them manually, as we didn't spend much extra time
looking at more tweaks for uncrustify to make the result readable. But
I'm not sure we documented our preferences on function declarations, I
don't recall that now.
Even though we are not united in the use of uncrustify after the
reformatting patches we did in December, I think it makes sense to at
least double check what uncrustify would change and consider those. The
lesser the gap is to that result, the easier it will be to have a
consistent coding style over the complete code base.
For reference, the uncrustify command line I used was:
$ uncrustify -c dev-tools/uncrustify.conf \
--no-backup -l C -p debug.uncr \
src/openvpn/route.c
--
kind regards,
David Sommerseth
OpenVPN Technologies, Inc
diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index 605c367c..3ae75b1b 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -57,20 +57,20 @@ static bool add_route_service(const struct route_ipv4 *, const struct tuntap *);
static bool del_route_service(const struct route_ipv4 *, const struct tuntap *);
static bool add_route_ipv6_service(const struct route_ipv6 *,
- const struct tuntap *);
+ const struct tuntap *);
static bool del_route_ipv6_service(const struct route_ipv6 *,
- const struct tuntap *);
+ const struct tuntap *);
#endif
static void delete_route(struct route_ipv4 *r, const struct tuntap *tt,
- unsigned int flags,
- const struct route_gateway_info *rgi,
- const struct env_set *es);
+ unsigned int flags,
+ const struct route_gateway_info *rgi,
+ const struct env_set *es);
static void get_bypass_addresses(struct route_
Re: [Openvpn-devel] [PATCH] route: cleanup codestyle and make code more readable
Hi,
On 07/09/17 04:45, David Sommerseth wrote:
> On 23/08/17 07:30, Antonio Quartulli wrote:
>> This patch does not introduce any functional change.
>>
>> The code in route.c seems to have been written in different
>> periods by different people, without sticking to a clear
>> codestyle. For this reason the code in this file in not
>> consistent at all.
>>
>> Clean it up by:
>> - removing spaces from function invocations
>> - cutting line longer than 80 chars (where possible)
>> - moving function arguments on the same line when there is enough space
>> - adding empty line between var declarations and code
>> - adding empty line between code and final return
>> - adding empty line to make the code less sticky and easier to parse
>>
>> Signed-off-by: Antonio Quartulli
>> ---
>>
>> Yes, this is a quite big patch. However, since we are planning a big
>> restructuring of the route.c file, it is better to take care of the
>> style in a separate patch (this) so that later we don't need to mixup
>> cleanups
>> and refactoring.
>>
>> Note that this patch is based on master plus the following patches:
>>
>> - ensure function declarations are compiled with their definitions
>> - fix a couple of typ0s in comments and strings
>> - route: avoid definition of unused variables in certain configurations
>> - convert *_inline attributes to bool
>> - reformatting: fix style in crypto*.{c, h}
>> - Allow learning iroutes with network made up of all 0s (only if netbits < 8)
>> - ifconfig-ipv6(-push): allow using hostnames
>>
>>
>> Applying this patch without the above, might lead to screams,
>> natural disasters and endless nightmares.
>
> I got it applying quite nicely (working my way through more patches
> now). And yes, I like that we clean up the coding style further in this
> file. But unfortunately, I'll have to say NAK in this round.
>
I am not sure that what you get by applying this patch without the rest
is 100% correct. But I can rebase this patch on top of master if you are
willing to work on this one before the others mentioned in the patch
message.
> - Many places you replace spaces with tabs.
I must have messed up my vim style file recently. will recheck. Thanks!
> - There are several scenarios where our uncrustify config actually
> improves your patch further (see the attachment).
Oh ok. I never thought about re-running uncrustify. Thanks for the
suggestion.
> - And the contradictions like the ones below
>
>> -static void delete_route(struct route_ipv4 *r, const struct tuntap *tt,
>> unsigned int flags, const struct route_gateway_info *rgi, const struct
>> env_set *es);
>> +static void delete_route(struct route_ipv4 *r, const struct tuntap *tt,
>> + unsigned int flags,
>> + const struct route_gateway_info *rgi,
>> + const struct env_set *es);
>
> vs
>
>> static void
>> -delete_route(struct route_ipv4 *r,
>> - const struct tuntap *tt,
>> - unsigned int flags,
>> - const struct route_gateway_info *rgi,
>> - const struct env_set *es)
>> +delete_route(struct route_ipv4 *r, const struct tuntap *tt, unsigned int
>> flags,
>> + const struct route_gateway_info *rgi, const struct env_set *es)
>
> I think the change you do in the former one is also more readable than
> squeezing everything into as few lines as possible, especially when
> there's lots of arguments.
Honestly I only applied the formal rule "a line should not be longer
than 80 chars". This is what I grasped from our codestyle. Is this not
correct?
In the first case less arguments fit on the first line because of the
return type. In the second case there is more space available.
This is a contradiction given by the codingstyle itself.
Should we use another rule to decide how function declarations and
definitions should look like? I think in the rest of the code we tried
to apply the line length rule too.
Let me know how this should be arranged otherwise (and let's write it in
the codingstyle page) :)
>
> Our uncrustify config doesn't touch these details of function
> declarations, as tun.c and route.c is fairly extreme in variations here.
> So we let that pass on the reformatting patch before the v2.4 release,
> to take care of them manually, as we didn't spend much extra time
> looking at more tweaks for uncrustify to make the result readable. But
> I'm not sure we documented our preferences on function declarations, I
> don't recall that now.
I couldn't find anything about it in [1]
>
> Even though we are not united in the use of uncrustify after the
> reformatting patches we did in December, I think it makes sense to at
> least double check what uncrustify would change and consider those. The
> lesser the gap is to that result, the easier it will be to have a
> consistent coding style over the complete code base.
Do you think it might be reasonable to document all the rules in the
CodingStyle wikipage?
Re: [Openvpn-devel] [PATCH] route: cleanup codestyle and make code more readable
Hi, On 06-09-17 22:45, David Sommerseth wrote: > On 23/08/17 07:30, Antonio Quartulli wrote: >> -static void delete_route(struct route_ipv4 *r, const struct tuntap *tt, >> unsigned int flags, const struct route_gateway_info *rgi, const struct >> env_set *es); >> +static void delete_route(struct route_ipv4 *r, const struct tuntap *tt, >> + unsigned int flags, >> + const struct route_gateway_info *rgi, >> + const struct env_set *es); > > vs > >> static void >> -delete_route(struct route_ipv4 *r, >> - const struct tuntap *tt, >> - unsigned int flags, >> - const struct route_gateway_info *rgi, >> - const struct env_set *es) >> +delete_route(struct route_ipv4 *r, const struct tuntap *tt, unsigned int >> flags, >> + const struct route_gateway_info *rgi, const struct env_set *es) > > I think the change you do in the former one is also more readable than > squeezing everything into as few lines as possible, especially when > there's lots of arguments. I disagree that stretching out function prototypes/declarations over multiple lines improves readability. Adding newlines wastes my vertical screen real estate, which results in more scrolling and reduced overview. Or: I fully agree with the proposed change by Antonio (modulo the tabs, of course). -Steffan 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
