Re: svn commit: r1828492 - in /apr/apr/trunk: include/apr_reslist.h util-misc/apr_reslist.c

[Ruediger Pluem <rp...@apache.org>]

On 04/06/2018 09:53 AM, ylavic@apache.org wrote:
> Author: ylavic
> Date: Fri Apr  6 07:53:02 2018
> New Revision: 1828492
> 
> URL: http://svn.apache.org/viewvc?rev=1828492&view=rev
> Log:
> reslist: follow up to r1828289: enfore empty list requirement when setting fifo.
> 
> The doxygen remark wasn't enough as noted by Ruediger.
> 
> 
> Modified:
>     apr/apr/trunk/include/apr_reslist.h
>     apr/apr/trunk/util-misc/apr_reslist.c
> 

> 
> Modified: apr/apr/trunk/util-misc/apr_reslist.c
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/util-misc/apr_reslist.c?rev=1828492&r1=1828491&r2=1828492&view=diff
> ==============================================================================
> --- apr/apr/trunk/util-misc/apr_reslist.c (original)
> +++ apr/apr/trunk/util-misc/apr_reslist.c Fri Apr  6 07:53:02 2018
> @@ -445,9 +445,15 @@ APR_DECLARE(void) apr_reslist_timeout_se
>      reslist->timeout = timeout;
>  }
>  
> -APR_DECLARE(void) apr_reslist_fifo_set(apr_reslist_t *reslist, int fifo)
> +APR_DECLARE(apr_status_t) apr_reslist_fifo_set(apr_reslist_t *reslist,
> +                                               int fifo)
>  {
> +    if (!APR_RING_EMPTY(&reslist->avail_list, apr_res_t, link)) {

Don't we need to lock if want to do this or is this sufficiently atomic?
Further on don't we need to make the check and the setting of reslist->fifo atomic, hence both done while we hold the lock?

> +        return APR_EBUSY;
> +    }
> +
>      reslist->fifo = fifo;
> +    return APR_SUCCESS;
>  }
>  

Regards

Rüdiger

Re: svn commit: r1828492 - in /apr/apr/trunk: include/apr_reslist.h util-misc/apr_reslist.c

[Yann Ylavic <yl...@gmail.com>]

On Fri, Apr 6, 2018 at 10:26 AM, Ruediger Pluem <rp...@apache.org> wrote:
>
> On 04/06/2018 09:53 AM, ylavic@apache.org wrote:
>>
>> +    if (!APR_RING_EMPTY(&reslist->avail_list, apr_res_t, link)) {
>
> Don't we need to lock if want to do this or is this sufficiently
> atomic? Further on don't we need to make the check and the setting of
> reslist->fifo atomic, hence both done while we hold the lock?

As I said in the other thread, I'd prefer to keep the test "light"
since we document how the helper should be used.

It's indeed racy without the lock but still good enough I think: only
the first added (or last removed) resource(s) concerned for that race
be relevant, and then the winner would have to pop (resp. push) the
resource(s) before the loser is rescheduled for anything to possibly
"malfunction", where the "malfunction" would be that the resource(s)
might not expire on time, while the time diff for a race is quite tiny
in the first place...

No crash/corruption possible in any case, a non-issue IMHO, and almost
deserved if one wants to use the API outside of its scope, it's not
like we enforce everything in APR :)

Regards,
Yann.