Re: 2.2 roadmap with respect to APR was Re: [NOTICE] CVS to SVN migration complete
William A. Rowe, Jr. wrote: At 01:29 PM 11/24/2004, Cliff Woolley wrote: On Wed, 24 Nov 2004, Justin Erenkrantz wrote: I'm sick of all talk and no action. We tried this last year when we were "almost" ready to branch APR 1.0 and all action on that front ceased entirely for a YEAR. This time it's one or the other. I'll wait 24 hours at most to hear opinions. Whichever route gets the most support wins. So far we have: trunk remains 1.1, 64-bit is sandboxed: jwoolley, jerenkrantz wrowe: (conditional on committing to head once it's reviewed, and branch 1.1 if we want to keep 1.1 alive.) That is fine by me. I agree that the extent of the changes will probably be significant so at least starting with a sandbox branch may be judicious, at least until we get a feel for the extent of the changes, and working on a branch may actually help accelerate the work. Allan
Re: 2.2 roadmap with respect to APR was Re: [NOTICE] CVS to SVN migration complete
man, how did I get so far behind on my email... I'd like to see us get this into httpd 2.2 for the reasons previously outlined and think we need to get the work underway as quickly as possible to determine how extensive the changes are going to be and how fast progress can be made. First order of business now that we are on SVN is to focus on the APR changes that are needed. It's not clear to me though, now that we have an APR 1.0 branch, is the trunk open for API-breaking changes or do we need a separate branch for that work? If we can make good progress towards a stable 64 bit APR 2.0 then moving httpd 2.1/2.2 to it could make sense. The question is whether there is enough feature freeze pressure to say that 64 bit does not warrant the wait... I'd say let's see if we can make some progress first. Any help that can be offered on this endeavour will be greatly appreciated! Allan >Bill Stoddard wrote: William A. Rowe, Jr. wrote: At 08:23 AM 11/20/2004, Jim Jagielski wrote: On Nov 20, 2004, at 12:03 AM, Justin Erenkrantz wrote: So, my opinion is that we let Allen branch apr off now and let him go at it at a measured pace, but we shouldn't intend to hold httpd 2.2 for that. -- justin +1. Of course, I am assuming that his 64bit fixes will likely break binary compatibility. It does - that's the rub. And, for 2.2, this was always the plan. And that's precisely the reason we should attack the 64 bit problem for 2.2. This will give the 2.2 series a much longer life than if we push off the 64 bit work to 2.4. Bill
Re: cvs commit: apr/memory/unix apr_pools.c
Joe Orton wrote: The fact that these casts are unnecessary can be tracked using comments, a list of issues in STATUS, or, not wishing to rock the boat, bugzilla. Using a macro just obfuscates the code. s/unnecessary/undesirable/ I hardly think it is more obfuscating that any macro might be considered to be. The only reasonable alternative in my opinion would be to add a comment line for each cast with some common grep-able string. But to me that is uglier.
Re: cvs commit: apr/memory/unix apr_pools.c
Joe Orton wrote: Why is this a macro? It's not like apr_uint32_t is a name which is going to change any time soon? It's a macro because we don't want to lose sight of the fact that we did something there that we utimately want to back out (i.e. in APR 2.0 when we can change the API). If we used apr_uint32_t it would be easy to lose track of these places - make sense? Allan
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;
Re: [PATCH] WIN64: apr_pools.c
I'm like Jeff, I won't lose any sleep if we just say that
64 bit APR 1.0 cannot handle very large memory objects and go
the path of validating restricting and casting. At least let's
pursue that path and see how ugly it gets.
Going with plan 'B'... I tend to think it would be useful to
use a #define for any 64bit warning-quieting casts so that they
can be easily identified & ripped out for APR 2.0 - comments?
Allan
Index: srclib/apr/include/apr.h.in
===
RCS file: /home/cvs/apr/include/apr.h.in,v
retrieving revision 1.137
diff -U3 -r1.137 apr.h.in
--- srclib/apr/include/apr.h.in 5 Jun 2004 11:52:43 - 1.137
+++ srclib/apr/include/apr.h.in 5 Oct 2004 17:56:20 -
@@ -266,6 +266,10 @@
/* Are we big endian? */
#define APR_IS_BIGENDIAN @bigendian@
+#define APR_INT_CASTint
+#define APR_UINT32_CAST apr_uint32_t
+#define APR_UINT32_MAX 0xUL
+
/* Mechanisms to properly type numeric literals */
@int64_literal@
@uint64_literal@
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 5 Oct 2004 17:56:20 -
@@ -330,6 +330,9 @@
#define APR_UINT64_T_HEX_FMT "llx"
#define APR_TIME_T_FMT APR_INT64_T_FMT
+#define APR_INT_CASTint
+#define APR_UINT32_CAST apr_uint32_t
+#define APR_UINT32_MAX 0xUL
#define APR_DWORD_MAX 0xUL/* 2^32*/
/** @} */
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 5 Oct 2004 17:56:20 -
@@ -342,6 +342,9 @@
#define APR_SIZEOF_VOIDP 4
#endif
+#define APR_INT_CASTint
+#define APR_UINT32_CAST apr_uint32_t
+#define APR_UINT32_MAX 0xUL
#define APR_DWORD_MAX 0xUL
/* XXX These simply don't belong here, perhaps in apr_portable.h
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 5 Oct 2004 17:56:20 -
@@ -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_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_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 *mem;
-apr_uint32_t free_index;
+apr_size_t free_index;
size = APR_ALIGN_DEFAULT(size);
active = pool->active;
@@ -620,7 +626,7 @@
free_index = (APR_ALIGN(active->endp - active->first_avail + 1,
BOUNDARY_SIZE) - BOUNDARY_SIZE) >> BOUNDARY_INDEX;
-active->free_index = free_index;
+active->free_index = (APR_UINT32_CAST)free_index;
node = active->next;
if (free_index >= node->free_index)
return mem;
@@ -877,7 +883,7 @@
apr_size_t cur_len, size;
char *strp;
apr_pool_t *pool;
-apr_uint32_t free_index;
+apr_size_t free_index;
pool = ps->pool;
active = ps->node;
@@ -907,7 +913,7 @@
free_index = (APR_ALIGN(active->endp - active->first_avail + 1,
BOUNDARY_SIZE) - BOUNDARY_SIZE) >>
BOUNDARY_INDEX;
-active->free_index = free_index;
+active->free_index = (APR_UINT32_CAST)free_index;
node = active->next;
if (free_index < node->free_index) {
do {
@@ -948,7 +954,7 @@
char *strp;
apr_size_t size;
apr_memnode_t *active, *node;
-
Re: cvs commit: apr/network_io/win32 sendrecv.c
William A. Rowe, Jr. wrote: At 12:21 PM 10/1/2004, Greg Marr wrote: #ifdef DWORD_MAX #define APR_DWORD_MAX DWORD_MAX #else #define APR_DWORD_MAX 0xUL #endif Defining DWORD_MAX at all could cause problems if it was defined by a later header file. ++1, this is the right solution, and infinitely more legible. Not wishing to flog a dead horse too long... if DWORD_MAX is picked up from the Windows headers it is 0x and loses the UL we dutifully added in the #else clause. My suggestion was to drop the #ifdef DWORD_MAX and simply have #define APR_DWORD_MAX 0xUL Allan
Re: [PATCH] WIN64: apr_pools.c
William A. Rowe, Jr. wrote: The only fix we could possibly adopt before then, outside of -validating- and restricting these to 32 bit quantities on those platforms with mismatches, is to -retract- the release of APR 1.0 on all platforms with 32bit int/64bit pointers. I'm like Jeff, I won't lose any sleep if we just say that 64 bit APR 1.0 cannot handle very large memory objects and go the path of validating restricting and casting. At least let's pursue that path and see how ugly it gets. Allan
Re: [PATCH] WIN64: apr_pools.c
Joe Orton wrote:
It is part of the API, it can't be changed like this in 1.x. You can't
use the apr_allocator_* API without using the apr_memnode_t structure,
The allocator API uses an incomplete type for apr_allocator_t and
in the same way should (actually it does) use an incomplete type
for apr_memnode_t. It just seems like an oversight that the definition
of apr_memnode_t was left in apr_allocator.h
so I don't see how it could be made private either.
This patch demonstates that it can be made private,
httpd builds cleanly against it. The only reason I
can see that this breaks the API is if someone were
doing strange and unnatural things outside of the
normal use of the API. Do we want to deny this change
because of that remote possibility?
Allan
Index: apr/include/apr_allocator.h
===
RCS file: /home/cvs/apr/include/apr_allocator.h,v
retrieving revision 1.19
diff -U3 -r1.19 apr_allocator.h
--- apr/include/apr_allocator.h 13 Feb 2004 09:38:28 - 1.19
+++ apr/include/apr_allocator.h 30 Sep 2004 15:09:58 -
@@ -41,27 +41,6 @@
/** the structure which holds information about the allocation */
typedef struct apr_memnode_t apr_memnode_t;
-/** basic memory node structure
- * @note The next, ref and first_avail fields are available for use by the
- * caller of apr_allocator_alloc(), the remaining fields are read-only.
- * The next field has to be used with caution and sensibly set when the
- * memnode is passed back to apr_allocator_free(). See
apr_allocator_free()
- * for details.
- * The ref and first_avail fields will be properly restored by
- * apr_allocator_free().
- */
-struct apr_memnode_t {
-apr_memnode_t *next;/**< next memnode */
-apr_memnode_t **ref;/**< reference to self */
-apr_uint32_t index; /**< size */
-apr_uint32_t free_index; /**< how much free */
-char *first_avail; /**< pointer to first free memory */
-char *endp;/**< pointer to end of free memory */
-};
-
-/** The base size of a memory node - aligned. */
-#define APR_MEMNODE_T_SIZE APR_ALIGN_DEFAULT(sizeof(apr_memnode_t))
-
/** Symbolic constants */
#define APR_ALLOCATOR_MAX_FREE_UNLIMITED 0
Index: 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
--- apr/memory/unix/apr_pools.c 17 Jun 2004 14:13:58 - 1.206
+++ apr/memory/unix/apr_pools.c 30 Sep 2004 15:09:58 -
@@ -21,7 +21,7 @@
#include "apr_strings.h"
#include "apr_general.h"
#include "apr_pools.h"
-#include "apr_allocator.h"
+#include "arch/unix/apr_arch_allocator.h"
#include "apr_lib.h"
#include "apr_thread_mutex.h"
#include "apr_hash.h"
@@ -63,9 +63,9 @@
*/
struct apr_allocator_t {
-apr_uint32_tmax_index;
-apr_uint32_tmax_free_index;
-apr_uint32_tcurrent_free_index;
+apr_size_t max_index;
+apr_size_t max_free_index;
+apr_size_t current_free_index;
#if APR_HAS_THREADS
apr_thread_mutex_t *mutex;
#endif /* APR_HAS_THREADS */
@@ -141,7 +141,7 @@
APR_DECLARE(void) apr_allocator_max_free_set(apr_allocator_t *allocator,
apr_size_t size)
{
-apr_uint32_t max_free_index;
+apr_size_t max_free_index;
#if APR_HAS_THREADS
apr_thread_mutex_t *mutex;
@@ -168,7 +168,7 @@
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_size_t i, index, max_index;
/* Round up the block size to the next boundary, but always
* allocate at least a certain size (MIN_ALLOC).
@@ -304,8 +304,8 @@
void allocator_free(apr_allocator_t *allocator, apr_memnode_t *node)
{
apr_memnode_t *next, *freelist = NULL;
-apr_uint32_t index, max_index;
-apr_uint32_t max_free_index, current_free_index;
+apr_size_t index, max_index;
+apr_size_t max_free_index, current_free_index;
#if APR_HAS_THREADS
if (allocator->mutex)
@@ -582,7 +582,7 @@
{
apr_memnode_t *active, *node;
void *mem;
-apr_uint32_t free_index;
+apr_size_t free_index;
size = APR_ALIGN_DEFAULT(size);
active = pool->active;
@@ -877,7 +877,7 @@
apr_size_t cur_len, size;
char *strp;
apr_pool_t *pool;
-apr_uint32_t free_index;
+apr_size_t free_index;
pool = ps->pool;
active = ps->node;
@@ -948,7 +948,7 @@
char *strp;
apr_size_t size;
apr_memnode_t *active, *node;
-apr_uint32_t free_index;
+apr_size_t free_index;
ps.node = active = pool->active;
ps.pool = pool;
Index: apr-util/buckets/apr_buckets_alloc.c
===
RCS file: /home/cvs/apr-util/buckets/apr_buckets_
[PATCH] WIN64: apr_pools.c
In order to eliminate the following warnings for Win64 builds the
appended patch is needed. However this changes fields in the apr_memnode_t
structure in apr/include/apr_allocator.h
Seems that this strucure really should be private and moved along
with struct apr_allocator_t to include/arch/unix/apr_arch_allocator.h
but either moving it or changing it may be construed as changing
the 1.0 API. Personally I don't think so but if anyone thinks
otherwise let's discuss.
Allan
---
apr_pools.c(154) : warning C4267: '=' : conversion from 'size_t' to
'apr_uint32_t', possible loss of data
apr_pools.c(183) : warning C4267: '=' : conversion from 'size_t' to
'apr_uint32_t', possible loss of data
apr_pools.c(621) : warning C4244: '=' : conversion from '__int64' to
'apr_uint32_t', possible loss of data
apr_pools.c(908) : warning C4244: '=' : conversion from '__int64' to
'apr_uint32_t', possible loss of data
apr_pools.c(1009) : warning C4244: '=' : conversion from '__int64' to
'apr_uint32_t', possible loss of data
Index: apr/include/apr_allocator.h
===
RCS file: /home/cvs/apr/include/apr_allocator.h,v
retrieving revision 1.19
diff -U3 -r1.19 apr_allocator.h
--- apr/include/apr_allocator.h 13 Feb 2004 09:38:28 - 1.19
+++ apr/include/apr_allocator.h 29 Sep 2004 20:29:37 -
@@ -53,8 +53,8 @@
struct apr_memnode_t {
apr_memnode_t *next;/**< next memnode */
apr_memnode_t **ref;/**< reference to self */
-apr_uint32_t index; /**< size */
-apr_uint32_t free_index; /**< how much free */
+apr_size_t index; /**< size */
+apr_size_t free_index; /**< how much free */
char *first_avail; /**< pointer to first free memory */
char *endp;/**< pointer to end of free memory */
};
Index: 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
--- apr/memory/unix/apr_pools.c 17 Jun 2004 14:13:58 - 1.206
+++ apr/memory/unix/apr_pools.c 29 Sep 2004 20:29:37 -
@@ -63,9 +63,9 @@
*/
struct apr_allocator_t {
-apr_uint32_tmax_index;
-apr_uint32_tmax_free_index;
-apr_uint32_tcurrent_free_index;
+apr_size_t max_index;
+apr_size_t max_free_index;
+apr_size_t current_free_index;
#if APR_HAS_THREADS
apr_thread_mutex_t *mutex;
#endif /* APR_HAS_THREADS */
@@ -141,7 +141,7 @@
APR_DECLARE(void) apr_allocator_max_free_set(apr_allocator_t *allocator,
apr_size_t size)
{
-apr_uint32_t max_free_index;
+apr_size_t max_free_index;
#if APR_HAS_THREADS
apr_thread_mutex_t *mutex;
@@ -168,7 +168,7 @@
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_size_t i, index, max_index;
/* Round up the block size to the next boundary, but always
* allocate at least a certain size (MIN_ALLOC).
@@ -304,8 +304,8 @@
void allocator_free(apr_allocator_t *allocator, apr_memnode_t *node)
{
apr_memnode_t *next, *freelist = NULL;
-apr_uint32_t index, max_index;
-apr_uint32_t max_free_index, current_free_index;
+apr_size_t index, max_index;
+apr_size_t max_free_index, current_free_index;
#if APR_HAS_THREADS
if (allocator->mutex)
@@ -582,7 +582,7 @@
{
apr_memnode_t *active, *node;
void *mem;
-apr_uint32_t free_index;
+apr_size_t free_index;
size = APR_ALIGN_DEFAULT(size);
active = pool->active;
@@ -877,7 +877,7 @@
apr_size_t cur_len, size;
char *strp;
apr_pool_t *pool;
-apr_uint32_t free_index;
+apr_size_t free_index;
pool = ps->pool;
active = ps->node;
@@ -948,7 +948,7 @@
char *strp;
apr_size_t size;
apr_memnode_t *active, *node;
-apr_uint32_t free_index;
+apr_size_t free_index;
ps.node = active = pool->active;
ps.pool = pool;
Re: cvs commit: apr/network_io/win32 sendrecv.c
William A. Rowe, Jr. wrote: #ifdef DWORD_MAX #define APR_DWORD_MAX DWORD_MAX #else #define DWORD_MAX 4294967295UL #endif it seems we forgot to correctly sign this constant Definitely should use correct sign, but since Windows DWORD_MAX is not defined as UL I'm inclined to just ignore DWORD_MAX, but use the possibly clearer hex notation for APR_DWORD_MAX thus #define APR_DWORD_MAX 0xUL Allan
Re: DWORD_MAX (was: cvs commit: apr/include apr.hnw)
Jean-Jacques Clar wrote: I will be more than happy to do so, but first will like to ask Allan to grant me permission to APRize the define in apr.hw, win32\sendrecv.c, and win32\readwrite.c. Yes please go ahead, I would do it myself but I'm on v-slow dial up over the weekend. If you don't get to it before Monday I will fix things then. Thanks, Allan
[Fwd: Re: cvs commit: apr/atomic/win32 apr_atomic.c]
meant to reply to [EMAIL PROTECTED] also Original Message Subject: Re: cvs commit: apr/atomic/win32 apr_atomic.c Date: Mon, 20 Sep 2004 18:42:24 -0400 From: Allan Edwards <[EMAIL PROTECTED]> To: Cliff Woolley <[EMAIL PROTECTED]> References: <[EMAIL PROTECTED]> <[EMAIL PROTECTED]> Doesn't this just mean that the definition of apr_atomic_win32_ptr_val_fn is wrong on win64? Wouldn't it be cleaner to redefine that on win64 instead of these two #if #else cases? The problem is with the use of a typedef to remap the function. It makes the compiler think there is an external function called InterlockedExchangeAdd when it really is an intrinsic function. So redefining for win64 does not work (in fact no remapping would be needed for win64 since it matches the typedef). Allan
Re: Windows 2003 IA64 builds?
Joe Orton wrote: w.r.t. common code, depends what the changes are; if there are places where an int is used to store the difference between pointers, it's probably a good and safe change to make that an apr_ssize_t instead. Yes, there are a few of those, they are the easy ones to fix :) w.r.t. warnings: yes, gcc is just less picky. But Win64 has a 32-bit long, right? That's correct, but that actually contributes to a fairly low % of warnings +rv = collapse_iovec((char**) &ptfb->Tail, (apr_size_t *)&ptfb->TailLength, if that's casting (int *) to (size_t *) that looks dangerous. Will take another look at that. I'm offline for the next 3 days but hope to pick it up again next Monday. Thanks Allan
Re: Windows 2003 IA64 builds?
Here's a snapshot of work in progress. This patch
is against HEAD but I'm actually developing on the
0.9 branch and with Visual Studio 6 + IA64 SDK
cross compile.
As I mentioned in my previous note there are a lot
of warnings thrown when compiling 64 bit and this
just hits the low lying fruit for /W2 compile and
eliminates a couple of obvious bugs.
What is a little puzzling is that most of the
remaining warnings are in common code, but it
appears that a Linux IA64 build is pretty clean
even with -Wall.
eg. even backing off to /W2 the Win 64 compile throws
__int64 to int mismatch warnings for pointer arithmetic
char *p1, *p2;
int diff = p2 -p1;
also lots of size_t mismatch warnings show up with /W3.
I'm wondering why gcc doesn't do the same since
int=4 ptr=8 bytes for both platforms, is gcc just not
as strict? And how to proceed, do we start fixing
these warnings for Win64 even though they may not
represent real life concerns in order to achieve a
warning free /W3 compile for Win 64?
Allan
-
Index: apr/file_io/win32/open.c
===
RCS file: /home/cvs/apr/file_io/win32/open.c,v
retrieving revision 1.121
diff -u -3 -r1.121 open.c
--- apr/file_io/win32/open.c13 Feb 2004 09:38:27 - 1.121
+++ apr/file_io/win32/open.c17 Aug 2004 20:59:19 -
@@ -47,7 +47,7 @@
* Note that the \\?\ form only works for local drive paths, and
* \\?\UNC\ is needed UNC paths.
*/
-int srcremains = strlen(srcstr) + 1;
+apr_size_t srcremains = strlen(srcstr) + 1;
apr_wchar_t *t = retstr;
apr_status_t rv;
@@ -101,7 +101,7 @@
* then transform \\'s back into /'s since the \\?\ form never
* allows '/' path seperators, and APR always uses '/'s.
*/
-int srcremains = wcslen(srcstr) + 1;
+apr_size_t srcremains = wcslen(srcstr) + 1;
apr_status_t rv;
char *t = retstr;
if (srcstr[0] == L'\\' && srcstr[1] == L'\\' &&
Index: apr/file_io/win32/readwrite.c
===
RCS file: /home/cvs/apr/file_io/win32/readwrite.c,v
retrieving revision 1.80
diff -u -3 -r1.80 readwrite.c
--- apr/file_io/win32/readwrite.c 13 Feb 2004 09:38:27 - 1.80
+++ apr/file_io/win32/readwrite.c 17 Aug 2004 20:59:19 -
@@ -27,8 +27,9 @@
* read_with_timeout()
* Uses async i/o to emulate unix non-blocking i/o with timeouts.
*/
-static apr_status_t read_with_timeout(apr_file_t *file, void *buf, apr_size_t
len, apr_size_t *nbytes)
+static apr_status_t read_with_timeout(apr_file_t *file, void *buf, apr_size_t
len_in, apr_size_t *nbytes)
{
+DWORD len = (DWORD)len_in;
apr_status_t rv;
*nbytes = 0;
@@ -68,7 +69,7 @@
file->pOverlapped->OffsetHigh = (DWORD)(file->filePtr >> 32);
}
-rv = ReadFile(file->filehand, buf, len, nbytes, file->pOverlapped);
+rv = ReadFile(file->filehand, buf, len, (LPDWORD)nbytes,
file->pOverlapped);
if (!rv) {
rv = apr_get_os_error();
@@ -85,7 +86,7 @@
switch (rv) {
case WAIT_OBJECT_0:
GetOverlappedResult(file->filehand, file->pOverlapped,
-nbytes, TRUE);
+(LPDWORD)nbytes, TRUE);
rv = APR_SUCCESS;
break;
case WAIT_TIMEOUT:
@@ -309,7 +310,7 @@
rv = WaitForSingleObject(thefile->pOverlapped->hEvent,
INFINITE);
switch (rv) {
case WAIT_OBJECT_0:
-GetOverlappedResult(thefile->filehand,
thefile->pOverlapped, nbytes, TRUE);
+GetOverlappedResult(thefile->filehand,
thefile->pOverlapped, (LPDWORD)nbytes, TRUE);
rv = APR_SUCCESS;
break;
case WAIT_TIMEOUT:
@@ -343,7 +344,7 @@
{
apr_status_t rv = APR_SUCCESS;
apr_size_t i;
-DWORD bwrote = 0;
+size_t bwrote = 0;
char *buf;
*nbytes = 0;
@@ -361,7 +362,7 @@
APR_DECLARE(apr_status_t) apr_file_putc(char ch, apr_file_t *thefile)
{
-DWORD len = 1;
+apr_size_t len = 1;
return apr_file_write(thefile, &ch, &len);
}
@@ -375,7 +376,7 @@
APR_DECLARE(apr_status_t) apr_file_getc(char *ch, apr_file_t *thefile)
{
apr_status_t rc;
-int bread;
+apr_size_t bread;
bread = 1;
rc = apr_file_read(thefile, ch, &bread);
@@ -393,7 +394,7 @@
APR_DECLARE(apr_status_t) apr_file_puts(const char *str, apr_file_t *thefile)
{
-DWORD len = strlen(str);
+apr_size_t len = strlen(str);
return apr_file_write(thefile, str, &len);
}
Index: apr/network_io/win32/sendrecv.c
===
RCS file: /home/cvs/apr/network_io/win32/sendrecv.c,v
retrieving revision 1.67
diff -u -3 -r1.67 sendrecv.c
--- apr/network_io/win32/sendrecv.c 22 Jul 2004 01:48:34 -
Windows 2003 IA64 builds?
Has anyone attempted to build apr/apache for Windows 2003 Itanium? First attempts to build using 2003 SDK and Visual Studio 6.0 gave a surprising number of type mismatch warnings, about 100 for apr/aprutil and around 80 for libhttpd. Need to comb through these to see how many are real issues, but was wondering if this is pioneering work or has anyone ventured down this path ahead of me? Allan
Re: [PATCH] some progress with apr_socket_data_get()/apr_socket_data_set()
Justin Erenkrantz wrote: Store that key in the apr_socket_data? Another cheesy idea may be to use a linked list with the key in the structure. I'm just really thinking that for the general case, a key/value backing (like a hash) is overkill. I'm just not clear this is going to be more than 2 or 3 elements at most... Jeff's proposal is no more heavyweight than what we had before and I think we definitely should support more than 1 element so I'm +1 for the patch (of course I'm not against any optimizations should anyone be inclined to code them :) Allan
Re: windows nagle settings
On all Win32 socket layers, when you enable APR_TCP_NODELAY on a
listening socket that setting ___ ("WILL" or "WILL NOT") then be
enabled automatically for associated connected sockets.
On all Win32 socket layers, when you enable APR_SO_NOBLOCK on a
listening socket that setting _ ("WILL" or "WILL NOT") then
be enabled automatically for associated connected sockets.
MS doc for accept states that the accept socket "..has the same properties
as.." the listen socket. An AcceptEx socket will inherit the properties of
the listen socket when setsockopt(SO_UPATE_CONTEXT) is called (which we do).
Setting them to 1 when
some Windows socket layers don't work that way it will break the
application.
FIOBIO and TCP_NODELAY are part of the Winsock2 API so I see no reason
to exclude them.
[snip]
As far as the ap_sock_disable_nagle() calls... That doesn't seem to be
implemented in the most clear manner. The calls Apache makes are
dependent on APR_TCP_NODELAY_INHERITED and
AP_MPM_DISABLE_NAGLE_ACCEPTED_SOCK. I would think that it should work
this way:
Agreed the original code is somewhat confusing.
a) when setting up listening socket, unconditionally call
ap_sock_disable_nagle()
b) when setting up a newly-connected socket, unconditionally call
ap_sock_disable_nagle()
Very little extra overhead will be incurred since the APR setting of
APR_TCP_NODELAY_INHERITED will determine whether or not a syscall is
used when ap_sock_disable_nagle() calls
apr_socket_option_set(APR_TCP_NODELAY) on the newly-connected socket.
mmm, that's not what happens on Windows. The Accept apr_socket_t doesn't
pick up APR_TCP_NODELAY from the Listen socket and we incur the extra
syscall for every newly connected socket. At the moment I'm not sure
how hard it would be to fix that.
windows nagle settings
Anyone know why APR_TCP_NODELAY_INHERITED is not defined in apr.hw?
Or why ap_sock_disable_nagle is not defined for Windows (looks
like this was omitted unintentionally).
Windows is exhibiting predicatable performance problems
because nagle is enabled. The following patch fixes it.
Also, it's not part of the nagle problem but it looks to me
like APR_O_NONBLOCK_INHERITED was inadvertently left out of
apr.hw also.
Allan
---
Index: server/mpm_common.c
===
RCS file: /home/cvs/httpd-2.0/server/mpm_common.c,v
retrieving revision 1.105
diff -u -d -b -r1.105 mpm_common.c
--- server/mpm_common.c 20 Mar 2003 21:50:40 - 1.105
+++ server/mpm_common.c 9 Apr 2003 20:13:41 -
@@ -302,7 +302,7 @@
}
#endif /* AP_MPM_WANT_PROCESS_CHILD_STATUS */
-#if defined(TCP_NODELAY) && !defined(MPE) && !defined(TPF) && !defined(WIN32)
+#if defined(TCP_NODELAY) && !defined(MPE) && !defined(TPF)
void ap_sock_disable_nagle(apr_socket_t *s)
{
/* The Nagle algorithm says that we should delay sending partial
Index: srclib/apr/include/apr.hw
===
RCS file: /home/cvs/apr/include/apr.hw,v
retrieving revision 1.113
diff -u -d -b -r1.113 apr.hw
--- srclib/apr/include/apr.hw 23 Mar 2003 02:25:02 - 1.113
+++ srclib/apr/include/apr.hw 9 Apr 2003 20:13:41 -
@@ -342,6 +342,14 @@
*/
#define APR_CHARSET_EBCDIC0
+/* Is the TCP_NODELAY socket option inherited from listening sockets?
+*/
+#define APR_TCP_NODELAY_INHERITED 1
+
+/* Is the O_NONBLOCK flag inherited from listening sockets?
+*/
+#define APR_O_NONBLOCK_INHERITED 1
+
/* Typedefs that APR needs. */
typedef unsigned char apr_byte_t;
Re: 0.9.2-rc1 outstanding issues and win32 debug symbols
I think having the .dbg symbols should be quite enough for a normal httpd installation, so I think fixing the timestamp problem isn't that important for 0.9.2/2.0.45. As I said to Bill in another note, the timestamp difference does not seem to cause any problems for me. Using windbg to examine a user.dmp I can point it to the pdb files & source tree, and display the stack trace and variables with no problem. One point I meant to make... can we make the .dbg simplified symbols -p(rivate) such that they are even smaller? With the full blown .pdb's available to all who are interested, I don't see that as a shortcoming. +1 If we can really do without the map files once the .dbg's are available, then I see nothing wrong with killing them. +1 Allan
building Release symbols for Win32
Unless someone knows a trick that I'm not aware of debugging Win32 crash dumps (DrWatson .dmp files) can be a real pain unless you have symbols because Frame Pointer Omission records make it hard to construct the call stack from a dump. Having symbols that match the binary build handy when you examine the dump avoids this problem. Does anyone have a reason we shouldn't build .pdb files for Release builds (we already do it for Debug builds). This only increases the size of the DLL by a few bytes and has negligible impact on performance. The plan would be to update the Release build for every .dsp in Apache.dsw: - in compiler settings generate "Program Database" Debug Info (/Zi) - in the Linker Debug settings generate "Microsoft Format" debug Info (/debug) The other thing we would need to do would be to save away the .pdb files when the binary install package is built (I don't think we'd want to package them in the install image since the files are quite large). Allan Edwards
RE: APR Bug or misunderstanding ?
> H. I have just looked at the code in more detail, we have already
> an "if (alloc_mutex {" check both before locking and unlocking the
> mutex. This means that we shouldn't be seg faulting. When did you grab
> your copy of APR? Do you have the if calls in your copy?
Part of the problem I think is that apr_destroy_lock does
not null out the alloc_mutex pointer so that the if(alloc_mutex)
test in apr_destroy_pool is true and we trap (at least on Windows).
Passing in a apr_lock_t** intro apr_destroy_lock and nulling the pointer
might fix the problem.
I had noticed this problem in Apache this afternoon but hadn't
had time to investigate. BTW I am having chronic mail delivery problems
so if this has already been answered please ignore.
Allan
RE: cvs commit: apr/lib apr_pools.c
> > I assume the correct fix is to pass in a pool parameter to ap_is_rdirectory. > > Similar situation seems to exist with apr_stat being passed a NULL pool > > by ap_is_directory. I'll go ahead and fix these. > > I disagree that this is the correct solution. APR relies too heavily on > pools right now, and I think this is causing some of our memory leak. For > example, why does apr_get_oslevel take a pool as an argument? It doesn't I can't answer for apr_get_oslevel, maybe the pool should go away in that case. However both apr_stat and apr_lstat need the pool on Windows because of the utf8_to_unicode_path call which does use it. > use it ever. That pool should go away. Once that is done, the pool can > be removed from apr_stat and apr_lstat. As far as I can see, the pools > aren't used there either. > Allan
RE: cvs commit: apr/lib apr_pools.c
> -Original Message- > From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] > Sent: Monday, January 01, 2001 8:33 PM > To: [EMAIL PROTECTED] > Subject: cvs commit: apr/lib apr_pools.c > > > rbb 01/01/01 17:33:05 > > Modified:.CHANGES >lib apr_pools.c > Log: > Remove the ability to allocate out of a NULL pool. This was always a bad > idea, because it opened up memory leaks. It is very likely that this will > expose some seg faults This is indeed true, now that Windows httpd-2.0 is building again it has exposed a couple of problems. In ap_is_rdirectory, apr_lstat is passed a NULL pool pointer which eventually causes apr_palloc to segfault. I assume the correct fix is to pass in a pool parameter to ap_is_rdirectory. Similar situation seems to exist with apr_stat being passed a NULL pool by ap_is_directory. I'll go ahead and fix these. Allan
FW: cvs commit: apr-util libaprutil.def
whoops, this didn't get sent to the list -Original Message- From: Allan Edwards [mailto:[EMAIL PROTECTED] Sent: Thursday, January 04, 2001 4:25 PM To: Greg Stein Subject: RE: cvs commit: apr-util libaprutil.def > > Why is that showing up in two export files? Shouldn't it live in only one > library? > There are actually a few (haven't determined exactly how many) duplicate exports in libhttpd.def and libaprutil.def - It had struck me as rather odd when I made the name change and intended to go back and investigate. Having had chance to dig further I think the problem is solved (at least for ap_current_hooking_module) by removing it from libhttpd.def and adding a dependency on libaprutil in the mod_dav project. I'll go ahead and make that fix and remove any other duplicates I find in libhttpd.def BTW. the other question that crossed my mind was why do we need .def file entry anyway, we are defining APU_DECLARE_EXPORT which should translate to __declspec(dllexport) and according to MS doesn't require a .def file. (for some reason however it will not currently compile unless we do have the .def file entry) Thanks, Allan
