Proposed change to mpm_register_socket_callback(): apr_socket_t -> apr_pollfd_t
Hi all, I would like to propose a change to the mpm_register_socket_callback() hooks to add the ability to support both pipes as well as sockets. In addition, I need more control over the individual events to support the idea that one socket/pipe might want to read at the same time another might want to write. What I have in mind is to swap the apr_socket_t with a apr_pollfd_t, with all operation staying the same. Index: include/mpm_common.h === --- include/mpm_common.h(revision 1734657) +++ include/mpm_common.h(working copy) @@ -427,14 +427,14 @@ * @ingroup hooks */ AP_DECLARE_HOOK(apr_status_t, mpm_register_socket_callback, -(apr_socket_t **s, apr_pool_t *p, int for_read, ap_mpm_callback_fn_t *cbfn, void *baton)) +(apr_pollfd_t **pds, apr_pool_t *p, ap_mpm_callback_fn_t *cbfn, void *baton)) /* register the specified callback, with timeout * @ingroup hooks * */ AP_DECLARE_HOOK(apr_status_t, mpm_register_socket_callback_timeout, -(apr_socket_t **s, apr_pool_t *p, int for_read, +(apr_pollfd_t **pds, apr_pool_t *p, ap_mpm_callback_fn_t *cbfn, ap_mpm_callback_fn_t *tofn, void *baton, @@ -444,7 +444,7 @@ * @ingroup hooks */ AP_DECLARE_HOOK(apr_status_t, mpm_unregister_socket_callback, -(apr_socket_t **s, apr_pool_t *p)) +(apr_pollfd_t **pds, apr_pool_t *p)) /** Resume the suspended connection * @ingroup hooks Regards, Graham —
Re: Proposed change to mpm_register_socket_callback(): apr_socket_t -> apr_pollfd_t
On Sun, Mar 13, 2016 at 4:29 PM, Graham Leggett wrote: > What I have in mind is to swap the apr_socket_t with a apr_pollfd_t, with all > operation staying the same. I don't think I had a good reason for going one way or the other, +1 here. Trunk-only and presumably nobody consuming it from the lack of feedback. -- Eric Covener cove...@gmail.com
Re: Proposed change to mpm_register_socket_callback(): apr_socket_t -> apr_pollfd_t
On 13 Mar 2016, at 10:53 PM, Eric Covener wrote: > I don't think I had a good reason for going one way or the other, +1 > here. Trunk-only and presumably nobody consuming it from the lack of > feedback. From what I can see, this was never backported to v2.4. Regards, Graham —
Re: Proposed change to mpm_register_socket_callback(): apr_socket_t -> apr_pollfd_t
On Sun, Mar 13, 2016 at 4:54 PM, Graham Leggett wrote: > From what I can see, this was never backported to v2.4. I also meant the original feature never made it, so we can whatever we want to it.
Re: Proposed change to mpm_register_socket_callback(): apr_socket_t -> apr_pollfd_t
On 13 Mar 2016, at 10:55 PM, Eric Covener wrote: > I also meant the original feature never made it, so we can whatever we > want to it. What do you think of this? It includes a cleanup to the original pool to remove any unfired pollsets from the core. Index: include/ap_mpm.h === --- include/ap_mpm.h(revision 1734657) +++ include/ap_mpm.h(working copy) @@ -207,50 +207,48 @@ void *baton); /** - * Register a callback on the readability or writability on a group of sockets - * @param s Null-terminated list of sockets + * Register a callback on the readability or writability on a group of + * sockets/pipes. + * @param pds Null-terminated list of apr_pollfd_t * @param p pool for use between registration and callback - * @param for_read Whether the sockets are monitored for read or writability * @param cbfn The callback function * @param baton userdata for the callback function - * @return APR_SUCCESS if all sockets could be added to a pollset, + * @return APR_SUCCESS if all sockets/pipes could be added to a pollset, * APR_ENOTIMPL if no asynch support, or an apr_pollset_add error. - * @remark When activity is found on any 1 socket in the list, all are removed + * @remark When activity is found on any 1 socket/pipe in the list, all are removed * from the pollset and only 1 callback is issued. */ -AP_DECLARE(apr_status_t) ap_mpm_register_socket_callback(apr_socket_t **s, - apr_pool_t *p, - int for_read, - ap_mpm_callback_fn_t *cbfn, - void *baton); +AP_DECLARE(apr_status_t) ap_mpm_register_poll_callback(apr_pollfd_t **pds, + apr_pool_t *p, + ap_mpm_callback_fn_t *cbfn, + void *baton); /** - * Register a callback on the readability or writability on a group of sockets, with a timeout - * @param s Null-terminated list of sockets + * Register a callback on the readability or writability on a group of sockets/pipes, + * with a timeout. + * @param pds Null-terminated list of apr_polldf_t * @param p pool for use between registration and callback - * @param for_read Whether the sockets are monitored for read or writability * @param cbfn The callback function * @param tofn The callback function if the timeout expires * @param baton userdata for the callback function * @param timeout timeout for I/O in microseconds, unlimited if <= 0 - * @return APR_SUCCESS if all sockets could be added to a pollset, + * @return APR_SUCCESS if all sockets/pipes could be added to a pollset, * APR_ENOTIMPL if no asynch support, or an apr_pollset_add error. - * @remark When activity is found on any 1 socket in the list, all are removed + * @remark When activity is found on any 1 socket/pipe in the list, all are removed * from the pollset and only 1 callback is issued. * @remark For each call, only one of tofn or cbfn will be called, never both. */ -AP_DECLARE(apr_status_t) ap_mpm_register_socket_callback_timeout(apr_socket_t **s, +AP_DECLARE(apr_status_t) ap_mpm_register_poll_callback_timeout(apr_pollfd_t **pfds, apr_pool_t *p, - int for_read, ap_mpm_callback_fn_t *cbfn, ap_mpm_callback_fn_t *tofn, void *baton, apr_time_t timeout); -AP_DECLARE(apr_status_t) ap_mpm_unregister_socket_callback(apr_socket_t **s, - apr_pool_t *p); +AP_DECLARE(apr_status_t) ap_mpm_unregister_poll_callback(apr_pollfd_t **pfds, + apr_pool_t *p); typedef enum mpm_child_status { MPM_CHILD_STARTED, Index: include/mpm_common.h === --- include/mpm_common.h(revision 1734657) +++ include/mpm_common.h(working copy) @@ -426,15 +426,15 @@ * register the specified callback * @ingroup hooks */ -AP_DECLARE_HOOK(apr_status_t, mpm_register_socket_callback, -(apr_socket_t **s, apr_pool_t *p, int for_read, ap_mpm_callback_fn_t *cbfn, void *baton)) +AP_DECLARE_HOOK(apr_status_t, mpm_register_poll_callback, +(apr_pollfd_t **pds, apr_pool_t *p, ap_mpm_callback_fn_t *cbfn, void *baton)) /* register the specified callback, with timeout
Re: Proposed change to mpm_register_socket_callback(): apr_socket_t -> apr_pollfd_t
On Mon, Mar 14, 2016 at 12:41 AM, Graham Leggett wrote: > On 13 Mar 2016, at 10:55 PM, Eric Covener wrote: > >> I also meant the original feature never made it, so we can whatever we >> want to it. > > What do you think of this? Looks good and indeed very valuable to me, s/socket/pollfd/ is a great idea. Maybe a little nit-picking below... > > Index: include/ap_mpm.h > === > --- include/ap_mpm.h(revision 1734657) > +++ include/ap_mpm.h(working copy) > @@ -207,50 +207,48 @@ > void *baton); > > /** > - * Register a callback on the readability or writability on a group of > sockets > - * @param s Null-terminated list of sockets > + * Register a callback on the readability or writability on a group of > + * sockets/pipes. > + * @param pds Null-terminated list of apr_pollfd_t > * @param p pool for use between registration and callback > - * @param for_read Whether the sockets are monitored for read or writability > * @param cbfn The callback function > * @param baton userdata for the callback function > - * @return APR_SUCCESS if all sockets could be added to a pollset, > + * @return APR_SUCCESS if all sockets/pipes could be added to a pollset, > * APR_ENOTIMPL if no asynch support, or an apr_pollset_add error. > - * @remark When activity is found on any 1 socket in the list, all are > removed > + * @remark When activity is found on any 1 socket/pipe in the list, all are > removed > * from the pollset and only 1 callback is issued. > */ > > -AP_DECLARE(apr_status_t) ap_mpm_register_socket_callback(apr_socket_t **s, > - apr_pool_t *p, > - int for_read, > - > ap_mpm_callback_fn_t *cbfn, > - void *baton); > +AP_DECLARE(apr_status_t) ap_mpm_register_poll_callback(apr_pollfd_t **pds, > + apr_pool_t *p, > + ap_mpm_callback_fn_t > *cbfn, > + void *baton); Since apr_pollfd_t is not opaque (unlike apr_socket_t), maybe we could remove the indirection here (and in the code below) with somthing like (apr_pollfd_t *pfds, size_t npfds, ...). That would allow a single allocation (all pfds in once) and possibly make things easier for the caller. Regards, Yann.
Re: Proposed change to mpm_register_socket_callback(): apr_socket_t -> apr_pollfd_t
> Am 14.03.2016 um 09:32 schrieb Yann Ylavic : > > On Mon, Mar 14, 2016 at 12:41 AM, Graham Leggett wrote: >> On 13 Mar 2016, at 10:55 PM, Eric Covener wrote: >> >>> I also meant the original feature never made it, so we can whatever we >>> want to it. >> >> What do you think of this? > > Looks good and indeed very valuable to me, s/socket/pollfd/ is a great idea. Like the direction this is going as well. Do we need a MPM Query for detecting support before one actually has a handle for registration? > Maybe a little nit-picking below... > >> >> Index: include/ap_mpm.h >> === >> --- include/ap_mpm.h(revision 1734657) >> +++ include/ap_mpm.h(working copy) >> @@ -207,50 +207,48 @@ >>void *baton); >> >> /** >> - * Register a callback on the readability or writability on a group of >> sockets >> - * @param s Null-terminated list of sockets >> + * Register a callback on the readability or writability on a group of >> + * sockets/pipes. >> + * @param pds Null-terminated list of apr_pollfd_t >> * @param p pool for use between registration and callback >> - * @param for_read Whether the sockets are monitored for read or writability >> * @param cbfn The callback function >> * @param baton userdata for the callback function >> - * @return APR_SUCCESS if all sockets could be added to a pollset, >> + * @return APR_SUCCESS if all sockets/pipes could be added to a pollset, >> * APR_ENOTIMPL if no asynch support, or an apr_pollset_add error. >> - * @remark When activity is found on any 1 socket in the list, all are >> removed >> + * @remark When activity is found on any 1 socket/pipe in the list, all are >> removed >> * from the pollset and only 1 callback is issued. >> */ >> >> -AP_DECLARE(apr_status_t) ap_mpm_register_socket_callback(apr_socket_t **s, >> - apr_pool_t *p, >> - int for_read, >> - >> ap_mpm_callback_fn_t *cbfn, >> - void *baton); >> +AP_DECLARE(apr_status_t) ap_mpm_register_poll_callback(apr_pollfd_t **pds, >> + apr_pool_t *p, >> + ap_mpm_callback_fn_t >> *cbfn, >> + void *baton); > > Since apr_pollfd_t is not opaque (unlike apr_socket_t), maybe we could > remove the indirection here (and in the code below) with somthing like > (apr_pollfd_t *pfds, size_t npfds, ...). > That would allow a single allocation (all pfds in once) and possibly > make things easier for the caller. > > Regards, > Yann.
Re: Proposed change to mpm_register_socket_callback(): apr_socket_t -> apr_pollfd_t
On 14 Mar 2016, at 10:32 AM, Yann Ylavic wrote: > Since apr_pollfd_t is not opaque (unlike apr_socket_t), maybe we could > remove the indirection here (and in the code below) with somthing like > (apr_pollfd_t *pfds, size_t npfds, ...). > That would allow a single allocation (all pfds in once) and possibly > make things easier for the caller. This definitely makes sense. I originally wondered whether we could pass an apr_array_header_t into it, but it felt like overkill. Doing it your way means that we could use an array if we wanted to, or we could just pass structures on the stack, which would be much more flexible. Regards, Graham —
Re: Proposed change to mpm_register_socket_callback(): apr_socket_t -> apr_pollfd_t
> On Mar 14, 2016, at 7:07 AM, Graham Leggett wrote: > > On 14 Mar 2016, at 10:32 AM, Yann Ylavic wrote: > >> Since apr_pollfd_t is not opaque (unlike apr_socket_t), maybe we could >> remove the indirection here (and in the code below) with somthing like >> (apr_pollfd_t *pfds, size_t npfds, ...). >> That would allow a single allocation (all pfds in once) and possibly >> make things easier for the caller. > > This definitely makes sense. > > I originally wondered whether we could pass an apr_array_header_t into it, > but it felt like overkill. Doing it your way means that we could use an array > if we wanted to, or we could just pass structures on the stack, which would > be much more flexible. > +1 for stack approach. Also, +1 on the whole concept :)
Re: Proposed change to mpm_register_socket_callback(): apr_socket_t -> apr_pollfd_t
On 14 Mar 2016, at 10:32 AM, Yann Ylavic wrote: > Since apr_pollfd_t is not opaque (unlike apr_socket_t), maybe we could > remove the indirection here (and in the code below) with somthing like > (apr_pollfd_t *pfds, size_t npfds, ...). > That would allow a single allocation (all pfds in once) and possibly > make things easier for the caller. Having looked at this in more detail this isn’t as simple. The sticking point is the cleanup, which needs to be passed a single struct to do it’s work. To pass both the pfds and the npdfs these two need to be wrapped in a structure of some kind, which the user has to feed in from the outside common to both register and unregister. This structure probably should be an apr_array_header_t. Is it worth making the change from apr_pollfd_t **pfds to apr_array_header_t *pfds? Regards, Graham —
Re: Proposed change to mpm_register_socket_callback(): apr_socket_t -> apr_pollfd_t
On Mon, Mar 14, 2016 at 8:51 PM, Graham Leggett wrote: > On 14 Mar 2016, at 10:32 AM, Yann Ylavic wrote: > >> Since apr_pollfd_t is not opaque (unlike apr_socket_t), maybe we could >> remove the indirection here (and in the code below) with somthing like >> (apr_pollfd_t *pfds, size_t npfds, ...). >> That would allow a single allocation (all pfds in once) and possibly >> make things easier for the caller. > > Having looked at this in more detail this isn’t as simple. > > The sticking point is the cleanup, which needs to be passed a single struct > to do it’s work. To pass both the pfds and the npdfs these two need to be > wrapped in a structure of some kind, which the user has to feed in from the > outside common to both register and unregister. This structure probably > should be an apr_array_header_t. Hmm right, either this or, say, the ending pfd is the one with ->reqevents=0, or ->p=NULL, or probably better ->desc_type=APR_NO_DESC. Up to the caller to ensure this, it could possibly be documented. > > Is it worth making the change from apr_pollfd_t **pfds to apr_array_header_t > *pfds? That would still require some "complex" initialization, so I'd prefer the above if it doesn't sound to hacky... Regards, Yann.
Re: Proposed change to mpm_register_socket_callback(): apr_socket_t -> apr_pollfd_t
On 14 Mar 2016, at 11:17 PM, Yann Ylavic wrote : >> Having looked at this in more detail this isn’t as simple. >> >> The sticking point is the cleanup, which needs to be passed a single struct >> to do it’s work. To pass both the pfds and the npdfs these two need to be >> wrapped in a structure of some kind, which the user has to feed in from the >> outside common to both register and unregister. This structure probably >> should be an apr_array_header_t. > > Hmm right, either this or, say, the ending pfd is the one with > ->reqevents=0, or ->p=NULL, or probably better > ->desc_type=APR_NO_DESC. > Up to the caller to ensure this, it could possibly be documented. > >> >> Is it worth making the change from apr_pollfd_t **pfds to apr_array_header_t >> *pfds? > > That would still require some "complex" initialization, so I'd prefer > the above if it doesn't sound to hacky… I just gave it a try and it was a lot more elegant than I thought. You create an array the precise size you need, and off you go. Regards, Graham — httpd-register-poll2.patch Description: Binary data
Re: Proposed change to mpm_register_socket_callback(): apr_socket_t -> apr_pollfd_t
On 14 Mar 2016, at 11:13 AM, Stefan Eissing wrote: > Like the direction this is going as well. > > Do we need a MPM Query for detecting support before one actually has a handle > for registration? We do - most recent patch has that, and we managed to drop one directive out of mod_proxy_wstunnel. Regards, Graham —
Re: Proposed change to mpm_register_socket_callback(): apr_socket_t -> apr_pollfd_t
On Mon, Mar 14, 2016 at 11:29 PM, Graham Leggett wrote: > On 14 Mar 2016, at 11:17 PM, Yann Ylavic wrote > : >>> Having looked at this in more detail this isn’t as simple. >>> >>> The sticking point is the cleanup, which needs to be passed a single struct >>> to do it’s work. To pass both the pfds and the npdfs these two need to be >>> wrapped in a structure of some kind, which the user has to feed in from the >>> outside common to both register and unregister. This structure probably >>> should be an apr_array_header_t. >> >> Hmm right, either this or, say, the ending pfd is the one with >> ->reqevents=0, or ->p=NULL, or probably better >> ->desc_type=APR_NO_DESC. >> Up to the caller to ensure this, it could possibly be documented. >> >>> >>> Is it worth making the change from apr_pollfd_t **pfds to >>> apr_array_header_t *pfds? >> >> That would still require some "complex" initialization, so I'd prefer >> the above if it doesn't sound to hacky… > > I just gave it a try and it was a lot more elegant than I thought. > > You create an array the precise size you need, and off you go. That's indeed elegant enough, but I think I prefer the plain array solution, still :p Don't you like? { apr_pollfd_t pfds[3]; /* or */ apr_pollfd_t *pfds = apr_pcalloc(p, 3 * apr_pollfd_t); pfds[0].desc_type = APR_POLL_SOCKET; pfds[0].desc.s = s0; ... pfds[1].desc_type = APR_POLL_SOCKET; pfds[1].desc.s = s1; ... pfds[2].desc_type = APR_NO_DESC; ap_mpm_register_poll_callback_timeout(pfds, pool, ...); } And the callbacks/cleanups could then loop with: { apr_pollfd_t *pfd; for (pfd = pfds; pfd->desc_type != APR_NO_DESC; pfd++) { ... } } Since APR_NO_DESC == 0, we could even omit it above (both for initializing pfds[2] thanks to calloc, and for the != APR_NO_DESC in the loop)... No struct needed for cleanup_register() either, we can pass the pfds (plain) array as is (terminated by .desc_type == APR_NO_DESC instead of the NULL-pointer of your first/indirect version, we can likewise require this from the user). But, after all, I'm not really against using an apr_array either... Regards, Yann.
Re: Proposed change to mpm_register_socket_callback(): apr_socket_t -> apr_pollfd_t
On 15 Mar 2016, at 1:09 PM, Yann Ylavic wrote: >> I just gave it a try and it was a lot more elegant than I thought. >> >> You create an array the precise size you need, and off you go. > > That's indeed elegant enough, but I think I prefer the plain array > solution, still :p > > Don't you like? > { >apr_pollfd_t pfds[3]; >/* or */ >apr_pollfd_t *pfds = apr_pcalloc(p, 3 * apr_pollfd_t); > >pfds[0].desc_type = APR_POLL_SOCKET; >pfds[0].desc.s = s0; >... >pfds[1].desc_type = APR_POLL_SOCKET; >pfds[1].desc.s = s1; >... >pfds[2].desc_type = APR_NO_DESC; > >ap_mpm_register_poll_callback_timeout(pfds, pool, ...); > } The trouble with the above is that because of the pool cleanup we now have, pfds[3] needs to live as long as pool p. In your example it does, but there is nothing stopping someone trying to allocate pfds[3] on the stack and then returning. Later the cleanup is run, and boom - difficult to debug crash or weird behaviour. With the array you’re guaranteed the memory is allocated from a pool, which means the pool cleanup will always be safe. What we should also do is drop the apr_pool_t *p parameter and read it from apr_header_array_t’s pool instead. This will be a further check to stop the caller from doing anything pathological, as we definitely know the cleanup and the array belong to each other, and our API becomes simpler still. Attached patch does this. Regards, Graham — httpd-register-poll3.patch Description: Binary data
Re: Proposed change to mpm_register_socket_callback(): apr_socket_t -> apr_pollfd_t
On Tue, Mar 15, 2016 at 8:28 PM, Graham Leggett wrote: > > The trouble with the above is that because of the pool cleanup we now have, > pfds[3] needs to live as long as pool p. In your example it does, but there > is nothing stopping someone trying to allocate pfds[3] on the stack and then > returning. Later the cleanup is run, and boom - difficult to debug crash or > weird behaviour. Was just an example on the possibily to use a plain array for the interface (a stacked one is of course not recommended to register an event callback likely run out of scope). > > With the array you’re guaranteed the memory is allocated from a pool, which > means the pool cleanup will always be safe. You convinced me, I like your may better now ;) Possibly we could also force each pfd->p to pfds->pool in event_register_poll_callback_ex(). > > What we should also do is drop the apr_pool_t *p parameter and read it from > apr_header_array_t’s pool instead. This will be a further check to stop the > caller from doing anything pathological, as we definitely know the cleanup > and the array belong to each other, and our API becomes simpler still. > > Attached patch does this. +1 Regards, Yann.
Re: Proposed change to mpm_register_socket_callback(): apr_socket_t -> apr_pollfd_t
> On Mar 15, 2016, at 3:28 PM, Graham Leggett wrote: > > The trouble with the above is that because of the pool cleanup we now have, > pfds[3] needs to live as long as pool p. In your example it does, but there > is nothing stopping someone trying to allocate pfds[3] on the stack and then > returning. Later the cleanup is run, and boom - difficult to debug crash or > weird behaviour. > > With the array you’re guaranteed the memory is allocated from a pool, which > means the pool cleanup will always be safe. > > What we should also do is drop the apr_pool_t *p parameter and read it from > apr_header_array_t’s pool instead. This will be a further check to stop the > caller from doing anything pathological, as we definitely know the cleanup > and the array belong to each other, and our API becomes simpler still. > > Attached patch does this. > +1