RFR(S): 8170936: Logging: LogFileOutput.invalid_file_test crashes when executed twice.

[Thomas Stüfe]

Hi Kirill,

This looks fine now to me. Thanks for fixing!

Further comments inline.

On Tue, Dec 20, 2016 at 7:04 PM, Kirill Zhaldybin <
kirill.zhaldybin at oracle.com> wrote:

> Thomas,
>
> Thank you for you comments. I really appreciate your review.
> Here are a new WebRev: http://cr.openjdk.java.net/~kz
> haldyb/webrevs/JDK-8170936/webrev.01/
>
> Changes:
> 1. renamed delete_directory to delete_empty_directory
> 2. left only GetLastError() in error output
>
> Could you please also read comments inline?
>
> Thank you.
>
> Regards, Kirill
>
> On 20.12.2016 16:13, Thomas Stüfe wrote:
>
>> (also, I am not a (R)eviewer, so I guess this does not count as real
>> review)
>>
>> On Tue, Dec 20, 2016 at 2:13 PM, Thomas Stüfe <thomas.stuefe at gmail.com
>> <mailto:thomas.stuefe at gmail.com>> wrote:
>>
>>     Hi Kirill,
>>
>>     thanks a lot for fixing this! This was an annoyance on Windows.
>>
>>     Just small remarks:
>>     - RemoveDirectory will only work on empty directories. Recursively
>>     deleting non-empty directories seems to be more difficult. So could
>>     you please add a comment that this function only deletes empty
>>     directories and is not intended to work recursively? Or rename the
>>     function to delete_empty_directory()?
>>
> fixed
>
>>     - You do not have to output errno if RemoveDirectory failed, that
>>     number will be unrelated to the error. GetLastError should be enough.
>>
> fixed
>
>>
>>     I also wonder why all those utilitiy functions are imeplemented in a
>>     header file? I think it would be better for a C file (can change
>>     implementation without triggering rebuild; also binary may be
>>     smaller, at least on slowdebug).
>>
> well, I do not know why it was implemented that way but I could speculate
> that it was simpler (one file instead of two), the functions were not
> supposed to be changed often and side effects (size and build time
> increase) were not considered significant.
> But it is only my speculations :)
> If you think we should change this could you  please let me know? I will
> file an RFE.
>

I do not have strong emotions, just small preferences :) I would leave it
up to Marcus, this is more of a style question.

Kind Regards, Thomas


>
>>     Kind regards, Thomas
>>
>>
>>     On Tue, Dec 20, 2016 at 1:56 PM, Kirill Zhaldybin
>>     <kirill.zhaldybin at oracle.com <mailto:kirill.zhaldybin at oracle.com>>
>>     wrote:
>>
>>         Dear all,
>>
>>         Could you please review this fix for 8170936?
>>
>>         The issue was windows specific - remove() function failed to
>>         delete a directory on that platform.
>>         I added delete_directory function which uses winapi
>>         RemoveDirectory function and also does GTest style check on its
>>         result.
>>         I also changed TEST to TEST_VM on a few tests from the same file
>>         since they used ResourceMark and crashed if they were run
>> isolated.
>>
>>         WebRev:
>>         http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8170936/web
>> rev.00/
>>         <http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8170936/we
>> brev.00/>
>>         CR: https://bugs.openjdk.java.net/browse/JDK-8170936
>>         <https://bugs.openjdk.java.net/browse/JDK-8170936>
>>
>>         Thank you.
>>
>>         Regards, Kirill
>>
>>
>>
>>
>