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

[Kirill Zhaldybin]

Thomas,

Thank you for review!

Regards, Kirill

On 20.12.2016 23:20, Thomas Stüfe wrote:
> 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 <mailto: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/~kzhaldyb/webrevs/JDK-8170936/webrev.01/
>     <http://cr.openjdk.java.net/~kzhaldyb/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>
>         <mailto: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>
>         <mailto: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/webrev.00/
>         <http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8170936/webrev.00/>
>
>         <http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8170936/webrev.00/
>         <http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8170936/webrev.00/>>
>                  CR: https://bugs.openjdk.java.net/browse/JDK-8170936
>         <https://bugs.openjdk.java.net/browse/JDK-8170936>
>                  <https://bugs.openjdk.java.net/browse/JDK-8170936
>         <https://bugs.openjdk.java.net/browse/JDK-8170936>>
>
>                  Thank you.
>
>                  Regards, Kirill
>
>
>
>
>