| Message ID | 1538576212-2363-1-git-send-email-lstipakov@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] openvpnserv: clarify return values type | expand |
Hi, On Wed, Oct 3, 2018 at 10:20 AM 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, 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: > > 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. > Signed-off-by: Lev Stipakov <lev@openvpn.net> > --- > src/openvpnserv/automatic.c | 5 ----- > src/openvpnserv/common.c | 13 +++++++------ > src/openvpnserv/service.h | 5 +++-- > 3 files changed, 10 insertions(+), 13 deletions(-) > > diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c > index 1f98283..c2982c1 100644 > --- a/src/openvpnserv/automatic.c > +++ b/src/openvpnserv/automatic.c > @@ -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 }; > > diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c > index dc47666..69dd44b 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..eaa479a 100644 > --- a/src/openvpnserv/service.h > +++ b/src/openvpnserv/service.h > @@ -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); > > -- > 2.7.4 > Thanks, Selva <div dir="ltr">Hi,<br><br><div class="gmail_quote"><div dir="ltr">On Wed, Oct 3, 2018 at 10:20 AM Lev Stipakov <<a href="mailto:lstipakov@gmail.com">lstipakov@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From: Lev Stipakov <<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>><br> <br> Functions openvpn_vsntprintf and openvpn_sntprintf return<br> values of type int, but in reality it is always 0 or 1, which is<br> essentially bool.<br></blockquote><div><br></div><div>openvpn_sntprintf could return -1 if size = 0, but this looks like the<br>right approach and matches openvpn_snprintf in the core.<br>The code looks good too. </div><div><br></div><div>Just a thought about bool below:<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br> To make code more clear, change return type to bool. Also<br> use stdbool.h header instead of bool definition macros.<br></blockquote><div><br></div><div>We do use BOOL all over the service code (unavoidable with<br>Windows API), and bool is used in only one or two places. So would it<br>be better to just change this to BOOL/TRUE/FALSE and not include<br>stdbool.h?<br><br></div><div>Wishlist: openvpn_swprintf() with nul termination guarantee. I try to avoid<br>the TCHAR variety be explicit about wide and narrow characters.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br> Signed-off-by: Lev Stipakov <<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>><br> ---<br> src/openvpnserv/automatic.c | 5 -----<br> src/openvpnserv/common.c | 13 +++++++------<br> src/openvpnserv/service.h | 5 +++--<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..c2982c1 100644<br> --- a/src/openvpnserv/automatic.c<br> +++ b/src/openvpnserv/automatic.c<br> @@ -38,11 +38,6 @@<br> #include <stdarg.h><br> #include <process.h><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..69dd44b 100644<br> --- a/src/openvpnserv/common.c<br> +++ b/src/openvpnserv/common.c<br> @@ -31,7 +31,7 @@ LPCTSTR service_instance = TEXT("");<br> * These are necessary due to certain buggy implementations of (v)snprintf,<br> * that don't guarantee null termination for size > 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 >= 0 && len < 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 > 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 && default_value)<br> {<br> size_t len = size/sizeof(data[0]);<br> - if (openvpn_sntprintf(data, len, default_value) > 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..eaa479a 100644<br> --- a/src/openvpnserv/service.h<br> +++ b/src/openvpnserv/service.h<br> @@ -32,6 +32,7 @@<br> <br> #include <winsock2.h><br> #include <windows.h><br> +#include <stdbool.h><br> #include <stdlib.h><br> #include <tchar.h><br> <br> @@ -82,9 +83,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> -- <br> 2.7.4<br></blockquote><div><br></div><div>Thanks,<br><br></div><div>Selva <br></div></div></div>
On 03/10/18 17:08, Selva Nair wrote: > > > 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.
On Wed, Oct 3, 2018 at 12:05 PM David Sommerseth < openvpn@sf.lists.topphemmelig.net> wrote: > On 03/10/18 17:08, Selva Nair wrote: > > > > > > 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. > Yes, I know that's the path taken in openvpn core sources which is very sensible in a cross-platform code. > > 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 <div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Oct 3, 2018 at 12:05 PM David Sommerseth <<a href="mailto:openvpn@sf.lists.topphemmelig.net">openvpn@sf.lists.topphemmelig.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 03/10/18 17:08, Selva Nair wrote:<br> > <br> > <br> > To make code more clear, change return type to bool. Also<br> > use stdbool.h header instead of bool definition macros.<br> > <br> > We do use BOOL all over the service code (unavoidable with<br> > Windows API), and bool is used in only one or two places. So would it<br> > be better to just change this to BOOL/TRUE/FALSE and not include<br> > stdbool.h?<br> <br> Several years ago, we started towards a path to get rid of the BOOL/TRUE/FALSE<br> stuff, in favour of stdbool.h. And when we moved towards -std=c99, this made<br> this move even more reasonable.<br></blockquote><div><br></div><div>Yes, I know that's the path taken in openvpn core sources which is very sensible in<br></div><div>a cross-platform code.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br> But ... are you saying Windows C compilers we use/support does not support<br> stdbool.h? Even with -std=c99?<br></blockquote><div><br></div><div>It does, but we cannot avoid using the Windows-specific BOOL (which I<br>believe is a typedef to int pulled in through windows.h) as Windows API is<br>replete with it. In fact stdbool's bool may be internally an int and thus same<br>as BOOL but we can't assume that.<br></div><div><br></div><div>In case of the service code, which is Windows only, there are about 30 uses of<br>BOOL but only a few cases of bool (typdef to int) all of which are in the deprecated<br>automatic service code. This patch changes the latter to use stdbool which is<br>quite reasonable. But I just wonder whether its just better to use BOOL in<br>this occasion as the latter is not going to go away in Windows.<br></div><div><br></div><div>That said, I prefer stdbool too, and I'm fine with moving towards replacing all BOOL<br>with bool and casting to BOOL if/when required (say, calling Windows API).<br><br></div></div><div class="gmail_quote">Selva<br></div></div>
Hi, > 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.
Hi, Wishlist: openvpn_swprintf() with nul termination guarantee. I try to avoid > 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 > tmp[_countof(tmp)-1] = L'\0'; Will do. -Lev <div dir="ltr"><div dir="ltr">Hi,<div><br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div>Wishlist: openvpn_swprintf() with nul termination guarantee. I try to avoid<br>the TCHAR variety be explicit about wide and narrow characters.</div></div></div></blockquote><div><br></div><div>Makes sense, at the moment we have 8 swprintf calls all followed by something like</div><div><br></div><div> > tmp[_countof(tmp)-1] = L'\0';<br></div><div><br></div><div>Will do.</div><div><br></div><div>-Lev </div></div></div></div></div>
Hi, On Wed, Oct 3, 2018 at 12:56 PM Lev Stipakov <lstipakov@gmail.com> wrote: > Hi, > > Wishlist: openvpn_swprintf() with nul termination guarantee. I try to avoid >> 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 > > > 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 <div dir="ltr">Hi,<br><div><br><div class="gmail_quote"><div dir="ltr">On Wed, Oct 3, 2018 at 12:56 PM Lev Stipakov <<a href="mailto:lstipakov@gmail.com">lstipakov@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div dir="ltr">Hi,<div><br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div>Wishlist: openvpn_swprintf() with nul termination guarantee. I try to avoid<br>the TCHAR variety be explicit about wide and narrow characters.</div></div></div></blockquote><div><br></div><div>Makes sense, at the moment we have 8 swprintf calls all followed by something like</div><div><br></div><div> > tmp[_countof(tmp)-1] = L'\0';<br></div></div></div></div></div></blockquote><div><br></div><div>That must be me --- nul termination paranoia :) Cant blame, given none of these x[n]printf variants<br>guarantee nul termination in spite of taking the buffer length as an input..<br></div><div><br></div><div>Selva</div></div></div></div>
On 03/10/18 18:31, Selva Nair wrote: > > 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.
diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c index 1f98283..c2982c1 100644 --- a/src/openvpnserv/automatic.c +++ b/src/openvpnserv/automatic.c @@ -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 }; diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c index dc47666..69dd44b 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..eaa479a 100644 --- a/src/openvpnserv/service.h +++ b/src/openvpnserv/service.h @@ -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);