httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@ibm.net>
Subject Re: [PATCH] Re: extra syscalls related to ap_create_pipe()
Date Tue, 20 Jun 2000 15:15:51 GMT
> From: rbb@covalent.net
> Date: Tue, 20 Jun 2000 07:41:21 -0700 (PDT)
> 
> > Index: src/lib/apr/file_io/unix/pipe.c
> > ===================================================================
> > RCS file: /home/cvs/apache-2.0/src/lib/apr/file_io/unix/pipe.c,v
> > retrieving revision 1.34
> > diff -u -r1.34 pipe.c
> > --- src/lib/apr/file_io/unix/pipe.c	2000/06/17 17:04:16	1.34
> > +++ src/lib/apr/file_io/unix/pipe.c	2000/06/18 18:25:56
> > @@ -82,6 +82,8 @@
> >  #endif /* BEOS_BONE */
> >  
> >  #endif /* !BEOS */
> > +
> > +    thepipe->blocking = BLK_OFF;
> >      return APR_SUCCESS;
> >  }
> 
> No good.  This needs to be moved inside the #if...#else...#endif.  There
> is a case where BeOS can not set the pipe to non-blocking mode.  This
> makes it look like the pipe can have a timeout and it can't.

I see...  I think what should happen is that if we're BEOS but not
BEOS_BONE then pipenonblock() should return APR_ENOTIMPL.

Here is what happens with the current CVS code...

create a pipe:

  old BEOS:
    pipe returned in blocking state
  everywhere else
    pipe returned in non-blocking state

set pipe timeout:
  everywhere:
    no error

read from pipe when no data avail:
  old BEOS:
    hang
  everywhere else:
    return after timeout

If pipenonblock() returns APR_ENOTIMPL when called from
ap_set_pipe_timeout(), ap_set_pipe_timeout() will be able to tell the
caller that we can't do that.
    
> 
> I seriously dislike this new API.  I also do not think it is
> necessary.  Create all pipes as blocking.  When somebody specifically sets
> a pipe non-blocking or sets a timeout switch it.  What we have done here
> is to add complexity to an API that used to be very clean.  The
> (input|output)_blocking parameters are also integers, and this leads to
> messy-ness later.  See below.

O.k.  I'll keep the ap_create_pipe() signature like it was, make both
ends of the pipe blocking, and add ap_unblock_pipe() for when the
caller needs a handle to be non-blocking.  On Unix, ap_unblock_pipe() is just
the externalization of the internal function pipenonblock().

> > +        child_blocking = in == APR_FULL_BLOCK || in == APR_CHILD_BLOCK;
> > +        parent_blocking = in == APR_FULL_BLOCK || in == APR_PARENT_BLOCK;
> > +
> > +        if ((status = ap_create_pipe(&attr->child_in, child_blocking, 
> > +                                     &attr->parent_in, parent_blocking,

> > +                                     attr->cntxt)) != APR_SUCCESS) {
> >              return status;
> >          }
> > -        switch (in) {
> > -        case APR_FULL_BLOCK:
> > -            ap_block_pipe(attr->child_in);
> > -            ap_block_pipe(attr->parent_in);
> > -        case APR_PARENT_BLOCK:
> > -            ap_block_pipe(attr->parent_in);
> > -        case APR_CHILD_BLOCK:
> > -            ap_block_pipe(attr->child_in);
> > -        }
> >      } 
> 
> This whole section can be pretty much left alone if we remove those two
> parameters.  I think that makes the code much easier to read.

The style will remain the same; we'll just call ap_unblock_pipe() as
follows:

switch(in) {
case APR_FULL_BLOCK:
    break;
case APR_PARENT_BLOCK:
    ap_unblock_pipe(attr->child_in);
    break;
case APR_CHILD_BLOCK:
    ap_unblock_pipe(attr->parent_in);
    break;
default:
    ap_unblock_pipe(attr->child_in);
    ap_unblock_pipe(attr->parent_in);
}

Thanks,

Jeff
-- 
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
     http://www.geocities.com/SiliconValley/Park/9289/
          Born in Roswell... married an alien...

Mime
View raw message