openvpnserv: clarify return values type

PATCH Superseded Lev Stipakov 4 participants 11 messages Oct 3, 2018 – Oct 7, 2018

lore

Share & tools

lore

Lev Stipakov

author

2018-10-03 14:16 UTC #0 lore raw

PATCH 1/1 openvpnserv: clarify return values type

From: Lev Stipakov <[email protected]...>

Functions openvpn_vsntprintf and openvpn_sntprintf return values of type int, but in reality it is always 0 or 1, which is essentially bool.

To make code more clear, change return type to bool. Also use stdbool.h header instead of bool definition macros.

Signed-off-by Lev Stipakov <[email protected]...>

---
 src/openvpnserv/automatic.c |  5 -----
 src/openvpnserv/common.c    | 13 +++++++------
 src/openvpnserv/service.h   |  5 +++--
 3 files changed, 10 insertions(+), 13 deletions(-)

src/openvpnserv/automatic.c +0 -5

index 1f98283..c2982c1 100644
@@ -38,11 +38,6 @@
 #include <stdarg.h>
 #include <process.h>
 
-/* bool definitions */
-#define bool int
-#define true 1
-#define false 0
-
 static SERVICE_STATUS_HANDLE service;
 static SERVICE_STATUS status = { .dwServiceType = SERVICE_WIN32_SHARE_PROCESS };

src/openvpnserv/common.c +7 -6

index dc47666..69dd44b 100644
@@ -31,7 +31,7 @@ LPCTSTR service_instance = TEXT("");
  * These are necessary due to certain buggy implementations of (v)snprintf,
  * that don't guarantee null termination for size > 0.
  */
-int
+bool
 openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list arglist)
 {
     int len = -1;

@@ -42,18 +42,19 @@ openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list arglist)
     }
     return (len >= 0 && len < size);
 }
-int
+
+bool
 openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...)
 {
     va_list arglist;
-    int len = -1;
+    bool res = false;
     if (size > 0)
     {
         va_start(arglist, format);
-        len = openvpn_vsntprintf(str, size, format, arglist);
+        res = openvpn_vsntprintf(str, size, format, arglist);
         va_end(arglist);
     }
-    return len;
+    return res;
 }
 
 static DWORD

@@ -65,7 +66,7 @@ GetRegString(HKEY key, LPCTSTR value, LPTSTR data, DWORD size, LPCTSTR default_v
     if (status == ERROR_FILE_NOT_FOUND && default_value)
     {
         size_t len = size/sizeof(data[0]);
-        if (openvpn_sntprintf(data, len, default_value) > 0)
+        if (openvpn_sntprintf(data, len, default_value))
         {
             status = ERROR_SUCCESS;
         }

src/openvpnserv/service.h +3 -2

index 4d03b88..eaa479a 100644
@@ -32,6 +32,7 @@
 
 #include <winsock2.h>
 #include <windows.h>
+#include <stdbool.h>
 #include <stdlib.h>
 #include <tchar.h>

@@ -82,9 +83,9 @@ VOID WINAPI ServiceStartAutomatic(DWORD argc, LPTSTR *argv);
 VOID WINAPI ServiceStartInteractiveOwn(DWORD argc, LPTSTR *argv);
 VOID WINAPI ServiceStartInteractive(DWORD argc, LPTSTR *argv);
 
-int openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list arglist);
+bool openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list arglist);
 
-int openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...);
+bool openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...);
 
 DWORD GetOpenvpnSettings(settings_t *s);

Signature · 1 line

2.7.4

Selva Nair

2018-10-03 15:08 UTC #1 lore raw

Hi,

Quoted 6 lines · Lev Stipakov

From: Lev Stipakov <[email protected]...>
Functions openvpn_vsntprintf and openvpn_sntprintf return values of type int, but in reality it is always 0 or 1, which is essentially bool.

openvpn_sntprintf could return -1 if size = 0, but this looks like the right approach and matches openvpn_snprintf in the core. The code looks good too.

Just a thought about bool below:

Quoted 4 lines

