apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: svn commit: r1733775 - in /apr/apr/trunk: ./ include/ include/arch/unix/ locks/beos/ locks/netware/ locks/os2/ locks/unix/ locks/win32/
Date Mon, 07 Mar 2016 11:58:14 GMT


On 03/07/2016 12:53 PM, Yann Ylavic wrote:
> [resend: list's reply policy strikes again :/ ]
> 
> On Mon, Mar 7, 2016 at 12:47 PM, Yann Ylavic <ylavic.dev@gmail.com> wrote:
>> On Mon, Mar 7, 2016 at 10:52 AM, Ruediger Pluem <rpluem@apache.org> wrote:
>>>
>>> On 03/06/2016 01:19 AM, ylavic@apache.org wrote:
>>>>
>>>>  /* Implement OS-specific accessors defined in apr_portable.h */
>>>>
>>>> -apr_status_t apr_os_proc_mutex_get(apr_os_proc_mutex_t *ospmutex,
>>>> -                                   apr_proc_mutex_t *pmutex)
>>>> +APR_DECLARE(apr_status_t) apr_os_proc_mutex_get_ex(apr_os_proc_mutex_t *ospmutex,
>>>> +                                                   apr_proc_mutex_t *pmutex,
>>>> +                                                   apr_lockmech_e *mech)
>>>> +{
>>>> +#if 1
>>>> +    /* We need to change apr_os_proc_mutex_t to a pointer type
>>>> +     * to be able to implement this function.
>>>> +     */
>>>> +    return APR_ENOTIMPL;
>>>> +#else
>>>> +    if (!pmutex->mutex) {
>>>> +        return APR_ENOLOCK;
>>>> +    }
>>>> +    *ospmutex = pmutex->mutex->mutex;
>>>> +    if (mech) {
>>>> +        *mech = APR_LOCK_DEFAULT;
>>>> +    }
>>>> +    return APR_SUCCESS;
>>>> +#endif
>>>> +}
>>>> +
>>>> +APR_DECLARE(apr_status_t) apr_os_proc_mutex_get(apr_os_proc_mutex_t *ospmutex,
>>>> +                                                apr_proc_mutex_t *pmutex)
>>>>  {
>>>> -    if (pmutex)
>>>> -        ospmutex = pmutex->mutex->mutex;
>>>> -    return APR_ENOLOCK;
>>>> +    return apr_os_proc_mutex_get_ex(ospmutex, pmutex, NULL);
>>>>  }
>>>
>>> Previously we always returned APR_ENOLOCK now we return always APR_ENOTIMPL and
ospmutex will not be set.
>>> Is this correct?
>>
>> The pointer ospmutex is local here, there is no point in changing its
>> value, that won't help the caller :)
>> The issue is that on Netware, apr_os_proc_mutex_t is the native
>> NXMutex_t struct, so we can't really return a copy to the caller with
>> something like "*ospmutex = *pmutex->mutex;" either since it is likely
>> to not work as expected...

Thanks for explaining.

>> That's why I changed to apr_os_proc_mutex_t to NXMutex_t* in the
>> follow up (r1733777), which breaks the API, whereas this commit aims
>> to be backportable to 1.6 (still apr_os_proc_mutex_get() is
>> ENOTIMPLementable...).
>>
>> However I should have leaved (unconditionally) what is done in
>> r1733777 (and in other platforms) wrt ENOLOCK:
>>
>>     if (!pmutex->mutex) {
>>         return APR_ENOLOCK;
>>     }
>> #if 1
>>     /* ... */
>>     return APR_ENOTIMPL;
>> #else
>>     ...
>> #endif
>>
>> Does it work for you if I take care of this when/if backporting (since
>> trunk is already fixed)?

This is fine for me.

Regards

RĂ¼diger


Mime
View raw message