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); >>> >>> // >>> } >>> >>> 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 >>