On Fri, 22 Feb 2008, Plüm, Rüdiger, VF-Group wrote:
In general, that patch looks truly suspicious since it seems to me
it's typecasting wildly and not even using its newly invented
MAX_APR_SIZE_T in all places, because (apr_size_t)(-1)
really is the
same thing, right?
No, MAX_APR_SIZE_T and (apr_size_t)(-1) might be different
depending on the
platform. MAX_APR_SIZE_T is ~(apr_size_t)(0).
Won't both be 0xff...ff as long as apr_size_t is unsigned (which it
should be)? If not, the code makes even less sense..
I thought the same so far. But there seem to be platforms that we support
where this is not the case. Don't ask which platforms these are. Somebody?
size_t must be unsigned for (apr_size_t)(-1) to work, or this code
will be rather bogus IMHO. Comparing the length to signed -1 doesn't
seem really productive...
Both casting signed -1 to unsigned and flipping the bits of 0 are
standard methods to get the max-value possible to store in a
variable...
As I have overcome my confusion regarding apr_off_t / apr_size_t I
hope to have a look into the problem and find a solution how to do
all the casting stuff correctly.
My tip would be: less casts. If they're needed they're usually a sign
of bad design or a thinko somewhere.
Meanwhile I tried to clean this up in trunk. Can you please try the
attached patch?
Keep in mind that MAX_APR_SIZE_T is not present in apr-util 1.2.x
and that you need to adjust this manually. Remote eyes welcome as
well.
I'm still not liking the casts and the mixed -1's, APR_SIZE_MAX and
MAX_APR_SIZE_T...
In any case, I'll be busy for most of this weekend so I probably won't
have time to try patches until monday...
Index: apr_brigade.c
===================================================================
--- apr_brigade.c (revision 630122)
+++ apr_brigade.c (working copy)
@@ -97,6 +97,7 @@
apr_bucket *e;
const char *s;
apr_size_t len;
+ apr_uint64_t point64;
apr_status_t rv;
if (point < 0) {
@@ -108,17 +109,25 @@
return APR_SUCCESS;
}
+ /*
+ * Try to reduce the following casting mess: We know that point will be
+ * larger equal 0 now and forever and thus that point (apr_off_t) and
+ * apr_size_t will fit into apr_uint64_t in any case.
+ */
+ point64 = (apr_uint64_t)point;
+
APR_BRIGADE_CHECK_CONSISTENCY(b);
for (e = APR_BRIGADE_FIRST(b);
e != APR_BRIGADE_SENTINEL(b);
e = APR_BUCKET_NEXT(e))
{
- /* For an unknown length bucket, while 'point' is beyond the possible
+ /* For an unknown length bucket, while 'point64' is beyond the possible
* size contained in apr_size_t, read and continue...
*/
- if ((e->length == (apr_size_t)(-1)) && (point > APR_SIZE_MAX)) {
- /* point is too far out to simply split this bucket,
+ if ((e->length == (apr_size_t)(-1))
+ && (point64 > (apr_uint64_t)APR_SIZE_MAX)) {
+ /* point64 is too far out to simply split this bucket,
* we must fix this bucket's size and keep going... */
rv = apr_bucket_read(e, &s, &len, APR_BLOCK_READ);
if (rv != APR_SUCCESS) {
@@ -126,14 +135,15 @@
return rv;
}
}
- else if (((apr_size_t)point < e->length) || (e->length ==
(apr_size_t)(-1))) {
- /* We already consumed buckets where point is beyond
+ else if ((point64 < (apr_uint64_t)e->length)
+ || (e->length == (apr_size_t)(-1))) {
+ /* We already consumed buckets where point64 is beyond
* our interest ( point > MAX_APR_SIZE_T ), above.
- * Here point falls between 0 and MAX_APR_SIZE_T
+ * Here point falls between 0 and MAX_APR_SIZE_T
* and is within this bucket, or this bucket's len
* is undefined, so now we are ready to split it.
* First try to split the bucket natively... */
- if ((rv = apr_bucket_split(e, (apr_size_t)point))
+ if ((rv = apr_bucket_split(e, (apr_size_t)point64))
!= APR_ENOTIMPL) {
*after_point = APR_BUCKET_NEXT(e);
return rv;
@@ -150,17 +160,17 @@
/* this assumes that len == e->length, which is okay because e
* might have been morphed by the apr_bucket_read() above, but
* if it was, the length would have been adjusted appropriately */
- if ((apr_size_t)point < e->length) {
+ if (point64 < (apr_uint64_t)e->length) {
rv = apr_bucket_split(e, (apr_size_t)point);
*after_point = APR_BUCKET_NEXT(e);
return rv;
}
}
- if (point == e->length) {
+ if (point64 == (apr_uint64_t)e->length) {
*after_point = APR_BUCKET_NEXT(e);
return APR_SUCCESS;
}
- point -= e->length;
+ point64 -= (apr_uint64_t)e->length;
}
*after_point = APR_BRIGADE_SENTINEL(b);
return APR_INCOMPLETE;
Regards
Rüdiger
/Nikke
--
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Niklas Edmundsson, Admin @ {acc,hpc2n}.umu.se | [EMAIL PROTECTED]
---------------------------------------------------------------------------
Captain, I sense millions of minds focused on my cleavage.
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
