Re: svn commit: r1833786 - /apr/apr/branches/1.6.x/STATUS
On 21/06/18 10:36, Rainer Jung wrote:
> Am 21.06.2018 um 10:24 schrieb jean-frederic clere:
>> On 21/06/18 10:04, Yann Ylavic wrote:
>>> On Thu, Jun 21, 2018 at 8:00 AM, jean-frederic clere
>>> wrote:
OK if everyone agrees I will just commit my small fix and don't
backport
the 2 other revisions.
>>>
>>> Are we looking at the same 1.6.x code?
>>> For me apr_file_buffer_set() locks the mutex unconditionally so it
>>> must unlock it likewise, I don't understand what your patch solves.
>>
>> that is what my patch is doing ;-)
>
> Please be more explicit. I agree with Yann. From current unpatched APR
> 1.6.x file_io/win32/buffer.c:
>
> APR_DECLARE(apr_status_t) apr_file_buffer_set(apr_file_t *file,
> char * buffer,
> apr_size_t bufsize)
> {
> ...
> apr_thread_mutex_lock(file->mutex);
>
> if(file->buffered) {
> ...
> if (rv != APR_SUCCESS) {
> apr_thread_mutex_unlock(file->mutex);
> return rv;
> }
> }
>
> ... (no return)
>
> apr_thread_mutex_unlock(file->mutex);
>
> return APR_SUCCESS;
> }
>
> So where's the problem? Unconditional "apr_thread_mutex_lock" and
> unconditional "apr_thread_mutex_unlock" on every path before return.
>
> Looking at your patch
> "http://people.apache.org/~jfclere/patches/patch.180618.txt; (STATUS
> still contains a broken link, so we need to guess):
>
> Index: file_io/win32/buffer.c
> ===
> --- file_io/win32/buffer.c (revision 1833783)
> +++ file_io/win32/buffer.c (working copy)
> @@ -47,8 +47,10 @@
> */
> file->buffered = 0;
> }
> -
> - apr_thread_mutex_unlock(file->mutex);
> +
> + if (file->buffered) {
> + apr_thread_mutex_unlock(file->mutex);
> + }
>
> return APR_SUCCESS;
> }
>
>
> any call to "apr_file_buffer_set" for a file with "buffered" not set
> will end with not giving back the lock.
Correct... My problem is that file->mutex is NULL not that the file is
buffered or not...
>
> Regards,
>
> Rainer
>
--
Cheers
Jean-Frederic
Re: svn commit: r1833786 - /apr/apr/branches/1.6.x/STATUS
Am 21.06.2018 um 10:24 schrieb jean-frederic clere:
On 21/06/18 10:04, Yann Ylavic wrote:
On Thu, Jun 21, 2018 at 8:00 AM, jean-frederic clere wrote:
OK if everyone agrees I will just commit my small fix and don't backport
the 2 other revisions.
Are we looking at the same 1.6.x code?
For me apr_file_buffer_set() locks the mutex unconditionally so it
must unlock it likewise, I don't understand what your patch solves.
that is what my patch is doing ;-)
Please be more explicit. I agree with Yann. From current unpatched APR
1.6.x file_io/win32/buffer.c:
APR_DECLARE(apr_status_t) apr_file_buffer_set(apr_file_t *file,
char * buffer,
apr_size_t bufsize)
{
...
apr_thread_mutex_lock(file->mutex);
if(file->buffered) {
...
if (rv != APR_SUCCESS) {
apr_thread_mutex_unlock(file->mutex);
return rv;
}
}
... (no return)
apr_thread_mutex_unlock(file->mutex);
return APR_SUCCESS;
}
So where's the problem? Unconditional "apr_thread_mutex_lock" and
unconditional "apr_thread_mutex_unlock" on every path before return.
Looking at your patch
"http://people.apache.org/~jfclere/patches/patch.180618.txt; (STATUS
still contains a broken link, so we need to guess):
Index: file_io/win32/buffer.c
===
--- file_io/win32/buffer.c (revision 1833783)
+++ file_io/win32/buffer.c (working copy)
@@ -47,8 +47,10 @@
*/
file->buffered = 0;
}
-
-apr_thread_mutex_unlock(file->mutex);
+
+if (file->buffered) {
+apr_thread_mutex_unlock(file->mutex);
+}
return APR_SUCCESS;
}
any call to "apr_file_buffer_set" for a file with "buffered" not set
will end with not giving back the lock.
Regards,
Rainer
Re: svn commit: r1833786 - /apr/apr/branches/1.6.x/STATUS
On 21/06/18 10:04, Yann Ylavic wrote: > On Thu, Jun 21, 2018 at 8:00 AM, jean-frederic clere > wrote: >> >> OK if everyone agrees I will just commit my small fix and don't backport >> the 2 other revisions. > > Are we looking at the same 1.6.x code? > For me apr_file_buffer_set() locks the mutex unconditionally so it > must unlock it likewise, I don't understand what your patch solves. that is what my patch is doing ;-) > > Regards, > Yann. > -- Cheers Jean-Frederic
Re: svn commit: r1833786 - /apr/apr/branches/1.6.x/STATUS
On Thu, Jun 21, 2018 at 8:00 AM, jean-frederic clere wrote: > > OK if everyone agrees I will just commit my small fix and don't backport > the 2 other revisions. Are we looking at the same 1.6.x code? For me apr_file_buffer_set() locks the mutex unconditionally so it must unlock it likewise, I don't understand what your patch solves. Regards, Yann.
Re: svn commit: r1833786 - /apr/apr/branches/1.6.x/STATUS
On 21/06/18 01:08, Evgeny Kotkov wrote: > William A Rowe Jr writes: > >> Confusing... which patch is missing? >> >> http://svn.apache.org/viewvc?view=revision=1808457 >> http://svn.apache.org/viewvc?view=revision=1829962 >> >> It should not be possible at this point to need 1.x specific code >> that wouldn't correspond to a trunk commit. > > I think that r1829962 should be treated as a follow-up to r1808457 > (where I missed one calling site of apr_thread_mutex_unlock() in buffer.c). > > In other words, they should either be backported together, or not backported > at all. Since this change is a relatively minor optimization (not a fix), > I would think that it might make sense to keep it just in trunk. Actually it is a fix logresolve cores without it ;-) OK if everyone agrees I will just commit my small fix and don't backport the 2 other revisions. Cheers Jean-Frederic > > > Thanks, > Evgeny Kotkov >
Re: svn commit: r1833786 - /apr/apr/branches/1.6.x/STATUS
William A Rowe Jr writes: > Confusing... which patch is missing? > > http://svn.apache.org/viewvc?view=revision=1808457 > http://svn.apache.org/viewvc?view=revision=1829962 > > It should not be possible at this point to need 1.x specific code > that wouldn't correspond to a trunk commit. I think that r1829962 should be treated as a follow-up to r1808457 (where I missed one calling site of apr_thread_mutex_unlock() in buffer.c). In other words, they should either be backported together, or not backported at all. Since this change is a relatively minor optimization (not a fix), I would think that it might make sense to keep it just in trunk. Thanks, Evgeny Kotkov
Re: svn commit: r1833786 - /apr/apr/branches/1.6.x/STATUS
On Wed, Jun 20, 2018 at 1:50 AM, jean-frederic clere wrote: > On 19/06/18 11:02, Yann Ylavic wrote: > > Hi Jean-Frédéric, > > > > On Tue, Jun 19, 2018 at 10:01 AM, wrote: > >> Author: jfclere > >> Date: Tue Jun 19 08:01:45 2018 > >> New Revision: 1833786 > >> > >> URL: http://svn.apache.org/viewvc?rev=1833786=rev > >> Log: > >> proposal for review. > > > > Usually APR backports are CTR (Commit Then Review), but it does not > > prevent to propose them first ;) > > I don't find the corresponding trunk commit here, though. > > Trunk is quite different the missing piece in 1.6.x is in > file_io/win32/buffer.c line 55 (trunk is good). Confusing... which patch is missing? http://svn.apache.org/viewvc?view=revision=1808457 http://svn.apache.org/viewvc?view=revision=1829962 It should not be possible at this point to need 1.x specific code that wouldn't correspond to a trunk commit.
Re: svn commit: r1833786 - /apr/apr/branches/1.6.x/STATUS
On 19/06/18 11:02, Yann Ylavic wrote: > Hi Jean-Frédéric, > > On Tue, Jun 19, 2018 at 10:01 AM, wrote: >> Author: jfclere >> Date: Tue Jun 19 08:01:45 2018 >> New Revision: 1833786 >> >> URL: http://svn.apache.org/viewvc?rev=1833786=rev >> Log: >> proposal for review. > > Usually APR backports are CTR (Commit Then Review), but it does not > prevent to propose them first ;) > I don't find the corresponding trunk commit here, though. Trunk is quite different the missing piece in 1.6.x is in file_io/win32/buffer.c line 55 (trunk is good). > >> >> +* make sure we don't unlock mutex when we haven't locked it. >> + 1.6.x patch: http://people.apache.org/~jfclere/patch.180618.txt > (assuming this is > http://people.apache.org/~jfclere/patches/patch.180618.txt, above is > 404). > > In 1.6.x, the mutex is locked unconditionally, I don't think we can > unlock it when ->buffered only. > Don't you want the whole http://svn.apache.org/r1808457 instead? That is why I wrote in STATUS, wasn't sure what is best and my changes are minimal. If you think the whole http://svn.apache.org/r1808457 is better I will go for it. Cheers Jean-Frederic > > Regards, > Yann. >
Re: svn commit: r1833786 - /apr/apr/branches/1.6.x/STATUS
Hi Jean-Frédéric, On Tue, Jun 19, 2018 at 10:01 AM, wrote: > Author: jfclere > Date: Tue Jun 19 08:01:45 2018 > New Revision: 1833786 > > URL: http://svn.apache.org/viewvc?rev=1833786=rev > Log: > proposal for review. Usually APR backports are CTR (Commit Then Review), but it does not prevent to propose them first ;) I don't find the corresponding trunk commit here, though. > > +* make sure we don't unlock mutex when we haven't locked it. > + 1.6.x patch: http://people.apache.org/~jfclere/patch.180618.txt (assuming this is http://people.apache.org/~jfclere/patches/patch.180618.txt, above is 404). In 1.6.x, the mutex is locked unconditionally, I don't think we can unlock it when ->buffered only. Don't you want the whole http://svn.apache.org/r1808457 instead? Regards, Yann.
