On Thu, 31 Oct 2024 07:15:16 GMT, Taizo Kurashige <d...@openjdk.org> wrote:
> To resolve java/io/File/GetXSpace.java failure, I fix libGetXSpace.c to use > Cygwin’s `df` to get the size for comparison if the test target drive is a > CD-ROM drive. > > As described in JDK-8343342, GetDiskSpaceInformationW can't get information > about the size of the CD-ROM drive. > GetDiskFreeSpaceExW can also get information about the size of the CD-ROM > drive. However, because GetDiskFreeSpaceExW is called by the File.get-X-Space > methods, it seems more reasonable to compare the size got by other way than > GetDiskFreeSpaceExW as a test. For this reason, I use Cygwin's `df`. > In JDK-8298619, GetDiskSpaceInformationW was adopted instead of `df` because > the size got by File.get-X-Space methods may not match the size got by `df` > when per-user quotas are used. I don't think this problem applies to CD-ROM > drive, so I think we can use Cygwin's `df` for CD-ROM drive. > > After fix, I ran a test on Windows Server 2019 where drive C is a normal > local disk, drive D is an unmounted iso CD-ROM drive, and drive F is an iso > mounted CD-ROM drive and confirmed that it passes. > > I think this fix may also resolves the similar failure reported at > https://github.com/openjdk/jdk/pull/12397#issuecomment-1705164515. > > Thanks Thank you for your review. > > If there were also a simpler way to handle the empty drive case, that would > > be good. > > Actually this simple patch appears to fix the problem with a CD drive > attached whether it is empty or contains a CD: Indeed, applying your suggested code seems to solve the problem since an empty CD drive is not tested and GetDiskFreeSpaceExW is used to test a CD drive containing a CD. However, I recognize that the purpose of GetXSpace.java is to verify that the basic functionality of getTotalSpace(), getFreeSpace(), and getUsableSpace(). These methods are available for an empty CD drive, so I think it is more appropriate not to skip an empty CD drive's test. In addition, GetDiskFreeSpaceExW is used in the internal implementation of these methods. For this reason, it seems more appropriate to use values obtained by means other than GetDiskFreeSpaceExW (For example, "df") for testing. Therefore, I would prefer not to use GetDiskFreeSpaceExW to test a CD drive containing a CD. Based on the above thoughts, I considered the proposed changes undesirable. ------------- PR Comment: https://git.openjdk.org/jdk/pull/21799#issuecomment-2507988069
