apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: svn commit: r1797413 - /apr/apr/branches/1.6.x/locks/netware/proc_mutex.c
Date Sat, 03 Jun 2017 18:36:57 GMT
On Sat, Jun 3, 2017 at 2:25 AM, William A Rowe Jr <wrowe@rowe-clan.net> wrote:
> On Fri, Jun 2, 2017 at 5:56 PM, Yann Ylavic <ylavic.dev@gmail.com> wrote:
>> On Fri, Jun 2, 2017 at 7:44 PM,  <wrowe@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
 }

?

Mime
View raw message