Re: [Openvpn-devel] [PATCH v2] msvc: fix various level2 warnings

2020-06-25 Thread Lev Stipakov 
Hi,

> > -qsort(cipher_list, num_ciphers, sizeof(*cipher_list), cipher_name_cmp);
> > +qsort((void *)cipher_list, num_ciphers, sizeof(*cipher_list), 
> > cipher_name_cmp);
>
> I find this one questionable.  Qsort wants a void *, but every pointer type
> should be convertible to void *, without a warning?

Yes, but this is about "const". VS shows "'function': different
'const' qualifiers".
I'll go with this one:

/* cast to non-const to prevent warning */
qsort((EVP_CIPHER *)cipher_list, num_ciphers,
sizeof(*cipher_list), cipher_name_cmp);

> > -msg(D_ROUTE, "interactive service msg_channel=%u", (unsigned int) 
> > c->options.msg_channel);
> > +msg(D_ROUTE, "interactive service msg_channel=%u", (unsigned long 
> > long) c->options.msg_channel);
>
> This gets a NAK.  "%u" is "unsigned int", not (never!) (unsigned long long) -

Good point, thanks. The warning was about casting from HANDLE to
unsigned int. I'll change
format specifier to "%llu".

> > -(sd, IPPROTO_IP, IP_MTU_DISCOVER, _type, 
> > sizeof(mtu_type)))
> > +(sd, IPPROTO_IP, IP_MTU_DISCOVER, (const char 
> > *)_type, sizeof(mtu_type)))
>
> I find this questionable as well.  Setsockopt() on Unix wants a
> "void *", not a "const char *" - and this cast suggest to the reader
> that we're dealing with "something character based", which it is not.

That's the case in Windows:

https://docs.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-setsockopt

> int setsockopt( SOCKET s, int level, int optname, const char *optval, int 
> optlen );

> >  show_available_tls_ciphers_list(const char *cipher_list,
> >  const char *tls_cert_profile,
> > -bool tls13);
> > +const bool tls13);
>
> I'm not sure why this is needed or beneficial?

Otherwise there is warning "formal parameter 3 different from declaration".

I noticed that mbedtls implementation doesn't have "const", so I'll
just remove it from openssl implementation and won't change header.

> >  void
> > -delete_route_connected_v6_net(struct tuntap *tt,
> > +delete_route_connected_v6_net(const struct tuntap *tt,
> >const struct env_set *es)
>
> I might be convinced that this is a useful change, but if it's needed(!)
> for delete_route_connected_v6_net(), the "cost" should be added to
> add_route_connected_v6_net() as well.

I looked closer and in fact second argument of delete_route_connected_v6_net()
is always NULL, so I'll remove that parameter.


-- 
-Lev


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

Re: [Openvpn-devel] [PATCH v2] msvc: fix various level2 warnings

2020-06-25 Thread Arne Schwabe 


>> @@ -1812,7 +1812,7 @@ do_open_tun(struct context *c)
>>  #ifdef _WIN32
>>  /* store (hide) interactive service handle in tuntap_options */
>>  c->c1.tuntap->options.msg_channel = c->options.msg_channel;
>> -msg(D_ROUTE, "interactive service msg_channel=%u", (unsigned int) 
>> c->options.msg_channel);
>> +msg(D_ROUTE, "interactive service msg_channel=%u", (unsigned long long) 
>> c->options.msg_channel);
> 
> This gets a NAK.  "%u" is "unsigned int", not (never!) (unsigned long long) -
> if you want that, because the data type is so big, use %llu or %I64u
> (Windows special, according to common.h).

Or use the standardised ones. E.g. PRIu64 for uint64_t. We already use
that auth-token.c.

See also https://en.cppreference.com/w/cpp/header/cinttypes (or another
reference).


