[Openvpn-devel] Wrap openvpn_swprintf into Windows define

Message ID 1540306288-23847-1-git-send-email-lstipakov@gmail.com
State Accepted, archived
Headers show
Series
  • [Openvpn-devel] Wrap openvpn_swprintf into Windows define
Related show

Commit Message

Lev Stipakov Oct. 23, 2018, 2:51 p.m.
From: Lev Stipakov <lev@openvpn.net>

Commit 43a5a4f3b4e411419639c195fee8a76495fdc88e added
vswprintf() call which turned to me missing in OpenBSD 4.9.

Since that call is inside openvpn_swprintf() function which
is only used by Windows, wrap that function info #ifdef _WIN32.

Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
 src/openvpn/buffer.c | 3 +++
 src/openvpn/buffer.h | 5 +++++
 2 files changed, 8 insertions(+)

Comments

Antonio Quartulli Oct. 24, 2018, 10:22 a.m. | #1
Hi,

On 23/10/18 22:51, Lev Stipakov wrote:
> From: Lev Stipakov <lev@openvpn.net>
> 
> Commit 43a5a4f3b4e411419639c195fee8a76495fdc88e added
> vswprintf() call which turned to me missing in OpenBSD 4.9.
> 
> Since that call is inside openvpn_swprintf() function which
> is only used by Windows, wrap that function info #ifdef _WIN32.
> 
> Signed-off-by: Lev Stipakov <lev@openvpn.net>

This patch introduces another ifdef, but it is enclosing a whole
function to prevent it from being compiled on non-windows platforms,
therefore I consider it "acceptable" for the time being.

This patch fixes building openvpn on OpenBSD <= 4.9 (maybe this info
should be added to the commit message).

Other than that:

Acked-by: Antonio Quartulli <antonio@openvpn.net>
Selva Nair Oct. 24, 2018, 12:39 p.m. | #2
On Wed, Oct 24, 2018 at 6:23 AM Antonio Quartulli <a@unstable.cc> wrote:
>
> Hi,
>
> On 23/10/18 22:51, Lev Stipakov wrote:
> > From: Lev Stipakov <lev@openvpn.net>
> >
> > Commit 43a5a4f3b4e411419639c195fee8a76495fdc88e added
> > vswprintf() call which turned to me missing in OpenBSD 4.9.
> >
> > Since that call is inside openvpn_swprintf() function which
> > is only used by Windows, wrap that function info #ifdef _WIN32.
> >
> > Signed-off-by: Lev Stipakov <lev@openvpn.net>
>
> This patch introduces another ifdef, but it is enclosing a whole
> function to prevent it from being compiled on non-windows platforms,
> therefore I consider it "acceptable" for the time being.
>
> This patch fixes building openvpn on OpenBSD <= 4.9 (maybe this info
> should be added to the commit message).

Also there is a misplaced and mismatched comment added to
buffer.c that says

+        /* vswprintf is missing in OpenBSD 4.2 */
         len = vswprintf(str, size, format, arglist);

Makes little sense in that context and why 4.2..
That one could be removed.

Otherwise an ACK from me too.

