headers
Reimar Döffinger | 22 Jul 2012 21:05
Picon
Picon

[PATCH] [RFC] avoid _vscnprintf since it exists only since WinXP.

The MSDN documentation is wrong, this function does not exist
on e.g. Win2k.
By my reading of the only case where it is used, this hack should work
almost as well, though it is ugly and comes with a risk of breaking
in the future.

Signed-off-by: Reimar Döffinger <Reimar.Doeffinger <at> gmx.de>
---
 libavutil/bprint.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavutil/bprint.c b/libavutil/bprint.c
index 6e59f6b..8b4996e 100644
--- a/libavutil/bprint.c
+++ b/libavutil/bprint.c
 <at>  <at>  -37,7 +37,7  <at>  <at>  static int vsnprintf_fixed(char *s, size_t n, const char *format, va_list va)
     r = vsnprintf(s, n, format, va2);
     va_end(va2);
     if (r == -1)
-        r = _vscprintf(format, va);
+        r = 2*n;
     return r;
 }

--

-- 
1.7.10.4

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel <at> ffmpeg.org
(Continue reading)

Permalink | Reply |
headers
Nicolas George | 22 Jul 2012 21:10
Favicon

Re: [PATCH] [RFC] avoid _vscnprintf since it exists only since WinXP.

Le quintidi 5 thermidor, an CCXX, Reimar Döffinger a écrit :
> The MSDN documentation is wrong, this function does not exist
> on e.g. Win2k.
> By my reading of the only case where it is used, this hack should work
> almost as well, though it is ugly and comes with a risk of breaking
> in the future.

I intended to submit a slightly less ugly hack (vsnprintf to a reasonably
large automatic buffer and memcpy). But this version is fine for now.

I would be happier if there was a configure check. Unfortunately, I am
completely clueless about the maze of twisty compiler and libc versions, all
alike that makes the windows build environment

Regards,

--

-- 
  Nicolas George

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel <at> ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Permalink | Reply |
headers
Reimar Döffinger | 22 Jul 2012 21:29
Picon
Picon

Re: [PATCH] [RFC] avoid _vscnprintf since it exists only since WinXP.

On Sun, Jul 22, 2012 at 09:10:06PM +0200, Nicolas George wrote:
> Le quintidi 5 thermidor, an CCXX, Reimar Döffinger a écrit :
> > The MSDN documentation is wrong, this function does not exist
> > on e.g. Win2k.
> > By my reading of the only case where it is used, this hack should work
> > almost as well, though it is ugly and comes with a risk of breaking
> > in the future.
> 
> I intended to submit a slightly less ugly hack (vsnprintf to a reasonably
> large automatic buffer and memcpy). But this version is fine for now.

I think there is no hurry if you work on something anyway.

> I would be happier if there was a configure check. Unfortunately, I am
> completely clueless about the maze of twisty compiler and libc versions, all
> alike that makes the windows build environment

The problem is that the usual use-case on Windows is that someone builds
a generic binary everyone uses.
A configure check doesn't really work well.
Also IMHO unless there is a serious problem with it I consider it better
to always use the fallback code, it reduces overall complexity and
avoids having code paths that are almost never tested.
Permalink | Reply |
headers
jamal | 22 Jul 2012 21:45
Picon

Re: [PATCH] [RFC] avoid _vscnprintf since it exists only since WinXP.

How about using this for the time being?

#if (_WIN32_WINNT >= 0x0501)
        r = _vscprintf(format, va);
#else
        r = 2*n;
#endif

And as Döffinger said a configure check will do nothing since both mingw32 and mingw64 (32 and 64 bits)
declare _vscprintf unconditionally regardless of the target Windows version, which is what the MSDN
Library states.
I find it really weird that the MSDN Library has this wrong to be honest.

Regards.

On 22/07/12 4:10 PM, Nicolas George wrote:
> Le quintidi 5 thermidor, an CCXX, Reimar Döffinger a écrit :
>> The MSDN documentation is wrong, this function does not exist
>> on e.g. Win2k.
>> By my reading of the only case where it is used, this hack should work
>> almost as well, though it is ugly and comes with a risk of breaking
>> in the future.
> I intended to submit a slightly less ugly hack (vsnprintf to a reasonably
> large automatic buffer and memcpy). But this version is fine for now.
>
> I would be happier if there was a configure check. Unfortunately, I am
> completely clueless about the maze of twisty compiler and libc versions, all
> alike that makes the windows build environment
>
> Regards,
(Continue reading)

