Re: Hello

Re: ap_save_brigade vs ENOTIMPL setaside

On Thu, Sep 23, 2004 at 09:36:30PM +0100, Joe Orton wrote:
> So what's the proper fix?  If ap_save_brigade did:
> 
>  if bkt->setaside() == ENOTIMPL
>    bkt->read()
>    if bkt->setaside() != SUCCESS 
>      give up

There is a problem with this new logic: the mod_perl SV bucket type
happens to be a case where ->setaside() is ENOTIMPL (currently) and
->read() does not morph the bucket type, so then ap_save_brigade() will
abort without doing any work which is quite nasty.

I think the logic needs to be refined to copy the read data into a HEAP
or POOL bucket if the second setaside() call still gives ENOTIMPL, as
below.  I'm not sure whether POOL or HEAP is better, but if it does use
a POOL the data may get copied again if it is later turned into a HEAP,
which seems inefficient.  Comments?

--- server/util_filter.c	26 Sep 2004 15:52:51 -0000	1.101
+++ server/util_filter.c	1 Oct 2004 11:33:27 -0000
 <at>  <at>  -557,6 +557,18  <at>  <at> 
             rv = apr_bucket_read(e, &s, &n, APR_BLOCK_READ);
             if (rv == APR_SUCCESS) {
                 rv = apr_bucket_setaside(e, p);
+                if (rv == APR_ENOTIMPL) {
+                    apr_bucket *h;
+                    
+                    /* No choice but to manually move it into a heap bucket */
+                    h = apr_bucket_heap_create(s, n, NULL, f->c->bucket_alloc);
+                    
+                    APR_BUCKET_INSERT_AFTER(e, h);
+                    apr_bucket_delete(e);
+
+                    rv = APR_SUCCESS;
+                    e = h;
+                }
             }
         }

Re: Hello

Re: [PATCH 21492 30278 30419 31385] Several bugs in mod_cache / mod_disk_cache

Rüdiger Plüm wrote:

> 
> Justin Erenkrantz wrote:
> 
>> --On Wednesday, September 29, 2004 10:55 AM -0400 Bill Stoddard 
>> <bill <at> wstoddard.com> wrote:
>>
> 
> [..cut..]
> 
>>>
>>>
>>> Well the process is not quite so structured by design, but yes, I 
>>> plan to
>>> propose we backport these into 2.0 (in time for 2.0.53) and I'll do the
>>> backport as I have time (maybe later this week).
>>
>>
>>
>> Haha.  Well, it requires three developers to approve a change for 
>> backporting. And, Bill has been excellent in driving these changes to 
>> be backported.  I usually prefer to wait until I know that he or 
>> someone else confirms the commits work for them and don't do anything 
>> stupid before it even gets proposed.  And, then another developer 
>> needs to sign off first...
>>
>>> You can help by testing Justin's patch and providing feedback to this 
>>> list.
>>
>>
>>
>> Absolutely!  Thanks!  -- justin
> 
> 
> I just tested Justin's patch(es) by compiling 
> apr-util_20040930041253.tar.gz /
> / apr_20040930041241.tar.gz and httpd-2.0_20040930041734.tar.gz and 
> doing some
> tests for the problems that are described in 30278 / 21492. The result 
> of my tests
> is that the problem is now fixed by Justin's patches.
> As the patch for 30278 has nearly not changed since 2.0.50 I think it is 
> worth mentioning that
> 
> 1. it is used at our company on a production system running 2.0.50 
> without any problems
>    and fixing the problem it has been designed for
> 
> 2. it has been tested by another user (Igor Fedulov) who opened report 
> 30278
> 
> Hope that helps.

It does, thanks.  First order of business is to garner another vote for Justin's mod_disk_cache patch 
referenced in the 2.0 STATUS file. Then I'll submit his 'stale cache entry' patch to a vote for backport.

Bill

Re: ap_save_brigade vs ENOTIMPL setaside

On Fri, 1 Oct 2004, Joe Orton wrote:

> There is a problem with this new logic: the mod_perl SV bucket type
> happens to be a case where ->setaside() is ENOTIMPL (currently) and
> ->read() does not morph the bucket type, so then ap_save_brigade() will
> abort without doing any work which is quite nasty.

That sounds like a bug in the mod_perl SV bucket type.  ->read() is
supposed to be defined such that the result of the read is a bucket that
*can* be set aside.

From apr_buckets.h, starting at line 76:

  * read returns the address and size of the data in the bucket. If the
  * data isn't in memory then it is read in and the bucket changes type
  * so that it can refer to the new location of the data. If all the
  * data doesn't fit in the bucket then a new bucket is inserted into
  * the brigade to hold the rest of it.

In other words, if the data is not in memory, ->read() should put it there
and morph the bucket type.  If it is already in memory, then ->setaside()
should be implemented.

The logic to deal with all this is already complicated enough from the
caller's perspective... I'd rather not make it even *more* complicated
just to deal with broken bucket types.

--Cliff

Re: ap_save_brigade vs ENOTIMPL setaside

On Fri, Oct 01, 2004 at 11:16:13AM -0400, Cliff Woolley wrote:
> On Fri, 1 Oct 2004, Joe Orton wrote:
> 
> > There is a problem with this new logic: the mod_perl SV bucket type
> > happens to be a case where ->setaside() is ENOTIMPL (currently) and
> > ->read() does not morph the bucket type, so then ap_save_brigade() will
> > abort without doing any work which is quite nasty.
> 
> That sounds like a bug in the mod_perl SV bucket type.  ->read() is
> supposed to be defined such that the result of the read is a bucket that
> *can* be set aside.
>
> >From apr_buckets.h, starting at line 76:
> 
>   * read returns the address and size of the data in the bucket. If the
>   * data isn't in memory then it is read in and the bucket changes type
>   * so that it can refer to the new location of the data. If all the
>   * data doesn't fit in the bucket then a new bucket is inserted into
>   * the brigade to hold the rest of it.

Yes, the ->read() function meets those requirements fine, the bucket
type just wraps a Perl variable in memory so ->read() just gives you
back that memory.  But the problem is that ->setaside() is ENOTIMPL.

> In other words, if the data is not in memory, ->read() should put it there
> and morph the bucket type.  If it is already in memory, then ->setaside()
> should be implemented.
> 
> The logic to deal with all this is already complicated enough from the
> caller's perspective... I'd rather not make it even *more* complicated
> just to deal with broken bucket types.

OK, that makes sense.  I just want to be able to backport the
ap_save_brigade() changes without triggering unnecessary regressions in
current mod_perl installations.  The mod_perl guys know that the lack of
->setaside() is a bug and are working on that for future releases.

So a better proposal is this: if setaside() == ENOTIMPL, read() == OK, 
and then setaside() still == ENOTIMPL, then still carry on and setaside 
the rest of buckets in the brigade, but do return the error.

This is equivalent to the old logic except that it will return an error
for a bad bucket type which can't be setaside, and it will correctly
setaside more good bucket types e.g. CGI buckets.  (all
ap_save_brigade() callers in 2.0 ignore the return value so the former
change really makes little difference)

Is this OK?

--- server/util_filter.c	26 Sep 2004 15:52:51 -0000	1.101
+++ server/util_filter.c	1 Oct 2004 15:33:34 -0000
 <at>  <at>  -532,7 +532,7  <at>  <at> 
                                          apr_bucket_brigade **b, apr_pool_t *p)
 {
     apr_bucket *e;
-    apr_status_t rv;
+    apr_status_t rv, srv = APR_SUCCESS;

     /* If have never stored any data in the filter, then we had better
      * create an empty bucket brigade so that we can concat.
 <at>  <at>  -561,11 +561,16  <at>  <at> 
         }

         if (rv != APR_SUCCESS) {
-            return rv;
+            srv = rv;
+            /* Return an error but still save the brigade if
+             * ->setaside() is really not implemented. */
+            if (rv != APR_ENOTIMPL) {
+                return rv;
+            }
         }
     }
     APR_BRIGADE_CONCAT(*saveto, *b);
-    return APR_SUCCESS;
+    return srv;
 }

 AP_DECLARE_NONSTD(apr_status_t) ap_filter_flush(apr_bucket_brigade *bb, 

Time to test...

If you haven't already, please grab 1.3.32-dev HEAD and
test it out. I'd like some additional good feedback
before I commit to a T&R. As such, I'd like to
impose a on-hold policy for any more code commits.
--
=======================================================================
  Jim Jagielski   [|]   jim <at> jaguNET.com   [|]   http://www.jaguNET.com/
   "There 10 types of people: those who read binary and everyone else."

Re: cvs commit: httpd-2.0/os/unix os.h unixd.c

On 1 Oct 2004 16:03:09 -0000, jfclere <at> apache.org <jfclere <at> apache.org> wrote:
> jfclere     2004/10/01 09:03:09
> 
>   Modified:    os       config.m4
>                os/unix  os.h unixd.c
>   Log:
>   Move the few BS2000 specific in unixd.c

>   Index: unixd.c
>   ===================================================================
>   RCS file: /home/cvs/httpd-2.0/os/unix/unixd.c,v
>   retrieving revision 1.69
>   retrieving revision 1.70
>   diff -u -r1.69 -r1.70
>   --- unixd.c   24 Apr 2004 19:42:52 -0000      1.69
>   +++ unixd.c   1 Oct 2004 16:03:08 -0000       1.70
>    <at>  <at>  -457,11 +457,26  <at>  <at> 
>    {
>        apr_socket_t *csd;
>        apr_status_t status;
>   +#ifdef _OSD_POSIX
>   +    int sockdes;
>   +#endif
> 
>        *accepted = NULL;
>        status = apr_socket_accept(&csd, lr->sd, ptrans);
>        if (status == APR_SUCCESS) {
>            *accepted = csd;
>   +#ifdef _OSD_POSIX
>   +        apr_os_sock_get(&sockdes, csd);
>   +        if (sockdes >= FD_SETSIZE) {
>   +            ap_log_error(APLOG_MARK, APLOG_WARNING, 0, NULL,
>   +                         "new file descriptor %d is too large; you probably need "
>   +                         "to rebuild Apache with a larger FD_SETSIZE "
>   +                         "(currently %d)",
>   +                         sockdes, FD_SETSIZE);
>   +            apr_socket_close(csd);
>   +            return APR_EINTR;
>   +        }
>   +#endif

the old logic was removed on purpose

an analogous check is in APR at the point where select() is actually
needed; an error will be returned from the apr poll call or the apr
send/recv call

the APR check covers not just sockets but also pipes used to
communicate with CGIs

it would be nice if there was a special APR error code to use there
with a unique error message though

(btw, it is APR that would need to be rebuilt)

Re: ap_save_brigade vs ENOTIMPL setaside

On Fri, 1 Oct 2004, Joe Orton wrote:

> current mod_perl installations.  The mod_perl guys know that the lack of
> ->setaside() is a bug and are working on that for future releases.
>
> So a better proposal is this: if setaside() == ENOTIMPL, read() == OK,
> and then setaside() still == ENOTIMPL, then still carry on and setaside
> the rest of buckets in the brigade, but do return the error.
>
> Is this OK?

+1, sounds like a good compromise.

--Cliff

Patch available for mod_auth_ldap bug 24437

I just produced a patch that seems to fix the remaining issues for bug 
24437.

--
Jess Holle