[Openvpn-devel,4/4] Simplified if statements for better readability

Message ID 20190401112140.13212-4-cschenk@mail.uni-paderborn.de
State Superseded
Headers show
Series
  • [Openvpn-devel,1/4] Setting adapter mtu on windows systems
Related show

Commit Message

Christopher Schenk April 1, 2019, 11:21 a.m.
---
 src/openvpn/tun.c | 58 ++++++++++++-----------------------------------
 1 file changed, 14 insertions(+), 44 deletions(-)

Comments

Selva Nair April 1, 2019, 3:59 p.m. | #1
Hi,

On Mon, Apr 1, 2019 at 7:22 AM Christopher Schenk <
cschenk@mail.uni-paderborn.de> wrote:

> ---
>  src/openvpn/tun.c | 58 ++++++++++++-----------------------------------
>  1 file changed, 14 insertions(+), 44 deletions(-)
>

@@ -213,7 +213,7 @@ do_set_mtu_service(const struct tuntap *tt, const short
> family, const int mtu)
>         ack_message_t ack;
>         struct gc_arena gc = gc_new();
>         HANDLE pipe = tt->options.msg_channel;
> -
> +       char *family_name = (family == AF_INET6) ? "IPv6" : "IPv4";


This should be preferably declared as const char *family_name. Also see my
response to commit 2

Some general remarks follow:

Now we have 4 commits, two for the initial netsh implementation,
one replacing that by SetIpInterfaceEntry() and a fixup. We don't want to
keep all that irrelevant history, so these have to be squashed into one
commit
(or two if you wish to separate the interactive service part).
And edit the final commit message header and description to reflect the
final
version. Squashing all into one commit may be easier and that's fine in
this case.

We also prefer to have patches signed off. Adding -s to "git commit" or
doing "git commit -s --amend" will take care of that.

Finally, I'm not sure whether we still require patch authors to follow the
code formatting style or we automatically run uncrustify during merge.
Even if its not mandatory, its a good practice to follow the style in the
rest of
the code to the extent possible: for example, we use 4 spaces for indent
(not 8), no tabs anywhere etc. You can run
"uncrustify -c dev-tools/uncrustify.conf" on tun.c, interactive.c
and openvpn_msg.h to get most of that rectified. Note that uncrustify may
edit unrelated parts of the code -- if it does, do not include
such changes in the patch.

Thanks

Selva
<div dir="ltr"><div dir="ltr"> Hi,<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Apr 1, 2019 at 7:22 AM Christopher Schenk &lt;<a href="mailto:cschenk@mail.uni-paderborn.de" target="_blank">cschenk@mail.uni-paderborn.de</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">---<br>
 src/openvpn/tun.c | 58 ++++++++++++-----------------------------------<br>
 1 file changed, 14 insertions(+), 44 deletions(-)<br></blockquote><div><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">@@ -213,7 +213,7 @@ do_set_mtu_service(const struct tuntap *tt, const short family, const int mtu)<br>
        ack_message_t ack;<br>
        struct gc_arena gc = gc_new();<br>
        HANDLE pipe = tt-&gt;options.msg_channel;<br>
-<br>
+       char *family_name = (family == AF_INET6) ? &quot;IPv6&quot; : &quot;IPv4&quot;;</blockquote></div><div><br>This should be preferably declared as const char *family_name. Also see my<br>response to commit 2<br></div><div><br></div><div>Some general remarks follow:<br><br></div><div>Now we have 4 commits, two for the initial netsh implementation,<br>one replacing that by SetIpInterfaceEntry() and a fixup. We don&#39;t want to<br>keep all that irrelevant history, so these have to be squashed into one commit <br>(or two if you wish to separate the interactive service part). <br>And edit the final commit message header and description to reflect the final<br>version. Squashing all into one commit may be easier and that&#39;s fine in<br>this case.<br></div><div><br></div><div>We also prefer to have patches signed off. Adding -s to &quot;git commit&quot; or<br>doing &quot;git commit -s --amend&quot; will take care of that.<br><br></div><div>Finally, I&#39;m not sure whether we still require patch authors to follow the<br>code formatting style or we automatically run uncrustify during merge.<br>Even if its not mandatory, its a good practice to follow the style in the rest of<br>the code to the extent possible: for example, we use 4 spaces for indent<br>(not 8), no tabs anywhere etc. You can run <br>&quot;uncrustify -c dev-tools/uncrustify.conf&quot; on tun.c, interactive.c<br>and openvpn_msg.h to get most of that rectified. Note that uncrustify may<br>edit unrelated parts of the code -- if it does, do not include<br>such changes in the patch.<br></div><div><br></div><div>Thanks<br><br></div><div>Selva</div></div></div>
Gert Doering April 1, 2019, 8:40 p.m. | #2
Hi,