Permalink | Reply |
headers
Reimar Döffinger | 22 Jul 2012 22:01
Picon
Picon

Re: [PATCH] [RFC] avoid _vscnprintf since it exists only since WinXP.

On Sun, Jul 22, 2012 at 04:45:58PM -0300, jamal wrote:
> How about using this for the time being?
> 
> #if (_WIN32_WINNT >= 0x0501)
>         r = _vscprintf(format, va);
> #else
>         r = 2*n;
> #endif

Well, I am not keen on adding extra complexity for this case (win2k
doesn't seem important enough for that).
Part of the [RFC] thing is kind of e.g. why it is used at all,
and why is both the vsnprintf return value and a allocation loop used.
Overall the code seems kind of strange to me, for example the check
for negative return values - the man page says "If an output error is
encountered, a negative value is returned."
However there can be no output error for vsnprintf, so I would have
expected it to rather be e.g. an av_assert0...
But since Nicolas said be intends to change it anyway it's probably
moot to discuss it now.
Permalink | Reply |
headers
Derek Buitenhuis | 22 Jul 2012 21:38
Picon
Gravatar

Re: [PATCH] [RFC] avoid _vscnprintf since it exists only since WinXP.

On 22/07/2012 3:05 PM, Reimar Döffinger wrote:
> The MSDN documentation is wrong, this function does not exist
> on e.g. Win2k.
> By my reading of the only case where it is used, this hack should work
> almost as well, though it is ugly and comes with a risk of breaking
> in the future.

Why? Do we really support OSes that MS doesn't even support anymore?

If someone is using Win2k, they're already using an unsupported OS that
things will likely not work on. Adding this sort of thing for such a
purpose is just silly.

This sort of stuff pollutes the code base, and compounds over time.

"Bawwwwwww this binary won't work on my OS from 12 years ago that isn't
even supported by it's author!"

- Derek
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel <at> ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Permalink | Reply |
headers
Reimar Döffinger | 22 Jul 2012 21:53
Picon
Picon

Re: [PATCH] [RFC] avoid _vscnprintf since it exists only since WinXP.

On Sun, Jul 22, 2012 at 03:38:43PM -0400, Derek Buitenhuis wrote:
> On 22/07/2012 3:05 PM, Reimar Döffinger wrote:
> > The MSDN documentation is wrong, this function does not exist
> > on e.g. Win2k.
> > By my reading of the only case where it is used, this hack should work
> > almost as well, though it is ugly and comes with a risk of breaking
> > in the future.
> 
> Why? Do we really support OSes that MS doesn't even support anymore?

Well, I see no reason not to try.

> If someone is using Win2k, they're already using an unsupported OS that
> things will likely not work on. Adding this sort of thing for such a
> purpose is just silly.

Well, it actually removes some code that seems completely unnecessary.
To I believe the code would be nicer and simpler if it didn't rely on
the vsnprintf return value at all, or at least used it only as a hint.

> "Bawwwwwww this binary won't work on my OS from 12 years ago that isn't
> even supported by it's author!"

There's no need to be insulting. Obviously people are still using it,
and while I don't see them seriously complaining (reporting it seems
very reasonable since the MSDN docs are wrong) that's no reason
to not discuss simple solutions.
And we do support e.g. OS/2, BeOS/Haiku and a lot of other stuff that
I'm sure has fewer users than Win2k (there's a good chance there's
actually fewer Linux users out there than people using a Windows before
(Continue reading)

Permalink | Reply |
headers
Derek Buitenhuis | 22 Jul 2012 21:59
Picon
Gravatar

Re: [PATCH] [RFC] avoid _vscnprintf since it exists only since WinXP.

On 22/07/2012 3:53 PM, Reimar Döffinger wrote
>> Why? Do we really support OSes that MS doesn't even support anymore?
> 
> Well, I see no reason not to try.

I think if it fills the code base with hacks or workarounds, or just
lots of extra code, it's a waste of effort to support an already
unsupported OS.

>> If someone is using Win2k, they're already using an unsupported OS that
>> things will likely not work on. Adding this sort of thing for such a
>> purpose is just silly.
> 
> Well, it actually removes some code that seems completely unnecessary.
> To I believe the code would be nicer and simpler if it didn't rely on
> the vsnprintf return value at all, or at least used it only as a hint.

You're right on this point. Agreed.

