diff mbox series

[Openvpn-devel,v2] openvpnserv: clarify return values type

Message ID 1538587281-3209-1-git-send-email-lstipakov@gmail.com
State Accepted
Headers show
Series [Openvpn-devel,v2] openvpnserv: clarify return values type | expand

Commit Message

Lev Stipakov Oct. 3, 2018, 7:21 a.m. UTC 
From: Lev Stipakov <lev@openvpn.net>

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 <lev@openvpn.net>
---
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(-)

Comments

Selva Nair Oct. 3, 2018, 10:13 a.m. UTC | #1 
Hi,

On Wed, Oct 3, 2018 at 1:24 PM Lev Stipakov <lstipakov@gmail.com> wrote:

> From: Lev Stipakov <lev@openvpn.net>
>
> 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 <lev@openvpn.net>
> ---
> 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(-)
>
> diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c
> index 1f98283..73d3003 100644
> --- a/src/openvpnserv/automatic.c
> +++ b/src/openvpnserv/automatic.c
> @@ -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 };
>
> diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c
> index dc47666..6b3a859 100644
> --- a/src/openvpnserv/common.c
> +++ b/src/openvpnserv/common.c
> @@ -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;
>          }
> diff --git a/src/openvpnserv/service.h b/src/openvpnserv/service.h
> index 4d03b88..b90033a 100644
> --- a/src/openvpnserv/service.h
> +++ b/src/openvpnserv/service.h
> @@ -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);
>
>
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: selva.nair@gmail.com
<div dir="ltr">Hi,<br><br><div class="gmail_quote"><div dir="ltr">On Wed, Oct 3, 2018 at 1:24 PM Lev Stipakov &lt;<a href="mailto:lstipakov@gmail.com" target="_blank">lstipakov@gmail.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From: Lev Stipakov &lt;<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>&gt;<br>
<br>
Functions openvpn_vsntprintf and openvpn_sntprintf return<br>
values of type int, but in reality it is always 0 or 1 (and -1 for<br>
snrptinf), which can be represented as boolean.<br>
<br>
To make code clearer, change return type to BOOL. Also<br>
use stdbool.h header instead of bool definition macros in automatic.c<br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Signed-off-by: Lev Stipakov &lt;<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>&gt;<br>
---<br>
v2: replace bool with BOOL as discussed<br>
<br>
 src/openvpnserv/automatic.c |  6 +-----<br>
 src/openvpnserv/common.c    | 13 +++++++------<br>
 src/openvpnserv/service.h   |  4 ++--<br>
 3 files changed, 10 insertions(+), 13 deletions(-)<br>
<br>
diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c<br>
index 1f98283..73d3003 100644<br>
--- a/src/openvpnserv/automatic.c<br>
+++ b/src/openvpnserv/automatic.c<br>
@@ -36,13 +36,9 @@<br>
<br>
 #include &lt;stdio.h&gt;<br>
 #include &lt;stdarg.h&gt;<br>
+#include &lt;stdbool.h&gt; <br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
 #include &lt;process.h&gt;<br>
<br>
-/* bool definitions */<br>
-#define bool int<br>
-#define true 1<br>
-#define false 0<br>
-<br>
 static SERVICE_STATUS_HANDLE service;<br>
 static SERVICE_STATUS status = { .dwServiceType = SERVICE_WIN32_SHARE_PROCESS };<br>
<br>
diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c<br>
index dc47666..6b3a859 100644<br>
--- a/src/openvpnserv/common.c<br>
+++ b/src/openvpnserv/common.c<br>
@@ -31,7 +31,7 @@ LPCTSTR service_instance = TEXT(&quot;&quot;);<br>
  * These are necessary due to certain buggy implementations of (v)snprintf,<br>
  * that don&#39;t guarantee null termination for size &gt; 0.<br>
  */<br>
-int<br>
+BOOL<br>
 openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list arglist)<br>
 {<br>
     int len = -1;<br>
@@ -42,18 +42,19 @@ openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list arglist)<br>
     }<br>
     return (len &gt;= 0 &amp;&amp; len &lt; size);<br>
 }<br>
-int<br>
+<br>
+BOOL<br>
 openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...)<br>
 {<br>
     va_list arglist;<br>
-    int len = -1;<br>
+    BOOL res = FALSE;<br>
     if (size &gt; 0)<br>
     {<br>
         va_start(arglist, format);<br>
-        len = openvpn_vsntprintf(str, size, format, arglist);<br>
+        res = openvpn_vsntprintf(str, size, format, arglist);<br>
         va_end(arglist);<br>
     }<br>
-    return len;<br>
+    return res;<br>
 }<br>
<br>
 static DWORD<br>
@@ -65,7 +66,7 @@ GetRegString(HKEY key, LPCTSTR value, LPTSTR data, DWORD size, LPCTSTR default_v<br>
     if (status == ERROR_FILE_NOT_FOUND &amp;&amp; default_value)<br>
     {<br>
         size_t len = size/sizeof(data[0]);<br>
-        if (openvpn_sntprintf(data, len, default_value) &gt; 0)<br>
+        if (openvpn_sntprintf(data, len, default_value))<br>
         {<br>
             status = ERROR_SUCCESS;<br>
         }<br>
diff --git a/src/openvpnserv/service.h b/src/openvpnserv/service.h<br>
index 4d03b88..b90033a 100644<br>
--- a/src/openvpnserv/service.h<br>
+++ b/src/openvpnserv/service.h<br>
@@ -82,9 +82,9 @@ VOID WINAPI ServiceStartAutomatic(DWORD argc, LPTSTR *argv);<br>
 VOID WINAPI ServiceStartInteractiveOwn(DWORD argc, LPTSTR *argv);<br>
 VOID WINAPI ServiceStartInteractive(DWORD argc, LPTSTR *argv);<br>
<br>
-int openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list arglist);<br>
+BOOL openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list arglist);<br>
<br>
-int openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...);<br>
+BOOL openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...);<br>
<br>
 DWORD GetOpenvpnSettings(settings_t *s);<br>
<br></blockquote><div><br></div><div>The originals had confusing return values with a success/fail code hidden in what<br>looked like a length. The code is cleaner and clearer this way.<br>And, builds/runs fine (mingw cross-build + Windows 7 run time).</div><div> <br></div><div>Acked-by: <a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a><br></div></div></div>
Gert Doering Oct. 7, 2018, 1:23 a.m. UTC | #2 
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 <lev@openvpn.net>
     Acked-by: Selva Nair <selva.nair@gmail.com>
     Message-Id: <1538587281-3209-1-git-send-email-lstipakov@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg17532.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c
index 1f98283..73d3003 100644
--- a/src/openvpnserv/automatic.c
+++ b/src/openvpnserv/automatic.c
@@ -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 };
 
diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c
index dc47666..6b3a859 100644
--- a/src/openvpnserv/common.c
+++ b/src/openvpnserv/common.c
@@ -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;
         }
diff --git a/src/openvpnserv/service.h b/src/openvpnserv/service.h
index 4d03b88..b90033a 100644
--- a/src/openvpnserv/service.h
+++ b/src/openvpnserv/service.h
@@ -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);