To make code more clear, change return type to bool. Also use stdbool.h header instead of bool definition macros.

We do use BOOL all over the service code (unavoidable with Windows API), and bool is used in only one or two places. So would it be better to just change this to BOOL/TRUE/FALSE and not include stdbool.h?

Wishlist: openvpn_swprintf() with nul termination guarantee. I try to avoid the TCHAR variety be explicit about wide and narrow characters.

Quoted 99 lines

Signed-off-by Lev Stipakov <[email protected]...>

---
 src/openvpnserv/automatic.c |  5 -----
 src/openvpnserv/common.c    | 13 +++++++------
 src/openvpnserv/service.h   |  5 +++--
 3 files changed, 10 insertions(+), 13 deletions(-)

src/openvpnserv/automatic.c +0 -5

index 1f98283..c2982c1 100644
@@ -38,11 +38,6 @@
 #include <stdarg.h>
 #include <process.h>
 
-/* bool definitions */
-#define bool int
-#define true 1
-#define false 0
-
 static SERVICE_STATUS_HANDLE service;
 static SERVICE_STATUS status = { .dwServiceType =
 SERVICE_WIN32_SHARE_PROCESS };

src/openvpnserv/common.c +1 -1

index dc47666..69dd44b 100644
@@ -31,7 +31,7 @@ LPCTSTR service_instance = TEXT("");
  * These are necessary due to certain buggy implementations of
 (v)snprintf,
  * that don't guarantee null termination for size > 0.
  */
-int
+bool
 openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list
 arglist)

 {
     int len = -1;

src/openvpnserv/common.c +5 -4

@@ -42,18 +42,19 @@ openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR
 format, va_list arglist)
     }
     return (len >= 0 && len < size);
 }
-int
+
+bool
 openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...)
 {
     va_list arglist;
-    int len = -1;
+    bool res = false;
     if (size > 0)
     {
         va_start(arglist, format);
-        len = openvpn_vsntprintf(str, size, format, arglist);
+        res = openvpn_vsntprintf(str, size, format, arglist);
         va_end(arglist);
     }
-    return len;
+    return res;
 }

static DWORD

src/openvpnserv/common.c +0 -0

@@ -65,7 +66,7 @@ GetRegString(HKEY key, LPCTSTR value, LPTSTR data, DWORD

size, LPCTSTR default_v

     if (status == ERROR_FILE_NOT_FOUND && default_value)
     {
         size_t len = size/sizeof(data[0]);

src/openvpnserv/common.c +1 -1

-        if (openvpn_sntprintf(data, len, default_value) > 0)
+        if (openvpn_sntprintf(data, len, default_value))
         {
             status = ERROR_SUCCESS;
         }

src/openvpnserv/service.h +3 -2

index 4d03b88..eaa479a 100644
@@ -32,6 +32,7 @@
 
 #include <winsock2.h>
 #include <windows.h>
+#include <stdbool.h>
 #include <stdlib.h>
 #include <tchar.h>

@@ -82,9 +83,9 @@ VOID WINAPI ServiceStartAutomatic(DWORD argc, LPTSTR
 *argv);
 VOID WINAPI ServiceStartInteractiveOwn(DWORD argc, LPTSTR *argv);
 VOID WINAPI ServiceStartInteractive(DWORD argc, LPTSTR *argv);
 
-int openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list
 arglist);
+bool openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list
 arglist);
 
-int openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...);
+bool openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...);

Quoted 5 lines

DWORD GetOpenvpnSettings(settings_t *s);
--
2.7.4

Thanks,

Selva

David Sommerseth

2018-10-03 15:41 UTC #2 lore raw

Quoted 9 lines · Selva Nair

To make code more clear, change return type to bool. Also use stdbool.h header instead of bool definition macros.
We do use BOOL all over the service code (unavoidable with Windows API), and bool is used in only one or two places. So would it be better to just change this to BOOL/TRUE/FALSE and not include stdbool.h?

