[resend: list's reply policy strikes again :/ ]
On Mon, Mar 7, 2016 at 12:47 PM, Yann Ylavic <ylavic....@gmail.com> wrote:
> On Mon, Mar 7, 2016 at 10:52 AM, Ruediger Pluem <rpl...@apache.org> wrote:
>>
>> On 03/06/2016 01:19 AM, yla...@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...
> 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)?
>
>>
>>> Modified: apr/apr/trunk/locks/unix/proc_mutex.c
>>> URL:
>>> http://svn.apache.org/viewvc/apr/apr/trunk/locks/unix/proc_mutex.c?rev=1733775&r1=1733774&r2=1733775&view=diff
>>> ==============================================================================
>>> --- apr/apr/trunk/locks/unix/proc_mutex.c (original)
>>> +++ apr/apr/trunk/locks/unix/proc_mutex.c Sun Mar 6 00:19:51 2016
>>
>>> @@ -488,16 +504,16 @@ static apr_status_t proc_mutex_proc_pthr
>>> return errno;
>>> }
>>>
>>> - new_mutex->pthread_interproc = (pthread_mutex_t *)mmap(
>>> - (caddr_t) 0,
>>> - sizeof(proc_pthread_mutex_t),
>>> - PROT_READ | PROT_WRITE, MAP_SHARED,
>>> - fd, 0);
>>> - if (new_mutex->pthread_interproc == (pthread_mutex_t *) (caddr_t) -1) {
>>> + new_mutex->os.pthread_interproc = mmap(NULL,
>>> sizeof(proc_pthread_mutex_t),
>>> + PROT_READ | PROT_WRITE,
>>> MAP_SHARED,
>>> + fd, 0);
>>> + if (new_mutex->os.pthread_interproc == MAP_FAILED) {
>>> + new_mutex->os.pthread_interproc = NULL;
>>> + rv = errno;
>>> close(fd);
>>> - return errno;
>>> + return rv;
>>> }
>>> - close(fd);
>>
>> Why don't we close the fd any longer?
>
> Good catch! Not really intentional...
> Will fix.
>
> Thanks for the review Rüdiger.
>
> Regards,
> Yann.
