You are viewing a plain text version of this content. The canonical link for it is here.
Posted to apreq-dev@httpd.apache.org by Randy Kobes <ra...@theoryx5.uwinnipeg.ca> on 2005/02/08 08:06:35 UTC
[multi-env] t/parsers.c test on Win32
With the current svn multi-env branch, on Win32, the
Parsers test of testall fails:
1) locate_default_parsers: expected pointer <00403740>,
but was <10004270>
It doesn't matter in this failure of the test if I use just
one of any of URL_ENCTYPE, MFD_ENCTYPE, or MR_ENCTYPE.
I'm probably not understanding something here, but in
locate_default_parsers() of t/parsers.c, there is the
initial call
apreq_register_parser(NULL, NULL);
In apreq_register_parser(const char *enctype, ...) of
src/apreq_parsers.c, however, early on there's
if (enctype == NULL)
return APR_EINVAL;
I may be missing something, but does that mean that
apreq_register_parser(NULL, NULL) effectively returns
without doing anything?
I tried the following for t/parsers.c:
======================================================
Index: t/parsers.c
===================================================================
--- t/parsers.c (revision 152645)
+++ t/parsers.c (working copy)
@@ -109,6 +109,10 @@
/* initialize default-parser hash */
apreq_register_parser(NULL, NULL);
+ apreq_register_parser(URL_ENCTYPE, apreq_parse_urlencoded);
+ apreq_register_parser(MFD_ENCTYPE, apreq_parse_multipart);
+ apreq_register_parser(MR_ENCTYPE, apreq_parse_multipart);
+
f = apreq_parser(URL_ENCTYPE);
CuAssertPtrNotNull(tc, f);
================================================================
and got all the tests to pass. However, I don't understand
why - I thought this was done in apreq_initialize(),
which is called by testall.c (and the status checked for
success).
--
best regards,
randy
Re: [multi-env] t/parsers.c test on Win32
Posted by Max Kellermann <ma...@duempel.org>.
On 2005/02/08 16:34, Joe Schaefer <jo...@sunstarsys.com> wrote:
> Randy Kobes <ra...@theoryx5.uwinnipeg.ca> writes:
>
> [...]
>
> > I may be missing something, but does that mean that
> > apreq_register_parser(NULL, NULL) effectively returns
> > without doing anything?
>
> You are correct, I think. It should be safe to remove
> the "apreq_register_parser(NULL, NULL)" call.
Better say "replace that apreq_register_parser() call by
apreq_initialize()".
I forgot to replace that in my patch.
btw. apreq_register_parsers returns APR_EINVAL if you specify
enctype==NULL.
Max
Re: [multi-env] t/parsers.c test on Win32
Posted by Joe Schaefer <jo...@sunstarsys.com>.
Randy Kobes <ra...@theoryx5.uwinnipeg.ca> writes:
[...]
>> But we need to check the return values of the
>> apreq_register_parser calls, especially in apreq_initialize, because
>> apr threads may not be reliable on Win32 (what's your apr version btw)?
>
> It's 0.9.5, from Apache/2.0.52.
Ok, I doubt it's an apr bug then. It'd help to know
the latest revision of the multi-env branch which
passes for you on Win32. This particular test is pretty
self-contained, so I'm optimistic about locating the
particular change which broke it.
--
Joe Schaefer
Re: [multi-env] t/parsers.c test on Win32
Posted by Randy Kobes <ra...@theoryx5.uwinnipeg.ca>.
On Tue, 8 Feb 2005, Joe Schaefer wrote:
> Randy Kobes <ra...@theoryx5.uwinnipeg.ca> writes:
>
> [...]
>
> > I may be missing something, but does that mean that
> > apreq_register_parser(NULL, NULL) effectively returns
> > without doing anything?
>
> You are correct, I think. It should be safe to remove
> the "apreq_register_parser(NULL, NULL)" call.
>
> > I tried the following for t/parsers.c:
> > ======================================================
> > Index: t/parsers.c
> > ===================================================================
> > --- t/parsers.c (revision 152645)
> > +++ t/parsers.c (working copy)
> > @@ -109,6 +109,10 @@
> >
> > /* initialize default-parser hash */
> > apreq_register_parser(NULL, NULL);
> > + apreq_register_parser(URL_ENCTYPE, apreq_parse_urlencoded);
> > + apreq_register_parser(MFD_ENCTYPE, apreq_parse_multipart);
> > + apreq_register_parser(MR_ENCTYPE, apreq_parse_multipart);
> > +
> >
> > f = apreq_parser(URL_ENCTYPE);
> > CuAssertPtrNotNull(tc, f);
> > ================================================================
> > and got all the tests to pass. However, I don't understand
> > why - I thought this was done in apreq_initialize(),
> > which is called by testall.c (and the status checked for
> > success).
>
> But we need to check the return values of the apreq_register_parser
> calls, especially in apreq_initialize, because apr threads may
> not be reliable on Win32 (what's your apr version btw)?
It's 0.9.5, from Apache/2.0.52.
> I'd start by patching apreq_initialize and seeing what
> happens from there. If we can't identify the bug,
> lets consider either removing the new thread-locking
> stuff or determining which libapr version has reliable
> behavior.
I tried putting in some additional checks at various
stages in src/apreq_parsers.c:
=======================================================
Index: apreq_parsers.c
===================================================================
--- apreq_parsers.c (revision 152979)
+++ apreq_parsers.c (working copy)
@@ -129,12 +129,21 @@
return status;
default_parsers = apr_hash_make(default_parser_pool);
+ if (default_parsers == NULL)
+ return APR_EGENERAL;
- apreq_register_parser("application/x-www-form-urlencoded",
- apreq_parse_urlencoded);
- apreq_register_parser("multipart/form-data", apreq_parse_multipart);
- apreq_register_parser("multipart/related", apreq_parse_multipart);
-
+ status = apreq_register_parser("application/x-www-form-urlencoded",
+ apreq_parse_urlencoded);
+ if (status != APR_SUCCESS)
+ return status;
+ status = apreq_register_parser("multipart/form-data",
+ apreq_parse_multipart);
+ if (status != APR_SUCCESS)
+ return status;
+ status = apreq_register_parser("multipart/related",
+ apreq_parse_multipart);
+ if (status != APR_SUCCESS)
+ return status;
return APR_SUCCESS;
}
@@ -165,7 +174,9 @@
apr_hash_set(default_parsers, apr_pstrdup(default_parser_pool, enctype),
APR_HASH_KEY_STRING, f);
- apr_thread_rwlock_unlock(default_parsers_lock);
+ status = apr_thread_rwlock_unlock(default_parsers_lock);
+ if (status != APR_SUCCESS)
+ return status;
return APR_SUCCESS;
}
@@ -187,7 +198,9 @@
f = apr_hash_get(default_parsers, enctype, tlen);
- apr_thread_rwlock_unlock(default_parsers_lock);
+ status = apr_thread_rwlock_unlock(default_parsers_lock);
+ if (status != APR_SUCCESS)
+ return NULL;
if (f != NULL)
return *f;
================================================================
and then called apreq_initialize() from t/parsers.c (and
subsequently taking it out from testall.c):
===============================================================
Index: ../t/parsers.c
===================================================================
--- ../t/parsers.c (revision 152979)
+++ ../t/parsers.c (working copy)
@@ -106,9 +106,9 @@
static void locate_default_parsers(CuTest *tc)
{
apreq_parser_function_t f;
-
- /* initialize default-parser hash */
- apreq_register_parser(NULL, NULL);
+ apr_status_t status;
+ status = apreq_initialize(p);
+ CuAssertIntEquals(tc, status, APR_SUCCESS);
f = apreq_parser(URL_ENCTYPE);
CuAssertPtrNotNull(tc, f);
=======================================================
It still fails in the Parsers test:
1) locate_default_parsers: expected pointer <0040372A>
but was <10004280>
However, with these checks, apreq_initialize() was
APR_SUCCESS, and f != NULL.
--
best regards,
randy
Re: [multi-env] t/parsers.c test on Win32
Posted by Joe Schaefer <jo...@sunstarsys.com>.
Randy Kobes <ra...@theoryx5.uwinnipeg.ca> writes:
[...]
> I may be missing something, but does that mean that
> apreq_register_parser(NULL, NULL) effectively returns
> without doing anything?
You are correct, I think. It should be safe to remove
the "apreq_register_parser(NULL, NULL)" call.
> I tried the following for t/parsers.c:
> ======================================================
> Index: t/parsers.c
> ===================================================================
> --- t/parsers.c (revision 152645)
> +++ t/parsers.c (working copy)
> @@ -109,6 +109,10 @@
>
> /* initialize default-parser hash */
> apreq_register_parser(NULL, NULL);
> + apreq_register_parser(URL_ENCTYPE, apreq_parse_urlencoded);
> + apreq_register_parser(MFD_ENCTYPE, apreq_parse_multipart);
> + apreq_register_parser(MR_ENCTYPE, apreq_parse_multipart);
> +
>
> f = apreq_parser(URL_ENCTYPE);
> CuAssertPtrNotNull(tc, f);
> ================================================================
> and got all the tests to pass. However, I don't understand
> why - I thought this was done in apreq_initialize(),
> which is called by testall.c (and the status checked for
> success).
But we need to check the return values of the apreq_register_parser
calls, especially in apreq_initialize, because apr threads may
not be reliable on Win32 (what's your apr version btw)?
I'd start by patching apreq_initialize and seeing what
happens from there. If we can't identify the bug,
lets consider either removing the new thread-locking
stuff or determining which libapr version has reliable
behavior.
--
Joe Schaefer