Several years ago, we started towards a path to get rid of the BOOL/TRUE/FALSE stuff, in favour of stdbool.h. And when we moved towards -std=c99, this made this move even more reasonable.

But ... are you saying Windows C compilers we use/support does not support stdbool.h? Even with -std=c99?

I would prefer we switch over to using standard types instead of our own "workaround" solutions wherever possible.

Signature · 4 lines

kind regards,

David Sommerseth
OpenVPN Inc

Selva Nair

2018-10-03 16:31 UTC #3 lore raw

Quoted 11 lines · David Sommerseth

Quoted 9 lines · Selva Nair
To make code more clear, change return type to bool. Also use stdbool.h header instead of bool definition macros.
We do use BOOL all over the service code (unavoidable with Windows API), and bool is used in only one or two places. So would it be better to just change this to BOOL/TRUE/FALSE and not include stdbool.h?

Quoted 6 lines

Several years ago, we started towards a path to get rid of the BOOL/TRUE/FALSE
stuff, in favour of stdbool.h. And when we moved towards -std=c99, this made
this move even more reasonable.

Yes, I know that's the path taken in openvpn core sources which is very sensible in
a cross-platform code.

Quoted 4 lines

But ... are you saying Windows C compilers we use/support does not support stdbool.h? Even with -std=c99?

It does, but we cannot avoid using the Windows-specific BOOL (which I believe is a typedef to int pulled in through windows.h) as Windows API is replete with it. In fact stdbool's bool may be internally an int and thus same
as BOOL but we can't assume that.

In case of the service code, which is Windows only, there are about 30 uses of
BOOL but only a few cases of bool (typdef to int) all of which are in the deprecated
automatic service code. This patch changes the latter to use stdbool which is
quite reasonable. But I just wonder whether its just better to use BOOL in this occasion as the latter is not going to go away in Windows.

That said, I prefer stdbool too, and I'm fine with moving towards replacing all BOOL
with bool and casting to BOOL if/when required (say, calling Windows API).

Selva

Lev Stipakov

author

2018-10-03 16:41 UTC #4 lore raw

Hi,

Quoted 6 lines

In case of the service code, which is Windows only, there are about 30 uses of
BOOL but only a few cases of bool (typdef to int) all of which are in the deprecated
automatic service code.

I agree, it probably not worth to introduce a "new" type (stdbool) to interactive
service code.

The point of this patch is to make a few methods return boolean value and BOOL
fits well for this purpose.

Signature · 1 line

-Lev

Lev Stipakov

author

2018-10-03 17:21 UTC #5 lore raw

PATCH v2 1/1 openvpnserv: clarify return values type

From: Lev Stipakov <[email protected]...>

Functions openvpn_vsntprintf and openvpn_sntprintf return values of type int, but in reality it is always 0 or 1 (and -1 for snrptinf), which can be represented as boolean.

To make code clearer, change return type to BOOL. Also
use stdbool.h header instead of bool definition macros in automatic.c.

Signed-off-by Lev Stipakov <[email protected]...>

---
v2: replace bool with BOOL as discussed

 src/openvpnserv/automatic.c |  6 +-----
 src/openvpnserv/common.c    | 13 +++++++------
 src/openvpnserv/service.h   |  4 ++--
 3 files changed, 10 insertions(+), 13 deletions(-)

src/openvpnserv/automatic.c +1 -5

index 1f98283..73d3003 100644
@@ -36,13 +36,9 @@
 
 #include <stdio.h>
 #include <stdarg.h>
+#include <stdbool.h>
 #include <process.h>
 
-/* bool definitions */
-#define bool int
-#define true 1
-#define false 0
-
 static SERVICE_STATUS_HANDLE service;
 static SERVICE_STATUS status = { .dwServiceType = SERVICE_WIN32_SHARE_PROCESS };

src/openvpnserv/common.c +7 -6

index dc47666..6b3a859 100644
@@ -31,7 +31,7 @@ LPCTSTR service_instance = TEXT("");
  * These are necessary due to certain buggy implementations of (v)snprintf,
  * that don't guarantee null termination for size > 0.
  */
-int
+BOOL
 openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list arglist)
 {
     int len = -1;

@@ -42,18 +42,19 @@ openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list arglist)
     }
     return (len >= 0 && len < size);
 }
-int
+
+BOOL
 openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...)
 {
     va_list arglist;
-    int len = -1;
+    BOOL res = FALSE;
     if (size > 0)
     {
         va_start(arglist, format);
-        len = openvpn_vsntprintf(str, size, format, arglist);
+        res = openvpn_vsntprintf(str, size, format, arglist);
         va_end(arglist);
     }
-    return len;
+    return res;
 }
 
 static DWORD

@@ -65,7 +66,7 @@ GetRegString(HKEY key, LPCTSTR value, LPTSTR data, DWORD size, LPCTSTR default_v
     if (status == ERROR_FILE_NOT_FOUND && default_value)
     {
         size_t len = size/sizeof(data[0]);
-        if (openvpn_sntprintf(data, len, default_value) > 0)
+        if (openvpn_sntprintf(data, len, default_value))
         {
             status = ERROR_SUCCESS;
         }

src/openvpnserv/service.h +2 -2

index 4d03b88..b90033a 100644
@@ -82,9 +82,9 @@ VOID WINAPI ServiceStartAutomatic(DWORD argc, LPTSTR *argv);
 VOID WINAPI ServiceStartInteractiveOwn(DWORD argc, LPTSTR *argv);
 VOID WINAPI ServiceStartInteractive(DWORD argc, LPTSTR *argv);
 
-int openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list arglist);
+BOOL openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list arglist);
 
-int openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...);
+BOOL openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...);
 
 DWORD GetOpenvpnSettings(settings_t *s);

Signature · 1 line

2.7.4

Selva Nair

Acked-by on v2 1/1

2018-10-03 20:13 UTC #6 lore raw

Hi,

Quoted 24 lines · Lev Stipakov

From: Lev Stipakov <[email protected]...>
Functions openvpn_vsntprintf and openvpn_sntprintf return values of type int, but in reality it is always 0 or 1 (and -1 for snrptinf), which can be represented as boolean.
To make code clearer, change return type to BOOL. Also
use stdbool.h header instead of bool definition macros in automatic.c
Signed-off-by Lev Stipakov <[email protected]...>
---
v2: replace bool with BOOL as discussed
 src/openvpnserv/automatic.c |  6 +-----
 src/openvpnserv/common.c    | 13 +++++++------
 src/openvpnserv/service.h   |  4 ++--
 3 files changed, 10 insertions(+), 13 deletions(-)
src/openvpnserv/automatic.c +0 -0
index 1f98283..73d3003 100644

src/openvpnserv/automatic.c line 40

Quoted 5 lines

src/openvpnserv/automatic.c +1 -0

@@ -36,13 +36,9 @@
 
 #include <stdio.h>
 #include <stdarg.h>
+#include <stdbool.h>

#include <process.h>

Quoted 77 lines

src/openvpnserv/automatic.c +0 -5

-/* bool definitions */
-#define bool int
-#define true 1
-#define false 0
-
 static SERVICE_STATUS_HANDLE service;
 static SERVICE_STATUS status = { .dwServiceType =
 SERVICE_WIN32_SHARE_PROCESS };

src/openvpnserv/common.c +1 -1

index dc47666..6b3a859 100644
@@ -31,7 +31,7 @@ LPCTSTR service_instance = TEXT("");
  * These are necessary due to certain buggy implementations of
 (v)snprintf,
  * that don't guarantee null termination for size > 0.
  */
-int
+BOOL
 openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list
 arglist)

 {
     int len = -1;

src/openvpnserv/common.c +5 -4

@@ -42,18 +42,19 @@ openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR
 format, va_list arglist)
     }
     return (len >= 0 && len < size);
 }