On Mon, Apr 01, 2019 at 11:59:04AM -0400, Selva Nair wrote:
> Finally, I'm not sure whether we still require patch authors to follow the
> code formatting style or we automatically run uncrustify during merge.

We have decided that all *core* authors are required to either run
uncrustify or "get it right on their own" - but Christopher is not
"core", so I could do that for him.

OTOH, I prefer to commit patches "as they come in on the list", so,
no code changes by me, not even formatting...

@Christopher: the source tree has "openvpn/dev-tools/uncrustify.conf".

If you copy that (on a unixish system) to $HOME/.uncrustify.cfg,
you can just call "uncrustify --replace $sourcefile" to have it change 
the file to our accepted coding standard.  Much easier than hanggling
on the list later on.

gert

Patch

diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 9fe8444f..20e661a7 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -69,7 +69,7 @@  static void netsh_ifconfig(const struct tuntap_options *to,
                            const in_addr_t netmask,
                            const unsigned int flags);
 
-static void windows_set_mtu(const int iface_indey,
+static void windows_set_mtu(const int iface_index,
 								short family,
                                const int mtu);
 
@@ -213,7 +213,7 @@  do_set_mtu_service(const struct tuntap *tt, const short family, const int mtu)
 	ack_message_t ack;
 	struct gc_arena gc = gc_new();
 	HANDLE pipe = tt->options.msg_channel;
-
+	char *family_name = (family == AF_INET6) ? "IPv6" : "IPv4";
 	set_mtu_message_t mtu_msg = {
 		.header = {
 			msg_set_mtu,
@@ -230,31 +230,15 @@  do_set_mtu_service(const struct tuntap *tt, const short family, const int mtu)
 		goto out;
 	}
 
-	if (family == AF_INET)
+	if (ack.error_number != NO_ERROR)
 	{
-		if (ack.error_number != NO_ERROR)
-		{
-			msg(M_NONFATAL, "TUN: setting IPv4 mtu using service failed: %s [status=%u if_index=%d]",
-				strerror_win32(ack.error_number, &gc), ack.error_number, mtu_msg.iface.index);
-		}
-		else
-		{
-			msg(M_INFO, "IPv4 MTU set to %d on interface %d using service", mtu, mtu_msg.iface.index);
-			ret = true;
-		}
+		msg(M_NONFATAL, "TUN: setting %s mtu using service failed: %s [status=%u if_index=%d]",
+			family_name, strerror_win32(ack.error_number, &gc), ack.error_number, mtu_msg.iface.index);
 	}
-	else if (family == AF_INET6)
+	else
 	{
-		if (ack.error_number != NO_ERROR)
-		{
-			msg(M_NONFATAL, "TUN: setting IPv6 mtu using service failed: %s [status=%u if_index=%d]",
-				strerror_win32(ack.error_number, &gc), ack.error_number, mtu_msg.iface.index);
-		}
-		else
-		{
-			msg(M_INFO, "IPv6 MTU set to %d on interface %d using service", mtu, mtu_msg.iface.index);
-			ret = true;
-		}
+		msg(M_INFO, "%s MTU set to %d on interface %d using service", family_name, mtu, mtu_msg.iface.index);
+		ret = true;
 	}
 
 out:
@@ -5313,35 +5297,21 @@  windows_set_mtu(const int iface_index, const short family,
 	DWORD err = 0;
 	struct gc_arena gc = gc_new();
 	MIB_IPINTERFACE_ROW row;
+	char *family_name = (family == AF_INET6) ? "IPv6" : "IPv4";
 	InitializeIpInterfaceEntry(&row);
 	row.Family = family;
 	row.InterfaceIndex = iface_index;
 	row.NlMtu = mtu;
 
 	err = SetIpInterfaceEntry(&row);
-	if (family == AF_INET)
+	if (err != NO_ERROR)
 	{
-		if (err != NO_ERROR)
-		{
-			msg(M_WARN, "TUN: Setting IPv4 mtu failed: %s [status=%u if_index=%d]",
-				strerror_win32(err, &gc), err, iface_index);
-		}
-		else
-		{
-			msg(M_INFO, "Successfully set IPv4 mtu on interface %d", iface_index);
-		}
+		msg(M_WARN, "TUN: Setting %s mtu failed: %s [status=%u if_index=%d]",
+			family_name, strerror_win32(err, &gc), err, iface_index);
 	}
-	else if (family == AF_INET6)
+	else
 	{
-		if (err != NO_ERROR)
-		{
-			msg(M_WARN, "TUN: Setting IPv6 mtu failed: %s [status=%u if_index=%d]",
-				strerror_win32(err, &gc), err, iface_index);
-		}
-		else
-		{
-			msg(M_INFO, "Successfully set IPv6 mtu on interface %d", iface_index);
-		}
+		msg(M_INFO, "Successfully set %s mtu on interface %d", family_name, iface_index);
 	}
 }