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

FIleSize in os_fstat_t is uint32, should be 64bit friendly #572

Closed
ghost opened this issue Aug 20, 2020 · 5 comments · Fixed by #654
Closed

FIleSize in os_fstat_t is uint32, should be 64bit friendly #572

ghost opened this issue Aug 20, 2020 · 5 comments · Fixed by #654
Milestone

Comments

@ghost
Copy link

ghost commented Aug 20, 2020

FileSize in os_fstat_t is uint32, but should be off_t like in POSIX. On 64-bit systems. it's a 64-bit value and this causes conversion issues.

@jphickey
Copy link
Contributor

This hasn't been changed up to this point in the interest of backward compatibility, and the fact that flight systems rarely have even 4GB files systems let alone individual files that exceed 4GB in size.

But - to future proof it is probably reasonably safe to bump it from uint32 to uint64.

I would avoid off_t though - Preference is to use fixed width types in the API.

@ghost
Copy link
Author

ghost commented Aug 20, 2020

off_t allows for variable width across platforms. Unless this structure is being marshaled to a telemetry stream there's no reason not to do this. Otherwise we have to use typecasts in 64-bit operating systems.

It just seems awkward to be put in a position to make a 64-bit OSAL and have to typecast things in places.

Also it should be worth noting that I don't want to build with -Wconversion. I was using it as a tool to identify places in the API where conflicts between 32-bit and 64-bit environments may occur.

@jphickey
Copy link
Contributor

OSAL's role is to make all systems "look the same" to applications even if they are not the same underneath. So it should return/output a consistent data type which is not variable across platforms - That's the objective.

Note that large files/filesystems with 64 bit offsets can exist on 32-bit systems too. It is independent of CPU and purely a matter of the type of filesystem layer you are using.

The CCB just needs to decide if OSAL needs to support large file sizes - as the API is currently limited to 2GB individual file size (int32). I have no objection to moving to 64-bit offsets, but it should be fixed in the OSAL API, not variable.

@skliper skliper changed the title os_fstat_t should use off_t FIleSize in of_fstat_t is uint32, should be 64bit friendly Aug 20, 2020
@skliper skliper added the bug label Aug 20, 2020
@skliper skliper added this to the 6.0.0 milestone Aug 20, 2020
@skliper skliper added enhancement and removed bug labels Aug 20, 2020
@skliper skliper removed this from the 6.0.0 milestone Aug 20, 2020
@skliper
Copy link
Contributor

skliper commented Aug 20, 2020

Concur w/ fixed 64-bit.

Use the issue templates and fill out all information. Needs to clearly indicate the platform and what the underlying issue is (and be searchable as a historical record of issues).

@skliper skliper changed the title FIleSize in of_fstat_t is uint32, should be 64bit friendly FIleSize in os_fstat_t is uint32, should be 64bit friendly Dec 9, 2020
@skliper
Copy link
Contributor

skliper commented Dec 9, 2020

Now defined as size_t, fixed in #654.

@skliper skliper closed this as completed Dec 9, 2020
@skliper skliper linked a pull request Dec 9, 2020 that will close this issue
@skliper skliper added this to the 6.0.0 milestone Sep 24, 2021
jphickey pushed a commit to jphickey/osal that referenced this issue Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants