diff mbox series

[Openvpn-devel] Fix format errors when cross-compiling for Windows

Message ID 1518788739-16610-1-git-send-email-steffan.karger@fox-it.com
State Accepted
Headers
Series [Openvpn-devel] Fix format errors when cross-compiling for Windows |

Commit Message

Steffan Karger Feb. 16, 2018, 2:45 a.m. UTC 
  Not all supported windows formatting libs are C99 compliant and some do not
grasp %ll (similar to %zu).  Use int64_t and PRIi64 to work around that.

Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
---
 src/openvpn/error.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Selva Nair Feb. 18, 2018, 8:04 a.m. UTC | #1 
Hi,

On Fri, Feb 16, 2018 at 8:45 AM, Steffan Karger
<steffan.karger@fox-it.com> wrote:
> Not all supported windows formatting libs are C99 compliant and some do not
> grasp %ll (similar to %zu).  Use int64_t and PRIi64 to work around that.
>
> Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
> ---
>  src/openvpn/error.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/openvpn/error.c b/src/openvpn/error.c
> index bf987d2..bc14e8c 100644
> --- a/src/openvpn/error.c
> +++ b/src/openvpn/error.c
> @@ -342,8 +342,8 @@ x_msg_va(const unsigned int flags, const char *format, va_list arglist)
>                  struct timeval tv;
>                  gettimeofday(&tv, NULL);
>
> -                fprintf(fp, "%lld.%06lu %x %s%s%s%s",
> -                        (long long)tv.tv_sec,
> +                fprintf(fp, "%"PRIi64".%06lu %x %s%s%s%s",
> +                        (int64_t)tv.tv_sec,
>                          (unsigned long)tv.tv_usec,
>                          flags,
>                          prefix,
> --

This is good but there are a number of such instances (otime.c,
packet_id.c, forward.c etc. within code path relevant to Windows).
Many related to time_t like here, one or two other uses of %lld. And
may be more if some DEBUG flags are enabled.

If those are left for future patches, that's ok. Just saying in case
its an oversight..

Selva

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Steffan Karger Feb. 18, 2018, 9:16 p.m. UTC | #2 
Hi,

On 18-02-18 20:04, Selva Nair wrote:
> On Fri, Feb 16, 2018 at 8:45 AM, Steffan Karger
> <steffan.karger@fox-it.com> wrote:
>> Not all supported windows formatting libs are C99 compliant and some do not
>> grasp %ll (similar to %zu).  Use int64_t and PRIi64 to work around that.
>>
>> Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
>> ---
>>  src/openvpn/error.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/openvpn/error.c b/src/openvpn/error.c
>> index bf987d2..bc14e8c 100644
>> --- a/src/openvpn/error.c
>> +++ b/src/openvpn/error.c
>> @@ -342,8 +342,8 @@ x_msg_va(const unsigned int flags, const char *format, va_list arglist)
>>                  struct timeval tv;
>>                  gettimeofday(&tv, NULL);
>>
>> -                fprintf(fp, "%lld.%06lu %x %s%s%s%s",
>> -                        (long long)tv.tv_sec,
>> +                fprintf(fp, "%"PRIi64".%06lu %x %s%s%s%s",
>> +                        (int64_t)tv.tv_sec,
>>                          (unsigned long)tv.tv_usec,
>>                          flags,
>>                          prefix,
>> --
> 
> This is good but there are a number of such instances (otime.c,
> packet_id.c, forward.c etc. within code path relevant to Windows).
> Many related to time_t like here, one or two other uses of %lld. And
> may be more if some DEBUG flags are enabled.
> 
> If those are left for future patches, that's ok. Just saying in case
> its an oversight..

Good point - I did the lazy thing and just fixed what my compiler was
complaining about.  (My OpenVPN-NL buildbots build with
"-Werror=implicit -Werror=format -Werror=format-security".)

For now, let's leave the rest - that doesn't break my build ;-) - for a
future patch.  I'll put it on my todo list, but will also happily review
patches from someone else if (s)he beats me to it.

-Steffan

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Selva Nair Feb. 19, 2018, 9:18 a.m. UTC | #3 
Hi,

On Mon, Feb 19, 2018 at 3:16 AM, Steffan Karger
<steffan.karger@fox-it.com> wrote:
> Hi,
>
> On 18-02-18 20:04, Selva Nair wrote:
>> On Fri, Feb 16, 2018 at 8:45 AM, Steffan Karger
>> <steffan.karger@fox-it.com> wrote:
>>> Not all supported windows formatting libs are C99 compliant and some do not
>>> grasp %ll (similar to %zu).  Use int64_t and PRIi64 to work around that.
>>>
>>> Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
>>> ---
>>>  src/openvpn/error.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/openvpn/error.c b/src/openvpn/error.c
>>> index bf987d2..bc14e8c 100644
>>> --- a/src/openvpn/error.c
>>> +++ b/src/openvpn/error.c
>>> @@ -342,8 +342,8 @@ x_msg_va(const unsigned int flags, const char *format, va_list arglist)
>>>                  struct timeval tv;
>>>                  gettimeofday(&tv, NULL);
>>>
>>> -                fprintf(fp, "%lld.%06lu %x %s%s%s%s",
>>> -                        (long long)tv.tv_sec,
>>> +                fprintf(fp, "%"PRIi64".%06lu %x %s%s%s%s",
>>> +                        (int64_t)tv.tv_sec,
>>>                          (unsigned long)tv.tv_usec,
>>>                          flags,
>>>                          prefix,
>>> --
>>
>> This is good but there are a number of such instances (otime.c,
>> packet_id.c, forward.c etc. within code path relevant to Windows).
>> Many related to time_t like here, one or two other uses of %lld. And
>> may be more if some DEBUG flags are enabled.
>>
>> If those are left for future patches, that's ok. Just saying in case
>> its an oversight..
>
> Good point - I did the lazy thing and just fixed what my compiler was
> complaining about.  (My OpenVPN-NL buildbots build with
> "-Werror=implicit -Werror=format -Werror=format-security".)

Makes sense.. But I'm surprised why it doesn't catch all errors.

>
> For now, let's leave the rest - that doesn't break my build ;-) - for a
> future patch.  I'll put it on my todo list, but will also happily review
> patches from someone else if (s)he beats me to it.

I am sending in a patch that covers all format errors that I see in current
windows cross builds.

Selva

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Selva Nair Feb. 19, 2018, 9:44 a.m. UTC | #4 
Hi,

On Mon, Feb 19, 2018 at 3:18 PM, Selva Nair <selva.nair@gmail.com> wrote:
> Hi,
>
> On Mon, Feb 19, 2018 at 3:16 AM, Steffan Karger
> <steffan.karger@fox-it.com> wrote:
>> Hi,
>>
>> On 18-02-18 20:04, Selva Nair wrote:
>>> On Fri, Feb 16, 2018 at 8:45 AM, Steffan Karger
>>> <steffan.karger@fox-it.com> wrote:
>>>> Not all supported windows formatting libs are C99 compliant and some do not
>>>> grasp %ll (similar to %zu).  Use int64_t and PRIi64 to work around that.
>>>>
>>>> Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
>>>> ---
>>>>  src/openvpn/error.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/src/openvpn/error.c b/src/openvpn/error.c
>>>> index bf987d2..bc14e8c 100644
>>>> --- a/src/openvpn/error.c
>>>> +++ b/src/openvpn/error.c
>>>> @@ -342,8 +342,8 @@ x_msg_va(const unsigned int flags, const char *format, va_list arglist)
>>>>                  struct timeval tv;
>>>>                  gettimeofday(&tv, NULL);
>>>>
>>>> -                fprintf(fp, "%lld.%06lu %x %s%s%s%s",
>>>> -                        (long long)tv.tv_sec,
>>>> +                fprintf(fp, "%"PRIi64".%06lu %x %s%s%s%s",
>>>> +                        (int64_t)tv.tv_sec,
>>>>                          (unsigned long)tv.tv_usec,
>>>>                          flags,
>>>>                          prefix,
>>>> --
>>>
>>> This is good but there are a number of such instances (otime.c,
>>> packet_id.c, forward.c etc. within code path relevant to Windows).
>>> Many related to time_t like here, one or two other uses of %lld. And
>>> may be more if some DEBUG flags are enabled.
>>>
>>> If those are left for future patches, that's ok. Just saying in case
>>> its an oversight..
>>
>> Good point - I did the lazy thing and just fixed what my compiler was
>> complaining about.  (My OpenVPN-NL buildbots build with
>> "-Werror=implicit -Werror=format -Werror=format-security".)
>
> Makes sense.. But I'm surprised why it doesn't catch all errors.
>
>>
>> For now, let's leave the rest - that doesn't break my build ;-)

Anyway, assuming that (i) my longer patch will take more time to get
reviewed/tested and (ii) this is priority if a release is imminent,
ACK from me.

Acked-by: Selva Nair <selva.nair@gmail.com>

(Only compile tested)

Selva

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Gert Doering Feb. 19, 2018, 8:48 p.m. UTC | #5 
Your patch has been applied to the release/2.4 branch.

Note: the code in master looks subtly different here - also using %lld
for tv.tv_sec, but using %ld / (long) for tv.tv_usec so the patch does not
directly apply.  Since Selva is working on fixing all these for good, I've
decided to take the easy way and only apply this patch to 2.4 - which it
very much looks like it's intended for :-)

commit 9ba36639abcac4367c8227d2dd87b18fb56267c4 (release/2.4)
Author: Steffan Karger
Date:   Fri Feb 16 14:45:39 2018 +0100

     Fix format errors when cross-compiling for Windows

     Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
     Acked-by: Selva Nair <selva.nair@gmail.com>
     Message-Id: <1518788739-16610-1-git-send-email-steffan.karger@fox-it.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg16478.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
diff mbox series

Patch

diff --git a/src/openvpn/error.c b/src/openvpn/error.c
index bf987d2..bc14e8c 100644
--- a/src/openvpn/error.c
+++ b/src/openvpn/error.c
@@ -342,8 +342,8 @@  x_msg_va(const unsigned int flags, const char *format, va_list arglist)
                 struct timeval tv;
                 gettimeofday(&tv, NULL);
 
-                fprintf(fp, "%lld.%06lu %x %s%s%s%s",
-                        (long long)tv.tv_sec,
+                fprintf(fp, "%"PRIi64".%06lu %x %s%s%s%s",
+                        (int64_t)tv.tv_sec,
                         (unsigned long)tv.tv_usec,
                         flags,
                         prefix,