Re: Proposed change to mpm_register_socket_callback(): apr_socket_t -> apr_pollfd_t

[Jim Jagielski]

> 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

Re: Proposed change to mpm_register_socket_callback(): apr_socket_t -> apr_pollfd_t

[Yann Ylavic]

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

[Graham Leggett]

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

[Yann Ylavic]

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

[Graham Leggett]

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

[Graham Leggett]

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

[Yann Ylavic]

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

[Graham Leggett]

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

[Jim Jagielski]

> 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

[Graham Leggett]

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

[Stefan Eissing]

> 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

[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.

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

[Graham Leggett]

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 

Re: Proposed change to mpm_register_socket_callback(): apr_socket_t -> apr_pollfd_t

[Eric Covener]

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

[Graham Leggett]

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

[Eric Covener]

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