Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed fstat on win32 (support 64 bit file sizes) #1918

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

esohns
Copy link
Contributor

@esohns esohns commented Sep 5, 2022

No description provided.

@jwillemsen
Copy link
Member

See also #1074

@jwillemsen jwillemsen added the needs review Needs to be reviewed label Sep 6, 2022
@esohns
Copy link
Contributor Author

esohns commented Sep 6, 2022

Note: the ACE_OS::stat function (OS_NS_sys_stat.inl lines 221) has the same issue for the Windows CE case (ACE_HAS_WINCE). Not sure whether Windows CE ever supported 64 bits though... In that case, it might make more sense to use the alternate approach of #1074 and use the ACE macro (ACE_MAKE_QWORD) instead of my solution (ULARGE_INTEGER comes from the current winnt.h; I do not know when it was first introduced).

ACE_COMBINE_PARTS could be used instead also, but note it has a "signed" high part, while nFileSizeHigh and nFileSizeLow members of BY_HANDLE_FILE_INFORMATION are both unsigned DWORDs [Note: (oddly enough,) ACE_OFF_T is "signed" as well...why ?]. Also, it refers to winnt.h (for LARGE_INTEGER), which may not be available on Windows CE systems (this claim needs confirmation). IMHO using ACE_MAKE_QWORD seems to be the more generic approach (only bit shifts and logic operators).

So I also suggest to first move the macro ACE_MAKE_QWORD functionality out of OS_NS_time.h to a more "global" header (Global_Macros.h, somewhere around ACE_NOOP, or OS_NS_macros.h ?), as mitza-oci suggested, so it can be used everywhere; and someone could then update all usages of ACE_COMBINE_PARTS/ [U]LARGE_INTEGER to move to ACE_MAKE_QWORD later on.

As jwillemsen already mentioned, there probably should exist some test-case for this (for both 64 bit and 32 bit platforms, using ACE_MAKE_QWORD ?/ACE_COMBINE_PARTS ?/...?).

FWIW I tested this solution locally, and it works OK with files larger than 4GB (64 bit Windows 11 VS2022, ACE compiled with _FILE_OFFSET_BITS == 64). However, I cannot test the "true" 64 bit corner cases (i.e. between signed/unsigned ACE_OFF_T) due to lack of resources, and I am not going back to 32 bits (unless someone pays me...).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review Needs to be reviewed
Development

Successfully merging this pull request may close these issues.

2 participants