> > +#ifdef AP_MPM_USES_POD
> > +static apr_file_t *pod_in = NULL, *pod_out = NULL;
>
> Please use a context structure rather than globals. That will allow us to
> create a POD on a per-thread basis. Then you'll have your desired
> functionality of a parent telling a child, "hey *you*. please stop."
>
> An MPM can also choose to use just one POD for all of its children, and
> signal it N times (like you did in this patch).
>
> You could also use one POD for graceful restart and one for shutdown (let's
> say that an MPM is built to reset its state, rather than expect to die/spawn
> a thread).
I disagree. By using a simple global variable, we make it much easier to
use this in an MPM. I had originally done exactly what you suggested, and
I found that the code that needed to be added to the MPM was more complex
than I wanted it to be. This change makes the pod completely opaque to
MPMs. If they want to use it some other way, then they will need to
re-write the pod stuff anyway, and they can change the prototype, or just
keep it internal to the MPM.
> > +AP_DECLARE(apr_status_t) ap_mpm_pod_open(apr_pool_t *p)
>
> AP_DECLARE(apr_status_t) ap_mpm_pod_open(ap_pod_ctx **ctx, apr_pool_t *p)
>
> Take a pool. Return an ap_pod_ctx *.
>
> Note that the ap_pod_ctx can also keep a pool so the other functions won't> need to
>accept one.
As I said above, I disagree.
> > +{
> > + apr_file_t *pod;
> >
> > + pod = apr_file_pipe_create(&pod_in, &pod_out, p);
> > + apr_file_pipe_timeout_set(out, 0);
>
> "out" ... never heard of that variable. You sure you tested this? :-)
Compiled and tested. This function as it happens had all sorts of
problems that the compiler missed. I have fixed them all now.
> > +{
> > + char c;
> > + apr_size_t len = 1;
> > + apr_status_t rv;
> > +
> > + rv = apr_file_read(pod_in, &c, &len);
> > +
> > + if ((rv == APR_SUCCESS) && (len == 1)) {
> > + return APR_SUCCESS;
> > + }
> > + return 1;
>
> "1" is not a legal apr_status_t value. I would recommend one of two things
> for this function:
>
> 1) handle all errors internally (logging and whatnot) and return a boolean
> on whether the caller should continue/die.
It is not easy to log in this function, because I don't really have enough
information.
> 2) return APR_SUCCESS, APR_ESHOULDDIE (or whatever), or an error and make
> all callers deal with processing errors.
>
> I prefer option (1). A caller isn't going to know what to do with an error
> besides log it, so why return it to them?
>
> [ since the internals of ap_mpm_pod_check() are opaque, then when APR_EINTR
> is returned, how can the MPM know what to do? ]
I have already fixed this, by creating a new apr_status_t for Apache to
use for this case.
> >...
> > +AP_DECLARE(void) ap_mpm_pod_signal(apr_pool_t *p)
> > +{
> > + apr_socket_t *sock = NULL;
> > + apr_sockaddr_t *sa = NULL;
> > +
> > + if ((rv = apr_file_write(pipe_of_death_out, &char_of_death, &one))
>
> "pipe_of_death_out" "char_of_death" "one" What are these variables?
>
> Are you really sure you tried this? :-)
I'm telling you, this all worked, I haven't got a clue why the damn build
isn't catching these, especially since I did a make clean;make. This is
however beginning to bother me.
> >...
> > + apr_sockaddr_info_get(&sa, "127.0.0.1", APR_UNSPEC, ap_listeners->sd->port,
>0, p);
>
> Urk. This magic of looking into ap_listeners doesn't feel right. I don't
> have an immediate solution, but there must be some kind of abstraction for
> fetching the listening port.
Why? We have the listening port in the ap_listeners list, so we should
use it.
> > + apr_socket_create(&sock, sa->family, SOCK_STREAM, p);
> > + apr_connect(sock, sa);
> > +}
>
> Besides the leak in file descriptors (as Jeff mentioned), we should also be
> careful of the pool. Is this pool going to disappear soon? Or if we're doing
> a restart, will it continue to be used?
This is pconf. Therefore the pool will live until we restart, and then it
is cleared.
> Note that you also need to close the socket so the MPM will see that a
> request does not exist, thus allowing it to process the POD-signal right
> away. Otherwise, it will block on a read, waiting for input.
Already done. Thanks.
Ryan
_______________________________________________________________________________
Ryan Bloom [EMAIL PROTECTED]
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
