On Sat, Jun 3, 2017 at 2:25 AM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> On Fri, Jun 2, 2017 at 5:56 PM, Yann Ylavic <ylavic....@gmail.com> wrote:
>> On Fri, Jun 2, 2017 at 7:44 PM, <wr...@apache.org> wrote:
>>> Author: wrowe
>>> Date: Fri Jun 2 17:44:41 2017
>>> New Revision: 1797413
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1797413&view=rev
>>> Log:
>>> Revert to 1.5.x apr_os_proc_mutex_get() behavior on Netware with an
>>> additional
>>> guard against dereferencing a NULL pointer. The assignment appears to be a
>>> noop
>>> but avoiding changes in this logic from 1.5.x -> 1.6.x is the primary goal.
>>> ==============================================================================
>>> --- apr/apr/branches/1.6.x/locks/netware/proc_mutex.c (original)
>>> +++ apr/apr/branches/1.6.x/locks/netware/proc_mutex.c Fri Jun 2 17:44:41
>>> 2017
>>> @@ -116,9 +116,9 @@ APR_DECLARE(apr_status_t) apr_os_proc_mu
>>> apr_proc_mutex_t
>>> *pmutex,
>>> apr_lockmech_e *mech)
>>> {
>>> - if (!pmutex->mutex) {
>>> - return APR_ENOLOCK;
>>> - }
>>> + if (pmutex && pmutex->mutex)
>>> + ospmutex = pmutex->mutex->mutex;
>>> + return APR_ENOLOCK;
>>
>> Hmm, do you mean Netware users are doomed to get ENOLOCK whaever happens?
>> I don't see the issue without this change, the change was somehow a
>> bugfix imo...
>
> The downside is that we are changing the behavior.
>
> I don't mind if this all becomes ENOTIMPL in 2.0, but that is
> again a change in the API and problematic for 1.x.
Fair enough.
> I don't mind
> if the assignment to ospmutex was truly a noop and eliminated
> as a bugfix, but typically we would have backported such a bug
> fix back to 1.5.x which is where I was looking for the canonical
> behavior.
The fake/noop assignment is really misleading.
Maybe we could comment the situation better, like below?
Index: locks/netware/proc_mutex.c
===================================================================
--- locks/netware/proc_mutex.c (revision 1797526)
+++ locks/netware/proc_mutex.c (working copy)
@@ -129,9 +129,6 @@ APR_DECLARE(apr_status_t) apr_os_proc_mutex_get_ex
apr_proc_mutex_t *pmutex,
apr_lockmech_e *mech)
{
- if (pmutex && pmutex->mutex)
- ospmutex = pmutex->mutex->mutex;
- return APR_ENOLOCK;
#if 0
/* We need to change apr_os_proc_mutex_t to a pointer type
* to be able to implement this function.
@@ -141,6 +138,11 @@ APR_DECLARE(apr_status_t) apr_os_proc_mutex_get_ex
*mech = APR_LOCK_DEFAULT;
}
return APR_SUCCESS;
+#else
+ /* ENOTIMPL could be more meaningful, ENOLOCK is what 1.x has
+ * always returned... From 2.x, the API issue is fixed.
+ */
+ return APR_ENOLOCK;
#endif
}
?