Selva
David Sommerseth Oct. 24, 2018, 1 p.m. | #3
On 24/10/18 14:39, Selva Nair wrote:
> On Wed, Oct 24, 2018 at 6:23 AM Antonio Quartulli <a@unstable.cc> wrote:
>>
>> Hi,
>>
>> On 23/10/18 22:51, Lev Stipakov wrote:
>>> From: Lev Stipakov <lev@openvpn.net>
>>>
>>> Commit 43a5a4f3b4e411419639c195fee8a76495fdc88e added
>>> vswprintf() call which turned to me missing in OpenBSD 4.9.
>>>
>>> Since that call is inside openvpn_swprintf() function which
>>> is only used by Windows, wrap that function info #ifdef _WIN32.
>>>
>>> Signed-off-by: Lev Stipakov <lev@openvpn.net>
>>
>> This patch introduces another ifdef, but it is enclosing a whole
>> function to prevent it from being compiled on non-windows platforms,
>> therefore I consider it "acceptable" for the time being.
>>
>> This patch fixes building openvpn on OpenBSD <= 4.9 (maybe this info
>> should be added to the commit message).
> 
> Also there is a misplaced and mismatched comment added to
> buffer.c that says
> 
> +        /* vswprintf is missing in OpenBSD 4.2 */
>          len = vswprintf(str, size, format, arglist);
> 
> Makes little sense in that context and why 4.2..
> That one could be removed.
I'd say we should have such a reference in right below the #ifdef _WIN32 a few
lines higher up.  And it could be slightly more informative (and should
reference OpenBSD 4.9, which is the distro exploding without this patch.

Primary reason for this message is to explain why we do not provide this
function to "everyone", if someone has a use case for it later on in a
non-Windows code path.  If this is needed elsewhere, we at least know
instantly what might break and we can evaluate if it's worth keeping OpenBSD
4.9 support.

I suggest the comment below the #ifdef _WIN32 to be something like:

/*
 * openvpn_swprintf() is currently only used by Windows code paths
 * and when enabled for all platforms it will currently break
 * OpenBSD 4.9 which lacks vswprintf(3) support in its libc.
 */
Selva Nair Oct. 24, 2018, 1:36 p.m. | #4
Hi,

On Wed, Oct 24, 2018 at 9:00 AM David Sommerseth
<openvpn@sf.lists.topphemmelig.net> wrote:
>
> On 24/10/18 14:39, Selva Nair wrote:
> > On Wed, Oct 24, 2018 at 6:23 AM Antonio Quartulli <a@unstable.cc> wrote:
> >>
> >> Hi,
> >>
> >
> >
> > Also there is a misplaced and mismatched comment added to
> > buffer.c that says
> >
> > +        /* vswprintf is missing in OpenBSD 4.2 */
> >          len = vswprintf(str, size, format, arglist);
> >
> > Makes little sense in that context and why 4.2..
> > That one could be removed.
> I'd say we should have such a reference in right below the #ifdef _WIN32 a few
> lines higher up.  And it could be slightly more informative (and should
> reference OpenBSD 4.9, which is the distro exploding without this patch.

Yes, that's why I wrote "misplaced" and "mismatched" :)

> I suggest the comment below the #ifdef _WIN32 to be something like:
>
> /*
>  * openvpn_swprintf() is currently only used by Windows code paths
>  * and when enabled for all platforms it will currently break
>  * OpenBSD 4.9 which lacks vswprintf(3) support in its libc.
>  */

Looks good except,  <=4.9, not just 4.9, or whatever version is appropriate.
(As Antonio already pointed out.)

Selva
Gert Doering Oct. 24, 2018, 3:55 p.m. | #5
Your patch has been applied to the master branch.  

Make OpenBSD great^w green again!  (Tested before pushing :) )

(I have amended the comments according to the discussion)

commit 4ce1a9b6b3ae0be32a3d999c5bc93731c64d7bbb
Author: Lev Stipakov
Date:   Tue Oct 23 17:51:28 2018 +0300

     Wrap openvpn_swprintf into Windows define

     Signed-off-by: Lev Stipakov <lev@openvpn.net>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Acked-by: Selva Nair <selva.nair@gmail.com>
     Message-Id: <1540306288-23847-1-git-send-email-lstipakov@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg17799.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c
index 8ca189f..acf76ae 100644
--- a/src/openvpn/buffer.c
+++ b/src/openvpn/buffer.c
@@ -310,6 +310,7 @@  openvpn_snprintf(char *str, size_t size, const char *format, ...)
     return (len >= 0 && len < size);
 }
 
+#ifdef _WIN32
 bool
 openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...)
 {
@@ -318,12 +319,14 @@  openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const for
     if (size > 0)
     {
         va_start(arglist, format);
+        /* vswprintf is missing in OpenBSD 4.2 */
         len = vswprintf(str, size, format, arglist);
         va_end(arglist);
         str[size - 1] = L'\0';
     }
     return (len >= 0 && len < size);
 }
+#endif
 
 /*
  * write a string to the end of a buffer that was
diff --git a/src/openvpn/buffer.h b/src/openvpn/buffer.h
index c5b78a0..c48c925 100644
--- a/src/openvpn/buffer.h
+++ b/src/openvpn/buffer.h
@@ -449,7 +449,11 @@  __attribute__ ((format(__printf__, 3, 4)))
 ;
 
 
+#ifdef _WIN32
 /*
+ * This is under #ifdef because only Windows-specific code
+ * uses this function and its implementation is broken in OpenBSD 4.9
+ *
  * Like swprintf but guarantees null termination for size > 0
  */
 bool
@@ -458,6 +462,7 @@  openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const for
  * Unlike in openvpn_snprintf, we cannot use format attributes since
  * GCC doesn't support wprintf as archetype.
  */
+#endif
 
 /*
  * remove/add trailing characters