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
