[Openvpn-devel,4/7] Fix a number of mingw warnings

Message ID 20210421134348.1950392-4-arne@rfc2549.org
State Accepted
Headers show
Series
  • [Openvpn-devel,1/7] Remove --disable-multihome option
Related show

Commit Message

Arne Schwabe April 21, 2021, 1:43 p.m.
Move to definition inside the ifdef where they are used to avoid
unused warnings.

Fix a few printf related warnings when DWORD is used as paramter and
the printf format should be %lu (long unsigned int)

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/route.c  | 4 ++--
 src/openvpn/socket.c | 3 ++-
 src/openvpn/tun.c    | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

Comments

Antonio Quartulli April 22, 2021, 12:21 a.m. | #1
Hi,

On 21/04/2021 15:43, Arne Schwabe wrote:
> Move to definition inside the ifdef where they are used to avoid
> unused warnings.
> 
> Fix a few printf related warnings when DWORD is used as paramter and
> the printf format should be %lu (long unsigned int)
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  src/openvpn/route.c  | 4 ++--
>  src/openvpn/socket.c | 3 ++-
>  src/openvpn/tun.c    | 2 +-
>  3 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/src/openvpn/route.c b/src/openvpn/route.c
> index c6b3dc584..c83bd9e2b 100644
> --- a/src/openvpn/route.c
> +++ b/src/openvpn/route.c
> @@ -2360,7 +2360,6 @@ delete_route_ipv6(const struct route_ipv6 *r6, const struct tuntap *tt,
>  #else
>      int metric;
>  #endif
> -    const char *device = tt->actual_name;
>      bool gateway_needed = false;
>  
>      if ((r6->flags & (RT_DEFINED|RT_ADDED)) != (RT_DEFINED|RT_ADDED))
> @@ -2369,6 +2368,7 @@ delete_route_ipv6(const struct route_ipv6 *r6, const struct tuntap *tt,
>      }
>  
>  #ifndef _WIN32
> +    const char *device = tt->actual_name;

This variable is not used in all cases embraced by "ifndef _WIN32".
I.e. I think it is not used when any of the following is defined:
TARGET_OPENBSD
TARGET_NETBSD
TARGET_AIX

Therefore this change would fix *only* the warning on Windows.
Do we want to go this way? or should we rather rearrange the ifdefs a
bit so that we have a cleaner change/function?



>      if (r6->iface != NULL)              /* vpn server special route */
>      {
>          device = r6->iface;
> @@ -2713,7 +2713,7 @@ get_default_gateway_row(const MIB_IPFORWARDTABLE *routes)
>              const DWORD index = row->dwForwardIfIndex;
>              const DWORD metric = row->dwForwardMetric1;
>  
> -            dmsg(D_ROUTE_DEBUG, "GDGR: route[%d] %s/%s i=%d m=%d",
> +            dmsg(D_ROUTE_DEBUG, "GDGR: route[%lu] %s/%s i=%d m=%d",

shouldn't be enough to convert to %u ? (/me is not a DOWRD expert though)


>                   i,
>                   print_in_addr_t((in_addr_t) net, 0, &gc),
>                   print_in_addr_t((in_addr_t) mask, 0, &gc),
> diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
> index b13d2e0f1..8a6e42cc6 100644
> --- a/src/openvpn/socket.c
> +++ b/src/openvpn/socket.c
> @@ -2837,10 +2837,11 @@ print_link_socket_actual_ex(const struct link_socket_actual *act,
>  {
>      if (act)
>      {
> -        char ifname[IF_NAMESIZE] = "[undef]";
>          struct buffer out = alloc_buf_gc(128, gc);
>          buf_printf(&out, "%s", print_sockaddr_ex(&act->dest.addr.sa, separator, flags, gc));
>  #if ENABLE_IP_PKTINFO
> +        char ifname[IF_NAMESIZE] = "[undef]";

This makes sense.

> +
>          if ((flags & PS_SHOW_PKTINFO) && addr_defined_ipi(act))
>          {
>              switch (act->dest.addr.sa.sa_family)
> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> index 2c1b270dd..4ef34e4eb 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -5606,7 +5606,7 @@ windows_set_mtu(const int iface_index, const short family,
>  
>      if (err != NO_ERROR)
>      {
> -        msg(M_WARN, "TUN: Setting %s mtu failed: %s [status=%u if_index=%d]",
> +        msg(M_WARN, "TUN: Setting %s mtu failed: %s [status=%lu if_index=%d]",

same question as above.

>              family_name, strerror_win32(err, &gc), err, iface_index);
>      }
>      else
> 


Regards,
Arne Schwabe April 22, 2021, 12:39 p.m. | #2
>>  #ifndef _WIN32
>> +    const char *device = tt->actual_name;
> 
> This variable is not used in all cases embraced by "ifndef _WIN32".
> I.e. I think it is not used when any of the following is defined:
> TARGET_OPENBSD
> TARGET_NETBSD
> TARGET_AIX
> 
> Therefore this change would fix *only* the warning on Windows.
> Do we want to go this way? or should we rather rearrange the ifdefs a
> bit so that we have a cleaner change/function?


No all other platforms. The variable is used just 2 lines below it
definition to set gateway_needed which is used by all platforms apart
from _WIN32. So only WIN32 had this warning.

> 
> 
> 
>>      if (r6->iface != NULL)              /* vpn server special route */
>>      {
>>          device = r6->iface;
>> @@ -2713,7 +2713,7 @@ get_default_gateway_row(const MIB_IPFORWARDTABLE *routes)
>>              const DWORD index = row->dwForwardIfIndex;
>>              const DWORD metric = row->dwForwardMetric1;
>>  
>> -            dmsg(D_ROUTE_DEBUG, "GDGR: route[%d] %s/%s i=%d m=%d",
>> +            dmsg(D_ROUTE_DEBUG, "GDGR: route[%lu] %s/%s i=%d m=%d",
> 
> shouldn't be enough to convert to %u ? (/me is not a DOWRD expert though)

No. DWORD is a long unsigned int as mentioned in the commit message. It
is 32 bit int since Windows always has long as 32 bit. It is a bit
confusing when you are used to the long=64 bit :)

>> index 2c1b270dd..4ef34e4eb 100644
>> --- a/src/openvpn/tun.c
>> +++ b/src/openvpn/tun.c
>> @@ -5606,7 +5606,7 @@ windows_set_mtu(const int iface_index, const short family,
>>  
>>      if (err != NO_ERROR)
>>      {
>> -        msg(M_WARN, "TUN: Setting %s mtu failed: %s [status=%u if_index=%d]",
>> +        msg(M_WARN, "TUN: Setting %s mtu failed: %s [status=%lu if_index=%d]",
> 
> same question as above.
> 

Same answer :)

Arne
Antonio Quartulli April 22, 2021, 2:10 p.m. | #3
Hi,

On 22/04/2021 14:39, Arne Schwabe wrote:
> 
>>>  #ifndef _WIN32
>>> +    const char *device = tt->actual_name;
>>
>> This variable is not used in all cases embraced by "ifndef _WIN32".
>> I.e. I think it is not used when any of the following is defined:
>> TARGET_OPENBSD
>> TARGET_NETBSD
>> TARGET_AIX
>>
>> Therefore this change would fix *only* the warning on Windows.
>> Do we want to go this way? or should we rather rearrange the ifdefs a
>> bit so that we have a cleaner change/function?
> 
> 
> No all other platforms. The variable is used just 2 lines below it
> definition to set gateway_needed which is used by all platforms apart
> from _WIN32. So only WIN32 had this warning.

Well, it's assigned but not used on the platforms I mentioned.
I guess the compiler isn't able to throw a warning at the moment though.

Anyway, I am fine with keeping this change as it is. This code may
require some bigger restructuring at some point.
So fixing the usage of one variable only doesn't give us anything.

But at least this changes gets rid of a warning :)

> 
>>
>>
>>
>>>      if (r6->iface != NULL)              /* vpn server special route */
>>>      {
>>>          device = r6->iface;
>>> @@ -2713,7 +2713,7 @@ get_default_gateway_row(const MIB_IPFORWARDTABLE *routes)
>>>              const DWORD index = row->dwForwardIfIndex;
>>>              const DWORD metric = row->dwForwardMetric1;
>>>  
>>> -            dmsg(D_ROUTE_DEBUG, "GDGR: route[%d] %s/%s i=%d m=%d",
>>> +            dmsg(D_ROUTE_DEBUG, "GDGR: route[%lu] %s/%s i=%d m=%d",
>>
>> shouldn't be enough to convert to %u ? (/me is not a DOWRD expert though)
> 
> No. DWORD is a long unsigned int as mentioned in the commit message. It
> is 32 bit int since Windows always has long as 32 bit. It is a bit
> confusing when you are used to the long=64 bit :)
> 
>>> index 2c1b270dd..4ef34e4eb 100644
>>> --- a/src/openvpn/tun.c
>>> +++ b/src/openvpn/tun.c
>>> @@ -5606,7 +5606,7 @@ windows_set_mtu(const int iface_index, const short family,
>>>  
>>>      if (err != NO_ERROR)
>>>      {
>>> -        msg(M_WARN, "TUN: Setting %s mtu failed: %s [status=%u if_index=%d]",
>>> +        msg(M_WARN, "TUN: Setting %s mtu failed: %s [status=%lu if_index=%d]",
>>
>> same question as above.
>>

Thanks for the explanation on DWORD.

Compiled on my rig and no problem showed up.

Acked-by: Antonio Quartulli <antonio@openvpn.net>
Gert Doering April 22, 2021, 2:34 p.m. | #4
Your patch has been applied to the master branch.

commit 7890e51aab91b304045a0163462f334b8e5ae8e8
Author: Arne Schwabe
Date:   Wed Apr 21 15:43:45 2021 +0200

     Fix a number of mingw warnings

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20210421134348.1950392-4-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22176.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index c6b3dc584..c83bd9e2b 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -2360,7 +2360,6 @@  delete_route_ipv6(const struct route_ipv6 *r6, const struct tuntap *tt,
 #else
     int metric;
 #endif
-    const char *device = tt->actual_name;
     bool gateway_needed = false;
 
     if ((r6->flags & (RT_DEFINED|RT_ADDED)) != (RT_DEFINED|RT_ADDED))
@@ -2369,6 +2368,7 @@  delete_route_ipv6(const struct route_ipv6 *r6, const struct tuntap *tt,
     }
 
 #ifndef _WIN32
+    const char *device = tt->actual_name;
     if (r6->iface != NULL)              /* vpn server special route */
     {
         device = r6->iface;
@@ -2713,7 +2713,7 @@  get_default_gateway_row(const MIB_IPFORWARDTABLE *routes)
             const DWORD index = row->dwForwardIfIndex;
             const DWORD metric = row->dwForwardMetric1;
 
-            dmsg(D_ROUTE_DEBUG, "GDGR: route[%d] %s/%s i=%d m=%d",
+            dmsg(D_ROUTE_DEBUG, "GDGR: route[%lu] %s/%s i=%d m=%d",
                  i,
                  print_in_addr_t((in_addr_t) net, 0, &gc),
                  print_in_addr_t((in_addr_t) mask, 0, &gc),
diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index b13d2e0f1..8a6e42cc6 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -2837,10 +2837,11 @@  print_link_socket_actual_ex(const struct link_socket_actual *act,
 {
     if (act)
     {
-        char ifname[IF_NAMESIZE] = "[undef]";
         struct buffer out = alloc_buf_gc(128, gc);
         buf_printf(&out, "%s", print_sockaddr_ex(&act->dest.addr.sa, separator, flags, gc));
 #if ENABLE_IP_PKTINFO
+        char ifname[IF_NAMESIZE] = "[undef]";
+
         if ((flags & PS_SHOW_PKTINFO) && addr_defined_ipi(act))
         {
             switch (act->dest.addr.sa.sa_family)
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 2c1b270dd..4ef34e4eb 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -5606,7 +5606,7 @@  windows_set_mtu(const int iface_index, const short family,
 
     if (err != NO_ERROR)
     {
-        msg(M_WARN, "TUN: Setting %s mtu failed: %s [status=%u if_index=%d]",
+        msg(M_WARN, "TUN: Setting %s mtu failed: %s [status=%lu if_index=%d]",
             family_name, strerror_win32(err, &gc), err, iface_index);
     }
     else