-int
+
+BOOL
 openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...)
 {
     va_list arglist;
-    int len = -1;
+    BOOL res = FALSE;
     if (size > 0)
     {
         va_start(arglist, format);
-        len = openvpn_vsntprintf(str, size, format, arglist);
+        res = openvpn_vsntprintf(str, size, format, arglist);
         va_end(arglist);
     }
-    return len;
+    return res;
 }

static DWORD

src/openvpnserv/common.c +0 -0

@@ -65,7 +66,7 @@ GetRegString(HKEY key, LPCTSTR value, LPTSTR data, DWORD

size, LPCTSTR default_v

     if (status == ERROR_FILE_NOT_FOUND && default_value)
     {
         size_t len = size/sizeof(data[0]);

src/openvpnserv/common.c +1 -1

-        if (openvpn_sntprintf(data, len, default_value) > 0)
+        if (openvpn_sntprintf(data, len, default_value))
         {
             status = ERROR_SUCCESS;
         }

src/openvpnserv/service.h +2 -2

index 4d03b88..b90033a 100644
@@ -82,9 +82,9 @@ VOID WINAPI ServiceStartAutomatic(DWORD argc, LPTSTR
 *argv);
 VOID WINAPI ServiceStartInteractiveOwn(DWORD argc, LPTSTR *argv);
 VOID WINAPI ServiceStartInteractive(DWORD argc, LPTSTR *argv);
 
-int openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list
 arglist);
+BOOL openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list
 arglist);
 
-int openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...);
+BOOL openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...);

Quoted 3 lines

DWORD GetOpenvpnSettings(settings_t *s);

The originals had confusing return values with a success/fail code hidden in what
looked like a length. The code is cleaner and clearer this way. And, builds/runs fine (mingw cross-build + Windows 7 run time).

Acked-by [email protected]...

Gert Doering

2018-10-07 12:23 UTC #7 lore raw

In reply to Lev Stipakov

Your patch has been applied to the master branch.

(Test built on ubuntu1604, and a quick stare-at-code, but if Selva says it's good, that is good enough :-) )

commit ab9193c32b19e42a1847bd4f6cf967ea0e3293c8

Author: Lev Stipakov
Date:   Wed Oct 3 20:21:21 2018 +0300

     openvpnserv: clarify return values type

     Signed-off-by: Lev Stipakov <[email protected]...>
     Acked-by: Selva Nair <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg17532.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

David Sommerseth

2018-10-03 21:05 UTC #8 lore raw

In reply to Selva Nair

Quoted 9 lines · Selva Nair

But ... are you saying Windows C compilers we use/support does not support stdbool.h? Even with -std=c99?
It does, but we cannot avoid using the Windows-specific BOOL (which I believe is a typedef to int pulled in through windows.h) as Windows API is replete with it. In fact stdbool's bool may be internally an int and thus same as BOOL but we can't assume that.

Fair point, and I was not aware of this detail. In this case I have no issues with keeping BOOL when it is tied to Windows APIs expecting BOOL.

Signature · 4 lines

kind regards,

David Sommerseth
OpenVPN Inc

Lev Stipakov

author

2018-10-03 16:56 UTC #9 lore raw

In reply to Selva Nair

Hi,

Wishlist: openvpn_swprintf() with nul termination guarantee. I try to avoid

Quoted 2 lines

the TCHAR variety be explicit about wide and narrow characters.

Makes sense, at the moment we have 8 swprintf calls all followed by something like

Quoted 1 line

tmp[_countof(tmp)-1] = L'\0';

Will do.

-Lev

Selva Nair

2018-10-03 17:10 UTC #10 lore raw

Hi,

Quoted 11 lines · Lev Stipakov

Hi,
Wishlist: openvpn_swprintf() with nul termination guarantee. I try to avoid
Quoted 2 lines
the TCHAR variety be explicit about wide and narrow characters.
Makes sense, at the moment we have 8 swprintf calls all followed by something like
Quoted 1 line
tmp[_countof(tmp)-1] = L'\0';

That must be me --- nul termination paranoia :) Cant blame, given none of these x[n]printf variants
guarantee nul termination in spite of taking the buffer length as an input..

Selva