Re: [PATCH] fix apr_random
On Sat, Aug 28, 2004 at 04:43:55PM +0200, Mladen Turk wrote: Hi, This patch fixes the testrandom on WIN32. Found by try-error-fix, knowing that the allocated data on WIN32 is garbage, and on unixes it might be zeroed, and that could be the only cause why the test is passing on my FreeBDS, but fails on WIN32. I checked this in, the tests were failing consistently on Unix when using ElectricFence in non-zero-fill mode too. Thanks for tracking it down. joe
Re: [PATCH] apr_file_writev() on UNIX
Lets agree there is a bug in the way the function is used in mod_disk_cache. That will be looked at. As far as not having a bug in the !HAS_WRITEV implementation, I disagree. The current implementation does not havea singlechance of being successful if there is more than 1 vector. This does not make sense to me. Let assume the write part is successful, the function will return apr_success but has completely failed to his task. In the header file there is a remark saying that apr_file_writev is available even if the underlying os doesn't provide writev(). Why providing something that it is going to fail every single time? Is the current implementation meets the expectation of others and I am the only one that see something wrong here? Thanks, Jean-Jacques Joe Orton [EMAIL PROTECTED] 10/05/04 2:40 PM On Tue, Oct 05, 2004 at 11:23:35AM -0600, Jean-Jacques Clar wrote: [EMAIL PROTECTED] 10/05/04 12:16 AM On Mon, Oct 04, 2004 at 06:11:57PM -0600, Jean-Jacques Clar wrote: If HAS_WRITEV is not defined the current code will just push the first vector to the target. The function should write all the vectors or none. There is no guarantee that writev(2) writes all the vectors, it's allowed to return short just like write(2) is, so why should apr_file_writev() give such a guarantee only for the non-writev-based implementation? If HAS_WRITEV is not defined, there is a bug inside apr_file_writev(). Only the first vector will be written to the file. How do you think that bug should be fixed?I don't think that's a bug. apr_file_writev() should be allowed toreturn short, as the writev()-based implementation may already do thistoo. So the !HAS_WRITEV implementation is valid. apr_file_writev() is used multiple times without alternatives in mod_disk_cache.c and the current implementation is failing without informing the caller or any failure if writev() is not available.Right, the bug is in how the function is used. It's the same as callingapr_file_write() and presuming it won't return short. The fix is tocall apr_file_write_full(), in that case. In this case, there is noapr_file_writev_full(), so that should be fixed.joe
Re: [PATCH] apr_file_writev() on UNIX
On Wed, 6 Oct 2004, Jean-Jacques Clar wrote: Why providing something that it is going to fail every single time? That was my original thought. I wouldn't mind seeing both the caller as well as the !HAS_WRITEV cases fixed (or fixed if you prefer) for this reason. --Cliff
Re: [PATCH] apr_file_writev() on UNIX
At 11:12 AM 10/6/2004, you wrote: As far as not having a bug in the !HAS_WRITEV implementation, I disagree. The current implementation does not have a single chance of being successful if there is more than 1 vector. This does not make sense to me. Let assume the write part is successful, the function will return apr_success but has completely failed to his task. That's only partially true - we should never return for a blocking socket writev, only for a nonblocking one when the socket would block on the remaining vector(s). In the header file there is a remark saying that apr_file_writev is available even if the underlying os doesn't provide writev(). Why providing something that it is going to fail every single time? Does writev() return *success* on a partial-write? I very much doubt it (EINTR, EWOULDBLOCK or EAGAIN is what I would expect.) The performance of the existing code is obviously sub-par, and we should loop through all the vectors until we get a hiccup. At that point the user can retry the remaining vectors. I should make a point that it seems 'wrong' we implemented this lie - there is another property of writev() that we've ignored... writev() will gather up the vectors into a single datagram, even when nagle is off. So this implementation leaves us potentially sending dozens of datagrams for a few dozen very small buffers. It seems a true implementation would use it's own buffer to concat all of the smaller fragments into a single packet - the pain in cpu is nominal compared to the pain of multiple datagram processing. Bill
Re: [PATCH] apr_file_writev() on UNIX
On Wed, Oct 06, 2004 at 10:12:33AM -0600, Jean-Jacques Clar wrote: Lets agree there is a bug in the way the function is used in mod_disk_cache. That will be looked at. As far as not having a bug in the !HAS_WRITEV implementation, I disagree. The current implementation does not have a single chance of being successful if there is more than 1 vector. This does not make sense to me. Let assume the write part is successful, the function will return apr_success but has completely failed to his task. You're missing the point. It is valid for apr_file_writev() to return APR_SUCCESS and update *nbytes to less than the total number of bytes in the vector. The writev()-based implementation *may* do that. The non-writev-based implementation *will* do it, if passed a vector of more than one buffer. But the caller must cope with it either way. joe
Re: [PATCH] WIN64: apr_pools.c
Jeff Trawick wrote:
APR_32BIT_TRUNC_CAST
defined only in private headers only so that
a) we don't add yet more symbols to API
b) we continually identify where our API is broken w.r.t. handling
quantities properly in a 64-bit world
I believe this addresses the issues
Allan
-
Index: srclib/apr/include/apr.hnw
===
RCS file: /home/cvs/apr/include/apr.hnw,v
retrieving revision 1.49
diff -U3 -r1.49 apr.hnw
--- srclib/apr/include/apr.hnw 1 Oct 2004 19:24:03 - 1.49
+++ srclib/apr/include/apr.hnw 6 Oct 2004 18:48:51 -
@@ -330,7 +330,6 @@
#define APR_UINT64_T_HEX_FMT llx
#define APR_TIME_T_FMT APR_INT64_T_FMT
-#define APR_DWORD_MAX 0xUL/* 2^32*/
/** @} */
#ifdef __cplusplus
Index: srclib/apr/include/apr.hw
===
RCS file: /home/cvs/apr/include/apr.hw,v
retrieving revision 1.127
diff -U3 -r1.127 apr.hw
--- srclib/apr/include/apr.hw 1 Oct 2004 19:24:03 - 1.127
+++ srclib/apr/include/apr.hw 6 Oct 2004 18:48:51 -
@@ -342,8 +342,6 @@
#define APR_SIZEOF_VOIDP 4
#endif
-#define APR_DWORD_MAX 0xUL
-
/* XXX These simply don't belong here, perhaps in apr_portable.h
* based on some APR_HAVE_PID/GID/UID?
*/
Index: srclib/apr/include/arch/apr_private_common.h
===
RCS file: /home/cvs/apr/include/arch/apr_private_common.h,v
retrieving revision 1.2
diff -U3 -r1.2 apr_private_common.h
--- srclib/apr/include/arch/apr_private_common.h13 Feb 2004 09:38:29
- 1.2
+++ srclib/apr/include/arch/apr_private_common.h6 Oct 2004 18:48:51
-
@@ -33,4 +33,9 @@
char separator,
apr_pool_t *p);
+/* temporary defines to handle 64bit compile mismatches */
+#define APR_INT_TRUNC_CASTint
+#define APR_UINT32_TRUNC_CAST apr_uint32_t
+#define APR_UINT32_MAX0xUL
+
#endif /*APR_PRIVATE_COMMON_H*/
Index: srclib/apr/include/arch/netware/apr_private.h
===
RCS file: /home/cvs/apr/include/arch/netware/apr_private.h,v
retrieving revision 1.25
diff -U3 -r1.25 apr_private.h
--- srclib/apr/include/arch/netware/apr_private.h 6 Jul 2004 22:40:49
- 1.25
+++ srclib/apr/include/arch/netware/apr_private.h 6 Oct 2004 18:48:51
-
@@ -172,6 +172,9 @@
#define APR_OFF_T_STRFN strtol
#endif
+/* used to check DWORD overflow for 64bit compiles */
+#define APR_DWORD_MAX 0xUL
+
/*
* Include common private declarations.
*/
Index: srclib/apr/include/arch/win32/apr_private.h
===
RCS file: /home/cvs/apr/include/arch/win32/apr_private.h,v
retrieving revision 1.38
diff -U3 -r1.38 apr_private.h
--- srclib/apr/include/arch/win32/apr_private.h 4 Jun 2004 23:26:02 -
1.38
+++ srclib/apr/include/arch/win32/apr_private.h 6 Oct 2004 18:48:52 -
@@ -158,6 +158,9 @@
#define APR_OFF_T_STRFN strtoi
#endif
+/* used to check for DWORD overflow in 64bit compiles */
+#define APR_DWORD_MAX 0xUL
+
/*
* Include common private declarations.
*/
Index: srclib/apr/memory/unix/apr_pools.c
===
RCS file: /home/cvs/apr/memory/unix/apr_pools.c,v
retrieving revision 1.206
diff -U3 -r1.206 apr_pools.c
--- srclib/apr/memory/unix/apr_pools.c 17 Jun 2004 14:13:58 - 1.206
+++ srclib/apr/memory/unix/apr_pools.c 6 Oct 2004 18:48:53 -
@@ -139,9 +139,10 @@
}
APR_DECLARE(void) apr_allocator_max_free_set(apr_allocator_t *allocator,
- apr_size_t size)
+ apr_size_t in_size)
{
apr_uint32_t max_free_index;
+apr_uint32_t size = (APR_UINT32_TRUNC_CAST)in_size;
#if APR_HAS_THREADS
apr_thread_mutex_t *mutex;
@@ -168,7 +169,8 @@
apr_memnode_t *allocator_alloc(apr_allocator_t *allocator, apr_size_t size)
{
apr_memnode_t *node, **ref;
-apr_uint32_t i, index, max_index;
+apr_uint32_t max_index;
+apr_size_t i, index;
/* Round up the block size to the next boundary, but always
* allocate at least a certain size (MIN_ALLOC).
@@ -181,6 +183,10 @@
* dividing its size by the boundary size
*/
index = (size BOUNDARY_INDEX) - 1;
+
+if (index APR_UINT32_MAX) {
+return NULL;
+}
/* First see if there are any nodes in the area we know
* our node will fit into.
@@ -293,7 +299,7 @@
return NULL;
node-next = NULL;
-node-index = index;
+node-index = (APR_UINT32_TRUNC_CAST)index;
node-first_avail = (char *)node + APR_MEMNODE_T_SIZE;
node-endp = (char *)node + size;
@@ -582,7 +588,7 @@
{
apr_memnode_t *active, *node;
void
Re: [PATCH] apr_file_writev() on UNIX
Joe Orton [EMAIL PROTECTED] 10/06/04 12:46 PM On Wed, Oct 06, 2004 at 10:12:33AM -0600, Jean-Jacques Clar wrote: As far as not having a bug in the !HAS_WRITEV implementation, I disagree. The current implementation does not have a single chance of being successful if there is more than 1 vector. This does not make sense to me. Let assume the write part is successful, the function will return apr_success but has completely failed to his task.You're missing the point. It is valid for apr_file_writev() to returnAPR_SUCCESS and update *nbytes to less than the total number of bytes inthe vector. The writev()-based implementation *may* do that. Thenon-writev-based implementation *will* do it, if passed a vector of morethan one buffer. But the caller must cope with it either way.Joe, I clearly understand "your" point, but could I disagree with it? I have a problem with the *will* fail. Both option should have a *may* be successful. I don't think our views on that point are reconcilable, but maybe others could share whatever they think the current way is right or wrong. Regards, Jean-Jacques
