On 1/08/2013 12:18 a.m., Kinkie wrote:
On Wed, Jul 31, 2013 at 2:53 AM, Alex Rousskov
<[email protected]> wrote:
On 07/30/2013 03:56 AM, Kinkie wrote:
lp:~squid/squid/stringng-cherrypick has the most uptodate code.
I did not finish the review before running out of time, but I found one
or two bugs in r12761 (in the code I have not looked at before). There
are also a few polishing issues:
Thanks.
typedef int32_t size_type;
I think we should make it unsigned. I agree with your earlier
observation that it is better to do that now rather than later or never.
Yes, it is agreed. It's however something I'd prefer to do as a
separate change, after everything else has been defined in order to
have a clear impact.
Yes and no. It will probably cause a lot of little changes all over the
classes involved. So doing it at a point of stability as a separate
commit to stringng is good but not a reason to
void reserveSpace(size_type minSpace) {reserveCapacity(length()+minSpace);}
As I mentioned before, this lacks overflow control if the sum of
length() and minSpace exceeds size_type maximum. Please add a check.
reserve* calls cow() which - if a resizing is needed - calls reAlloc
which throws SBufTooBigException if the requested size is too big.
Isn't that good enough?
Not if the math overflowed down to a smaller value before it even got
passed to reserveCapacity().
cow(minSpace+length());
Same here, in rawSpace().
BTW, please add SBuf::spaceSize() as a public method so that
optimization like the above will work. Otherwise, non-privileged users
will have to write
foobar(buf.rawSpace(minSize), minSize);
instead of getting an advantage of the actual space size being larger
than the requested minimum.
Done, but I'm quite wary of this: it implies a knowledge about
internal store positioning which I would prefer to hide from clients.
I think we may find a reason in future to extend this futther to allow
caller handling of the cases where realloc is not possible to the new
minSize.
But for now that should be enough.
if (sz >= static_cast<int>(store_->spaceSize()))
rawSpace(sz*2); // TODO: tune heuristics
I would use reserveSpace() here instead of ignoring rawSpace() result
because we know the buffer needs to be reallocated at this point, right?
Hand-unrolled code doesn't ignore that return value anymore (see below)
/* snprintf on Linux returns -1 on overflows */
...
if (sz >= static_cast<int>(store_->spaceSize()))
rawSpace(sz*2); // TODO: tune heuristics
else if (sz < 0) // output error in vsnprintf
throw TextException("output error in vsnprintf",__FILE__,
__LINE__);
If snprintf on Linux returns -1 on overflows, we should not throw when
sz is -1. We should grow the buffer instead, right?
Man snprintf(3) says
If an output error is encountered, a negative value is returned.
I guess the comment is broken.
vsnprintf is standard as of C99; it should be therefore quite
consistent in our build environment.
What other types of outt error than buffer overflow can occur?
Might be worth a little test to see what overflow produces exactly. And
I agree with Alex that growing is the better way to handle it, on the
proviso that we can reliably detect that overflow.
while (length() <= maxSize) {
sz = vsnprintf(bufEnd(), store_->spaceSize(), fmt, ap);
...
/* snprintf on FreeBSD returns at least free_space on overflows */
if (sz >= static_cast<int>(store_->spaceSize()))
rawSpace(sz*2); // TODO: tune heuristics
else if (sz < 0) // output error in vsnprintf
throw TextException("output error in vsnprintf",__FILE__,
__LINE__);
else
break;
}
Nothing in that loop grows length(), which is your guard against an
"infinite" loop on FreeBSD (at least). Can you think of a better loop guard?
This loop will be executed at most twice via break clause, as on the
second go we _know_ how much we have to write, the first attempt is .
I'm hand-unrolling it.
len_ += sz;
// TODO: this does NOT belong here, but to class-init or autoconf
/* on Linux and FreeBSD, '\0' is not counted in return value */
/* on XXX it might be counted */
/* check that '\0' is appended and not counted */
if (operator[](len_-1) == '\0') {
--sz;
--len_;
}
There is no guarantee that len_ is positive. We could start with a zero
len_ and an empty "" pattern would result in zero sz. The above code may
then access character at -1 offset of the raw storage array...
vprintf() is probably so slow that any code using it is not optimized.
We should use at(len_-1) instead here.
Ok.
This will go away by itself when we move to unsigned size_type.
/* check that '\0' is appended and not counted */
if (operator[](len_-1) == '\0') {
Perhaps this day was too long, but I do not think the above checks
whether \0 was appended but not counted. It checks whether \0 was
appended and counted. Which is probably what you want to check, so the
comment should be adjusted to remove the word "not".
You're right, the comment as-is muddies the waters. Reworded as "check
whether '\0' was appended and counted.
But in the end, as vsnprintf is a C99 standard (aand we require C99 +
now to compile, so that check is now probably redundant.
Amos
