On Sat, Jun 3, 2017 at 1:36 PM, Yann Ylavic <ylavic....@gmail.com> wrote: > 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 > } > > ?
The troubling bit preventing us from returning APR_ENOTIMPL is that I believe only Netware has ever had such an error-result from this _get function, so we can't simply point to Unix and state that they were returning ENOTIMPL. It really seems it would be a change of error response. I agree with you that ENOTIMPL is a better 'answer', but I would want to hear from folks who are supporting the Netware schema.
