Re: svn commit: r1806603 - /apr/apr/trunk/file_io/win32/readwrite.c

[Ivan Zhakov]

On 30 August 2017 at 13:57, Evgeny Kotkov  wrote:
> Bert Huijben  writes:
>
>>>  rv = WriteFile(thefile->filehand, buf, (DWORD)*nbytes, ,
>>> thefile->pOverlapped);
>>> +if (rv == APR_SUCCESS && thefile->pOverlapped) {
>>> +thefile->filePtr += *nbytes;
>>> +}
>>
>> The result of WriteFile should not be checked against APR_SUCCESS, as that
>> is not a documented Windows result code. The right constant evaluating to
>> 0 should be used.
>
> Unfortunately, I think that I have missed how the return value of WriteFile()
> was being checked in the last part of apr_file_write(), and both the r1806592
> and r1806603 changesets are wrong and change the behavior of the code.
>
> As these changesets were meant to be refactorings that make the code
> clearer, without altering its behavior, I think that they should be reverted
> for now.
>
> (I will prepare an alternative patch with these simplifications that doesn't
> change the behavior of the code.)
>
>
> Sorry for that,
> Evgeny Kotkov
Reverted r1806592 and r1806603 in r1806701.

-- 
Ivan Zhakov

Re: svn commit: r1806603 - /apr/apr/trunk/file_io/win32/readwrite.c

[Evgeny Kotkov]

Bert Huijben  writes:

>>  rv = WriteFile(thefile->filehand, buf, (DWORD)*nbytes, ,
>> thefile->pOverlapped);
>> +if (rv == APR_SUCCESS && thefile->pOverlapped) {
>> +thefile->filePtr += *nbytes;
>> +}
>
> The result of WriteFile should not be checked against APR_SUCCESS, as that
> is not a documented Windows result code. The right constant evaluating to
> 0 should be used.

Unfortunately, I think that I have missed how the return value of WriteFile()
was being checked in the last part of apr_file_write(), and both the r1806592
and r1806603 changesets are wrong and change the behavior of the code.

As these changesets were meant to be refactorings that make the code
clearer, without altering its behavior, I think that they should be reverted
for now.

(I will prepare an alternative patch with these simplifications that doesn't
change the behavior of the code.)


Sorry for that,
Evgeny Kotkov

RE: svn commit: r1806603 - /apr/apr/trunk/file_io/win32/readwrite.c

[Bert Huijben]

> -Original Message-
> From: i...@apache.org [mailto:i...@apache.org]
> Sent: dinsdag 29 augustus 2017 16:59
> To: comm...@apr.apache.org
> Subject: svn commit: r1806603 - /apr/apr/trunk/file_io/win32/readwrite.c
> 
> Author: ivan
> Date: Tue Aug 29 14:59:11 2017
> New Revision: 1806603
> 
> URL: http://svn.apache.org/viewvc?rev=1806603=rev
> Log:
> Minor refactoring of the Win32 file write code.
> 
> * file_io/win32/readwrite.c
>   (apr_file_write): Update filePtr in the asynchronous I/O mode after
>the WriteFile() call.  This allows simplifying the condition by not
>checking for thefile->pipe property.
> 
> Patch by: Evgeny Kotkov 
> 
> Modified:
> apr/apr/trunk/file_io/win32/readwrite.c
> 
> Modified: apr/apr/trunk/file_io/win32/readwrite.c
> URL:
> http://svn.apache.org/viewvc/apr/apr/trunk/file_io/win32/readwrite.c?rev
> =1806603=1806602=1806603=diff
> ==
> 
> --- apr/apr/trunk/file_io/win32/readwrite.c (original)
> +++ apr/apr/trunk/file_io/win32/readwrite.c Tue Aug 29 14:59:11 2017
> @@ -398,6 +398,9 @@ APR_DECLARE(apr_status_t) apr_file_write
>  }
>  rv = WriteFile(thefile->filehand, buf, (DWORD)*nbytes, ,
> thefile->pOverlapped);
> +if (rv == APR_SUCCESS && thefile->pOverlapped) {
> +thefile->filePtr += *nbytes;
> +}

The result of WriteFile should not be checked against APR_SUCCESS, as that is 
not a documented Windows result code. The right constant evaluating to 0 should 
be used.

Bert