incubator-stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Martin Sebor <se...@roguewave.com>
Subject Re: svn commit: r647908 - in /stdcxx/trunk/tests: self/0.printf.cpp src/fmt_defs.h src/printf.cpp
Date Tue, 15 Apr 2008 04:34:50 GMT
Travis Vitek wrote:
>  
> 
>> Martin Sebor wrote:
>>
>> Travis Vitek wrote:
>>>  
>>>
>>> Martin Sebor wrote 
>>>
>>>> Wouldn't (*pbufsize == maxsize) in that case so that _rw_bufcat()
>>>> would never need to allocate new space to begin with (or it would
>>>> fail trying).
>>>>
>>> That only tells me if I've reached the maximum allowed 
>>> buffer size. It tells me nothing about where the *pbufsize
>>> bytes were allocated from, and that is exactly what I need,
>>> right?
>> Right. But nothing in printf.cpp should ever try to deallocate
>> the buffer except _rw_bufcat() to reallocate it, and _rw_bufcat()
>> should avoid reallocating when (*pbufsize == maxsize).
> 
> See discussion on this below.
> 
>>> I mean there are cases where (*pbufsize != maxsize) and we 
>>> don't want to deallocate the buffer. Consider this case which
>>> is taken from the test library...
>>>
>>>   static int
>>>   _rw_vsystem (const char *cmd, va_list va)
>>>   {
>>>       RW_ASSERT (0 != cmd);
>>>
>>>       char buffer [256];
>>>       char *buf = buffer;
>>>
>>>       size_t bufsize = sizeof buffer;
>>>
>>>       rw_vasnprintf (&buf, &bufsize, cmd, va);
>>>
>>>       // <snip>
>>>    }
>>>
>>> We definitely don't want `buf' to be deallocated, that much is clear.
>>> But how can `maxsize' possibly tell us anything about `buf'?
>> Here, rw_vasnprintf() should arrange for maxsize to be set to
>> bufsize so that _rw_bufcat() doesn't try to reallocate it when
>> it runs out of space. I realize that changes would be need to
>> make it work this way but it seems that it should be doable
>> with the existing machinery, no?
> 
> Unless I'm totally misunderstanding you (it seems that I may be), this
> would limit maximum command line length to 256 characters.

I forgot about this use case. I suppose with my approach code
like this would need to be changed to call rw_vasnprintf() twice
(or multiple times) to make this work the way it does now. The
same way we would use vsnprintf(). I.e., the first call would
either succeed or fail and return a value greater than bufsize
indicating the minimum size to fit the formatted string. Or, if
the function currently returns -1 on failure and we didn't want
to mess with changing it to do the formating w/o a buffer, we
could call it in a loop, increasing the size of the buffer in
each iteration, until it succeeded.

> 
>> Btw., this patch (applied to printf.cpp before rev 647908)
>> *seems* to do what I'm describing except that rw_printf()
>> croaks with the error:
>>
>> stdcxx/tests/src/printf.cpp:1027: rw_vasnprintf(0x7fbffff538, 
>> 0x7fbffff530, "%s", va_list) error: errno = 12: Cannot allocate memory
>>
>> Replacing it with a return statement with the short count
>> shouldn't be too hard.
>>
>> Index: tests/src/printf.cpp
>> ===================================================================
>> --- tests/src/printf.cpp        (revision 647907)
>> +++ tests/src/printf.cpp        (working copy)
>> @@ -955,11 +955,18 @@
>>  #define vacpy DONT_TOUCH_ME
>>
>>      size_t default_bufsize = 1024;
>> -    if (0 == pbufsize)
>> -        pbufsize = &default_bufsize;
>> +    size_t maxbufsize;
>>
>> -    Buffer buf = { pbuf, pbufsize, _RWSTD_SIZE_MAX, 0 };
>> +    if (0 == pbufsize) {
>> +        pbufsize   = &default_bufsize;
>> +        maxbufsize = _RWSTD_SIZE_MAX;
>> +    }
>> +    else {
>> +        maxbufsize = *pbufsize;
>> +    }
>>
>> +    Buffer buf = { pbuf, pbufsize, maxbufsize, 0 };
>>
> 
> I believe that if you applied this patch you would run into all sorts of

I know. The patch wasn't meant to be a finished product, just an
outline of the approach I'm proposing. There would need to be other
changes to make things like %{+} work.

> trouble in the test suite. Consider the following code (which is not
> unlike that which is not totally unlike some code that is in the
> 21.strings.cpp]
> 
>   char   buf* = 0;
>   size_t bufsz = 0;
> 
>   rw_vasnprintf (&buf, &bufsz, "%s", "hello world!");
>   rw_vasnprintf (&buf, &bufsz, "%{+}%s", "goodbye world!");
> 
> The first call has pbufsize != 0 && *pbufsize == 0. So buf.maxsize is
> going to be set to 0, and *buf.pbufsize will also be 0.

Right. For %{+} the patch above would need a tweak to set maxsize
to SIZE_MAX.

Let me write down the combination of rw_vasnprintf(buf, bufsize, fmat)
argument values and the behavior of the function (existing or possible
with some changes):

     buf     bufsize   maxbufsize %{+}   behavior
1.  0       0         SIZE_MAX   NO     allocate up to SIZE_MAX
2.  0       0         SIZE_MAX   YES    allocate up to SIZE_MAX
3.  0       non-0     bufsize    NO     allocate up to bufsize
4.  0       non-0     bufsize    YES    allocate up to bufsize
5.  non-0   0         bufsize    NO     do not reallocate
6.  non-0   0         SIZE_MAX   YES    reallocate up to SIZE_MAX
7.  non-0   non-0     bufsize    NO     do not reallocate
8.  non-0   non-0     SIZE_MAX   YES    reallocate up to SIZE_MAX

rw_sprintf() is case (5) with bufsize == SIZE_MAX, rw_snprintf()
is case (7). There's no way to call the function like _rw_vsystem()
does anymore and I can't think of a way to make it possible without
adding some info telling _rw_bufcat() not to free the initially
provided buffer, kind of like you did in your patch.

Okay, I'm convinced that there's no way to accommodate the existing
use cases w/o changing some of the callers so I'm fine with your
approach. (Although from your recent post it sounds like we might
be making some changes either way.) If you decide to keep it, I'd
like to suggest to invert the meaning of owned to indicate that
rw_printf, not the caller, owns the buffer. And maybe change the
type from size_t to bool or int.

Btw., with your patch, what will be the behavior of w_vasnprintf()
for the cases above?

Martin

> In the first
> call to _rw_bufcat() we would check (*pbufsize == maxsize), which would
> pass, and we would never allocate [according to comments above]? That is
> no good. So lets say we do allocate, but we use (*pbufsize == maxsize)
> as a flag to not deallocate. Both values are 0, so we don't deallocate.
> We then allocate space for the string and copy it in. The value of
> *buf.pbufsize is set to something above 12 [say 15], which will set
> bufsz to that same value.
> 
> The second call as pbufsize != 0 && *pbufsize == 15. We set buf.maxsize
> to 15 by the above code. Eventually we get back down to _rw_bufcat().
> There isn't enough space. We check (*pbufsize == maxsize), which is
> false, so we do deallocate. When we calculate the new buffer size, there
> is already a test to ensure we don't grow past maxsize. In this case, we
> would like to allocate a total of 27 bytes. But 27 is greater than
> maxsize, so we get a failure.
> 
>> Martin
>>


Mime
View raw message