storage: port host-flash device to the Phoenix-RTOS platform
#574
Conversation
Remove dependency on pread/pwrite (not yet present in the Phoenix), slightly refactor the code and enable build on ia32-generic platform. JIRA: RTOS-1105
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @agkaminski, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request ports the host-flash storage device driver to the Phoenix-RTOS platform. The primary goal is to remove the dependency on pread and pwrite system calls, which are not present in Phoenix-RTOS, by refactoring the file I/O operations. This change enables the host-flash component to be built and utilized on the ia32-generic platform, improving compatibility and expanding the capabilities of the storage subsystem within the Phoenix-RTOS ecosystem.
Highlights
- Build System Integration: The
host-flashcomponent has been added to the default build components for theia32-genericplatform, making it available for use in that environment. - File I/O Abstraction: The
host-flashdriver has been updated to remove its dependency onpreadandpwritesystem calls, which are not available in the Phoenix-RTOS environment. This was achieved by introducinghostflash_safeReadandhostflash_safeWritewrappers that use standardreadandwritecalls with explicitlseekoperations. - Code Refactoring and Portability: The logic for
hostflash_read,hostflash_write, andhostflash_sectorErasefunctions has been refactored to leverage the newly introducedhostflash_safeReadandhostflash_safeWritefunctions, simplifying their implementation and ensuring compatibility with the target platform.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request successfully ports the host-flash device to the Phoenix-RTOS platform by removing dependencies on pread and pwrite. The refactoring to use hostflash_safeRead and hostflash_safeWrite is a good improvement for code clarity. However, I've identified a few important issues. There are recurring type mismatches, using int where off_t or size_t would be correct, which could cause problems on 64-bit systems or with large data sizes. More critically, there is a potential data corruption bug in hostflash_sectorErase due to unhandled short writes. My review provides specific suggestions to fix these issues.
| ssize_t stat = hostflash_safeWrite(tempTab, sizeof(tempTab), sectorAddr + erased); | ||
| if (stat < 0) { | ||
| return (int)stat; | ||
| } |
There was a problem hiding this comment.
This implementation does not handle short writes from hostflash_safeWrite. If hostflash_safeWrite writes fewer bytes than requested without returning an error (e.g., if write() returns 0 because the device is full), stat will be positive but less than sizeof(tempTab). This will result in a partially erased sector, which can lead to data corruption. The return value should be checked to ensure the entire buffer was written.
| ssize_t stat = hostflash_safeWrite(tempTab, sizeof(tempTab), sectorAddr + erased); | |
| if (stat < 0) { | |
| return (int)stat; | |
| } | |
| ssize_t stat = hostflash_safeWrite(tempTab, sizeof(tempTab), sectorAddr + erased); | |
| if (stat != sizeof(tempTab)) { | |
| return (stat < 0) ? (int)stat : -EIO; | |
| } |
| static ssize_t hostflash_safeRead(void *buff, size_t bufflen, off_t offset) | ||
| { | ||
| ssize_t stat; | ||
| int ret = lseek(hostflash_common.filefd, offset, SEEK_SET); |
There was a problem hiding this comment.
The return type of lseek is off_t, but it's being stored in an int. This could lead to truncation on 64-bit systems where off_t is larger than int. Please use off_t for the ret variable to ensure correctness across different architectures.
| int ret = lseek(hostflash_common.filefd, offset, SEEK_SET); | |
| off_t ret = lseek(hostflash_common.filefd, offset, SEEK_SET); |
| return (ssize_t)ret; | ||
| } | ||
|
|
||
| int readsz = 0; |
There was a problem hiding this comment.
The readsz variable is of type int, but it accumulates the number of bytes read into a buffer whose size is given by bufflen (a size_t). If bufflen is larger than INT_MAX, readsz could overflow, leading to unpredictable behavior. It is safer to declare readsz as size_t to prevent this.
| int readsz = 0; | |
| size_t readsz = 0; |
|
|
||
| static ssize_t hostflash_safeWrite(const void *buff, size_t bufflen, off_t offset) | ||
| { | ||
| int ret = lseek(hostflash_common.filefd, offset, SEEK_SET); |
There was a problem hiding this comment.
The return type of lseek is off_t, but it's being stored in an int. This could lead to truncation on 64-bit systems where off_t is larger than int. Please use off_t for the ret variable to ensure correctness across different architectures.
| int ret = lseek(hostflash_common.filefd, offset, SEEK_SET); | |
| off_t ret = lseek(hostflash_common.filefd, offset, SEEK_SET); |
|
|
||
| while ((stat = pread(hostflash_common.filefd, (void *)((char *)buff + readsz), bufflen - readsz, offs + readsz)) != 0) { | ||
| if (stat < 0) { | ||
| int writesz = 0; |
There was a problem hiding this comment.
The writesz variable is of type int, but it accumulates the number of bytes written from a buffer whose size is given by bufflen (a size_t). If bufflen is larger than INT_MAX, writesz could overflow, leading to unpredictable behavior. It is safer to declare writesz as size_t to prevent this.
| int writesz = 0; | |
| size_t writesz = 0; |
Remove dependency on pread/pwrite (not yet present in the Phoenix), slightly refactor the code and enable build on ia32-generic platform.
JIRA: RTOS-1105
Description
Motivation and Context
Types of changes
How Has This Been Tested?
Checklist:
Special treatment