>> "Bawwwwwww this binary won't work on my OS from 12 years ago that isn't
>> even supported by it's author!"
> 
> There's no need to be insulting. Obviously people are still using it,
> and while I don't see them seriously complaining (reporting it seems
> very reasonable since the MSDN docs are wrong) that's no reason
> to not discuss simple solutions.
> And we do support e.g. OS/2, BeOS/Haiku and a lot of other stuff that
> I'm sure has fewer users than Win2k (there's a good chance there's
> actually fewer Linux users out there than people using a Windows before
> XP).
(Continue reading)

Permalink | Reply |
headers
Reimar Döffinger | 22 Jul 2012 22:15
Picon
Picon

Re: [PATCH] [RFC] avoid _vscnprintf since it exists only since WinXP.

On Sun, Jul 22, 2012 at 03:59:05PM -0400, Derek Buitenhuis wrote:
> On 22/07/2012 3:53 PM, Reimar Döffinger wrote
> >> Why? Do we really support OSes that MS doesn't even support anymore?
> > 
> > Well, I see no reason not to try.
> 
> I think if it fills the code base with hacks or workarounds, or just
> lots of extra code, it's a waste of effort to support an already
> unsupported OS.

I disagree with it being a waste of effort, but I agree that the
level of extra effort/code needs to be very limited.
But I admit that I was curious about the code for a few other reasons
and I probably should have brought that up from the start.

> >> "Bawwwwwww this binary won't work on my OS from 12 years ago that isn't
> >> even supported by it's author!"
> > 
> > There's no need to be insulting. Obviously people are still using it,
> > and while I don't see them seriously complaining (reporting it seems
> > very reasonable since the MSDN docs are wrong) that's no reason
> > to not discuss simple solutions.
> > And we do support e.g. OS/2, BeOS/Haiku and a lot of other stuff that
> > I'm sure has fewer users than Win2k (there's a good chance there's
> > actually fewer Linux users out there than people using a Windows before
> > XP).
> 
> Sorry, I'll take off my troll hat.

Thanks. I felt compelled to complain about it because I am not aware of
(Continue reading)

Permalink | Reply |
headers
Nicolas George | 23 Jul 2012 01:01
Favicon

Re: [PATCH] [RFC] avoid _vscnprintf since it exists only since WinXP.

Le quintidi 5 thermidor, an CCXX, Reimar Döffinger a écrit :
> But I admit that I was curious about the code for a few other reasons
> and I probably should have brought that up from the start.

The basic reason behind the design of the code is this: printf and co are a
complex piece of code from the libc. With so many supported OSes, there will
likely be bogus implementations.

There are two specific points about vsnprintf that I do not want to trust
unconditionally:

- That it will not fail for bizarre reasons (maybe a strange locale set by
  the application).

- That the return value when the buffer is too small is accurate.

For the first point, I can not do much, but I can be sure not to loop when
the return value is negative.

For the second point here is my solution:

If I trusted vsnprintf, I could write:

	len = vsnprintf(...);
	if (the buffer was too small) {
	    enlarge the buffer to len;
	    len = vsnprintf(...);
	}

and it would work. What I wrote instead is:
(Continue reading)

Permalink | Reply |
headers
Nicolas George | 22 Jul 2012 22:00
Favicon

Re: [PATCH] [RFC] avoid _vscnprintf since it exists only since WinXP.

Le quintidi 5 thermidor, an CCXX, Reimar Döffinger a écrit :
> Well, it actually removes some code that seems completely unnecessary.
> To I believe the code would be nicer and simpler if it didn't rely on
> the vsnprintf return value at all, or at least used it only as a hint.

There are two issues for that:

First, one of the design features of bprintf is to be able to compute the
size of the contents even beyond the size limit on the buffer. If we drop
this feature (only used in avfilter_graph_dump() AFAIK, and easily
replaced), it is less of a problem.

Second, doubling the buffer until vsnprintf stops returning an error is
risky since there may be other reasons for vsnprintf to return an error than
a buffer too small: in that case, the error will never go away and the
buffer will double until INT_MAX.

Regards,

--

-- 
  Nicolas George

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel <at> ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Permalink | Reply |
headers
Reimar Döffinger | 22 Jul 2012 22:04
Picon
Picon

Re: [PATCH] [RFC] avoid _vscnprintf since it exists only since WinXP.

On Sun, Jul 22, 2012 at 10:00:17PM +0200, Nicolas George wrote:
> Second, doubling the buffer until vsnprintf stops returning an error is
> risky since there may be other reasons for vsnprintf to return an error than
> a buffer too small: in that case, the error will never go away and the
> buffer will double until INT_MAX.

Actually that was part of why it is RFC: Can you tell me when that would
be the case? I am not aware of when that could happen.
Permalink | Reply |

Gmane