You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Yann Ylavic <yl...@gmail.com> on 2017/06/02 22:56:56 UTC
Re: svn commit: r1797413 - /apr/apr/branches/1.6.x/locks/netware/proc_mutex.c
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...
> #if 0
> /* We need to change apr_os_proc_mutex_t to a pointer type
> * to be able to implement this function.
> @@ -127,12 +127,8 @@ APR_DECLARE(apr_status_t) apr_os_proc_mu
> if (mech) {
> *mech = APR_LOCK_DEFAULT;
> }
> -#else
> - if (mech) {
> - *mech = APR_LOCK_DEFAULT;
> - }
> -#endif
> return APR_SUCCESS;
> +#endif
> }
Re: svn commit: r1797413 - /apr/apr/branches/1.6.x/locks/netware/proc_mutex.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Sat, Jun 3, 2017 at 1:36 PM, Yann Ylavic <yl...@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 <yl...@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.
Re: svn commit: r1797413 - /apr/apr/branches/1.6.x/locks/netware/proc_mutex.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Jun 5, 2017 at 6:07 PM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> On Jun 3, 2017 13:36, "Yann Ylavic" <yl...@gmail.com> wrote:
>
>
> #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
> }
>
>
> Can I challenge your assumpion here for 1.7.x?
>
> You are reading a change in apr_proc_mutex_t and apr_os_proc_mutex_t as a
> binary breaking change.
I didn't that feel this change :
-typedef NXMutex_t apr_os_proc_mutex_t;
+typedef NXMutex_t* apr_os_proc_mutex_t;
was a "minor" change.
>
> But if you consider that apr's type is opaque, and that the underlying
> system apr_os_proc_mutex_t could not be _get or _set through 1.6.x, then the
> apr_os_proc_mutex_t on Netware was simply unused.
But I'd like to concur to this point of view!
> Is there an actual
> versioning conflict correcting the type declaration on Netware so these work
> from 1.7.0 forwards?
What do you mean by "versioning conflict"?
If everyone is fine with the above type change (which can only fix
things, I don't see either how current apr_os_proc_mutex_t on netware
can be used), I'm very much for it to go to 1.7 (and even 1.6, why
not?)...
Re: svn commit: r1797413 - /apr/apr/branches/1.6.x/locks/netware/proc_mutex.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Jun 3, 2017 13:36, "Yann Ylavic" <yl...@gmail.com> wrote:
#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
}
Can I challenge your assumpion here for 1.7.x?
You are reading a change in apr_proc_mutex_t and apr_os_proc_mutex_t as a
binary breaking change.
But if you consider that apr's type is opaque, and that the underlying
system apr_os_proc_mutex_t could not be _get or _set through 1.6.x, then
the apr_os_proc_mutex_t on Netware was simply unused. Is there an actual
versioning conflict correcting the type declaration on Netware so these
work from 1.7.0 forwards?
Re: svn commit: r1797413 - /apr/apr/branches/1.6.x/locks/netware/proc_mutex.c
Posted by Yann Ylavic <yl...@gmail.com>.
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 <yl...@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
}
?