> 
>> index 04868cd6..8178ff06 100644
>> --- a/src/openvpn/mtu.c
>> +++ b/src/openvpn/mtu.c
>> @@ -175,7 +175,7 @@ set_mtu_discover_type(int sd, int mtu_type, sa_family_t 
>> proto_af)
>>  #if defined(HAVE_SETSOCKOPT) && defined(IP_MTU_DISCOVER)
>>  case AF_INET:
>>  if (setsockopt
>> -(sd, IPPROTO_IP, IP_MTU_DISCOVER, _type, 
>> sizeof(mtu_type)))
>> +(sd, IPPROTO_IP, IP_MTU_DISCOVER, (const char 
>> *)_type, sizeof(mtu_type)))
> 
> I find this questionable as well.  Setsockopt() on Unix wants a 
> "void *", not a "const char *" - and this cast suggest to the reader
> that we're dealing with "something character based", which it is not.
> 
>>  #if defined(HAVE_SETSOCKOPT) && defined(IPV6_MTU_DISCOVER)
>>  case AF_INET6:
>>  if (setsockopt
>> -(sd, IPPROTO_IPV6, IPV6_MTU_DISCOVER, _type, 
>> sizeof(mtu_type)))
>> +(sd, IPPROTO_IPV6, IPV6_MTU_DISCOVER, (const char 
>> *)_type, sizeof(mtu_type)))
> 
> Same here.
> 
>>  show_available_tls_ciphers_list(const char *cipher_list,
>>  const char *tls_cert_profile,
>> -bool tls13);
>> +const bool tls13);
> 
> I'm not sure why this is needed or beneficial?  This const stuff is
> good for pointers ("do not modify the pointed-to area") but for
> call-by-value arguments, the compiler can figure this out itself.

Clang tidy actually warns against this for integral types saying that
they are passed by value always in C++. Not sure if this also applies to C.

Arne



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

Re: [Openvpn-devel] [PATCH v2] msvc: fix various level2 warnings

2020-06-25 Thread Gert Doering 
Hi,

On Thu, Jun 25, 2020 at 11:37:45AM +0300, Lev Stipakov wrote:
> @@ -315,7 +315,7 @@ show_available_ciphers(void)
>  }
>  }
>  
> -qsort(cipher_list, num_ciphers, sizeof(*cipher_list), cipher_name_cmp);
> +qsort((void *)cipher_list, num_ciphers, sizeof(*cipher_list), 
> cipher_name_cmp);

I find this one questionable.  Qsort wants a void *, but every pointer type
should be convertible to void *, without a warning?

(Adding casts for things that are explicitly always allowed and well-defined 
is something I do not like very much - it just makes "more code to read")


> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index 2c8db68d..107bb4c9 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -1812,7 +1812,7 @@ do_open_tun(struct context *c)
>  #ifdef _WIN32
>  /* store (hide) interactive service handle in tuntap_options */
>  c->c1.tuntap->options.msg_channel = c->options.msg_channel;
> -msg(D_ROUTE, "interactive service msg_channel=%u", (unsigned int) 
> c->options.msg_channel);
> +msg(D_ROUTE, "interactive service msg_channel=%u", (unsigned long long) 
> c->options.msg_channel);

This gets a NAK.  "%u" is "unsigned int", not (never!) (unsigned long long) -
if you want that, because the data type is so big, use %llu or %I64u
(Windows special, according to common.h).


> index 04868cd6..8178ff06 100644
> --- a/src/openvpn/mtu.c
> +++ b/src/openvpn/mtu.c
> @@ -175,7 +175,7 @@ set_mtu_discover_type(int sd, int mtu_type, sa_family_t 
> proto_af)
>  #if defined(HAVE_SETSOCKOPT) && defined(IP_MTU_DISCOVER)
>  case AF_INET:
>  if (setsockopt
> -(sd, IPPROTO_IP, IP_MTU_DISCOVER, _type, 
> sizeof(mtu_type)))
> +(sd, IPPROTO_IP, IP_MTU_DISCOVER, (const char 
> *)_type, sizeof(mtu_type)))

I find this questionable as well.  Setsockopt() on Unix wants a 
"void *", not a "const char *" - and this cast suggest to the reader
that we're dealing with "something character based", which it is not.

>  #if defined(HAVE_SETSOCKOPT) && defined(IPV6_MTU_DISCOVER)
>  case AF_INET6:
>  if (setsockopt
> -(sd, IPPROTO_IPV6, IPV6_MTU_DISCOVER, _type, 
> sizeof(mtu_type)))
> +(sd, IPPROTO_IPV6, IPV6_MTU_DISCOVER, (const char 
> *)_type, sizeof(mtu_type)))

Same here.

>  show_available_tls_ciphers_list(const char *cipher_list,
>  const char *tls_cert_profile,
> -bool tls13);
> +const bool tls13);

I'm not sure why this is needed or beneficial?  This const stuff is
good for pointers ("do not modify the pointed-to area") but for
call-by-value arguments, the compiler can figure this out itself.

> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -233,10 +233,11 @@ do_set_mtu_service(const struct tuntap *tt, const short 
> family, const int mtu)
>  sizeof(set_mtu_message_t),
>  0
>  },
> -.iface = {.index = tt->adapter_index,.name = tt->actual_name },
> +.iface = {.index = tt->adapter_index},

Sure of that?  As in "are you sure the service does not need the name,
never"?

>  .mtu = mtu,
>  .family = family
>  };
> +strncpynt(mtu_msg.iface.name, tt->actual_name, 
> sizeof(mtu_msg.iface.name));

Well spotted.  This is interesting, the assignment ".name = tt->actual_name"
shouldn't be legal (as it's basically a hidden strcpy(), not a integral
type or structure assignment), but mingw is not complaining and the 
resulting binary works.  But this definitely is a necessary bugfix.


>  if (!send_msg_iservice(pipe, _msg, sizeof(mtu_msg), , "Set_mtu"))
>  {
> @@ -889,7 +890,7 @@ add_route_connected_v6_net(struct tuntap *tt,
>  }
>  
>  void
> -delete_route_connected_v6_net(struct tuntap *tt,
> +delete_route_connected_v6_net(const struct tuntap *tt,
>const struct env_set *es)

I might be convinced that this is a useful change, but if it's needed(!)
for delete_route_connected_v6_net(), the "cost" should be added to 
add_route_connected_v6_net() as well.

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

[Openvpn-devel] [PATCH v2] msvc: fix various level2 warnings

2020-06-25 Thread Lev Stipakov 
From: Lev Stipakov 

Also set warnings level to level2 and
enable "treat warnings as errors" flag.

Signed-off-by: Lev Stipakov 
---
 src/compat/Debug.props   |  1 -
 src/openvpn/block_dns.c  |  3 ---
 src/openvpn/crypto_openssl.c |  2 +-
 src/openvpn/init.c   |  2 +-
 src/openvpn/mtu.c|  4 ++--
 src/openvpn/openvpn.vcxproj  | 26 +-
 src/openvpn/options.c|  2 +-
 src/openvpn/route.c  |  2 +-
 src/openvpn/ssl_backend.h|  2 +-
 src/openvpn/tun.c|  5 +++--
 10 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/src/compat/Debug.props b/src/compat/Debug.props
index e5e9f681..31bb9d91 100644
--- a/src/compat/Debug.props
+++ b/src/compat/Debug.props
@@ -15,7 +15,6 @@
   
_DEBUG;%(PreprocessorDefinitions)
   MultiThreadedDebugDLL
   EditAndContinue
-  true
 
   
   
diff --git a/src/openvpn/block_dns.c b/src/openvpn/block_dns.c
index 889d6bb9..f4718fc2 100644
--- a/src/openvpn/block_dns.c
+++ b/src/openvpn/block_dns.c
@@ -109,9 +109,6 @@ DEFINE_GUID(
 
 static WCHAR *FIREWALL_NAME = L"OpenVPN";
 
-VOID NETIOAPI_API_
-InitializeIpInterfaceEntry(PMIB_IPINTERFACE_ROW Row);
-
 /*
  * Default msg handler does nothing
  */
diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index 2027d8dc..8805e25e 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -315,7 +315,7 @@ show_available_ciphers(void)
 }
 }
 
-qsort(cipher_list, num_ciphers, sizeof(*cipher_list), cipher_name_cmp);
+qsort((void *)cipher_list, num_ciphers, sizeof(*cipher_list), 
cipher_name_cmp);
 
 for (i = 0; i < num_ciphers; i++)
 {
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 2c8db68d..107bb4c9 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1812,7 +1812,7 @@ do_open_tun(struct context *c)
 #ifdef _WIN32
 /* store (hide) interactive service handle in tuntap_options */
 c->c1.tuntap->options.msg_channel = c->options.msg_channel;
-msg(D_ROUTE, "interactive service msg_channel=%u", (unsigned int) 
c->options.msg_channel);
+msg(D_ROUTE, "interactive service msg_channel=%u", (unsigned long long) 
c->options.msg_channel);
 #endif
 
 /* allocate route list structure */
diff --git a/src/openvpn/mtu.c b/src/openvpn/mtu.c
index 04868cd6..8178ff06 100644
--- a/src/openvpn/mtu.c
+++ b/src/openvpn/mtu.c
@@ -175,7 +175,7 @@ set_mtu_discover_type(int sd, int mtu_type, sa_family_t 
proto_af)
 #if defined(HAVE_SETSOCKOPT) && defined(IP_MTU_DISCOVER)
 case AF_INET:
 if (setsockopt
-(sd, IPPROTO_IP, IP_MTU_DISCOVER, _type, 
sizeof(mtu_type)))
+(sd, IPPROTO_IP, IP_MTU_DISCOVER, (const char 
*)_type, sizeof(mtu_type)))
 {
 msg(M_ERR, "Error setting IP_MTU_DISCOVER type=%d on 
TCP/UDP socket",
 mtu_type);
@@ -186,7 +186,7 @@ set_mtu_discover_type(int sd, int mtu_type, sa_family_t 
proto_af)
 #if defined(HAVE_SETSOCKOPT) && defined(IPV6_MTU_DISCOVER)
 case AF_INET6:
 if (setsockopt
-(sd, IPPROTO_IPV6, IPV6_MTU_DISCOVER, _type, 
sizeof(mtu_type)))
+(sd, IPPROTO_IPV6, IPV6_MTU_DISCOVER, (const char 
*)_type, sizeof(mtu_type)))
 {
 msg(M_ERR, "Error setting IPV6_MTU_DISCOVER type=%d on 
TCP6/UDP6 socket",
 mtu_type);
diff --git a/src/openvpn/openvpn.vcxproj b/src/openvpn/openvpn.vcxproj
index 53ac5482..c34733ea 100644
--- a/src/openvpn/openvpn.vcxproj
+++ b/src/openvpn/openvpn.vcxproj
@@ -28,23 +28,23 @@
   
 Application
 true
-Unicode
+NotSet
 v142
   
   
 Application
 true
-Unicode
+NotSet
 v142
   
   
 Application
-Unicode
+NotSet
 v142
   
   
 Application
-Unicode
+NotSet
 v142
   
   
@@ -74,7 +74,9 @@
 
   
..\compat;$(TAP_WINDOWS_HOME)/include;$(OPENSSL_HOME)/include;$(LZO_HOME)/include;$(PKCS11H_HOME)/include;%(AdditionalIncludeDirectories)
   
_CONSOLE;%(PreprocessorDefinitions)
-  
UNICODE;%(UndefinePreprocessorDefinitions)
+  
%(UndefinePreprocessorDefinitions)
+  Level2
+  true
 
 
 
@@ -87,7 +89,9 @@
 
   
..\compat;$(TAP_WINDOWS_HOME)/include;$(OPENSSL_HOME)/include;$(LZO_HOME)/include;$(PKCS11H_HOME)/include;%(AdditionalIncludeDirectories)
   
_CONSOLE;%(PreprocessorDefinitions)
-  
UNICODE;%(UndefinePreprocessorDefinitions)
+  
%(UndefinePreprocessorDefinitions)
+  Level2
+  true
 
 
 
@@ -100,7 +104,9 @@
 
   
..\compat;$(TAP_WINDOWS_HOME)/include;$(OPENSSL_HOME)/include;$(LZO_HOME)/include;$(PKCS11H_HOME)/include;%(AdditionalIncludeDirectories)
   
_CONSOLE;%(PreprocessorDefinitions)
-  
UNICODE;%(UndefinePreprocessorDefinitions)
+  
%(UndefinePreprocessorDefinitions)
+  Level2
+