[GitHub] [incubator-nuttx] codebje commented on a change in pull request #2861: fs: change geometry sizes from `size_t` to `uint32_t`

[GitBox <gi...@apache.org>]

codebje commented on a change in pull request #2861:
URL: https://github.com/apache/incubator-nuttx/pull/2861#discussion_r578181641



##########
File path: include/nuttx/fs/fs.h
##########
@@ -206,11 +206,11 @@ struct file_operations
 #ifndef CONFIG_DISABLE_MOUNTPOINT
 struct geometry
 {
-  bool   geo_available;    /* true: The device is available */
-  bool   geo_mediachanged; /* true: The media has changed since last query */
-  bool   geo_writeenabled; /* true: It is okay to write to this device */
-  size_t geo_nsectors;     /* Number of sectors on the device */
-  size_t geo_sectorsize;   /* Size of one sector */
+  bool     geo_available;    /* true: The device is available */
+  bool     geo_mediachanged; /* true: The media has changed since last query */
+  bool     geo_writeenabled; /* true: It is okay to write to this device */
+  uint32_t geo_nsectors;     /* Number of sectors on the device */
+  uint32_t geo_sectorsize;   /* Size of one sector */

Review comment:
       @xiaoxiang781216 yes - and the more I look into it, the more there's an eclectic mix of integer types involved. At the very least, `struct block_operations` uses `size_t` for start sector for read and write (but `unsigned long` for the sector count). `struct file_operations` uses `size_t` and `ssize_t`, which I think is both POSIX defined and reasonable - when reading and writing, the limiting factor is the maximum addressable block. `seek` is already using `off_t`.
   
   There's other possible problems, like `fat_read` uses variously `size_t`, `ssize_t`, `unsigned int`, `int`, `uint32_t`, and `off_t` without checking for conversion issues. The file size in `struct fat_file_s` is an `off_t` so files are restricted to 2GB rather than the full 4GB of FAT32. In practice these aren't likely to be significant issues.
   
   I think the minimum possible change to stop using `size_t` for block counts would be to modify `struct block_operations` for `read` and `write` along with the block drivers implementing those methods.
   
   I'd rather not try to make `size_t` be 32 bits on the eZ80. If the change isn't workable, I'll maintain it on a fork anyway, because that somehow seems like a better plan than just swapping my 16Gb card for an 8Gb one.
   
   It's not just the eZ80 that's affected, either - all the architectures with `CONFIG_SMALL_MEMORY` have a 16-bit `size_t` and would be restricted to 32Mb block devices. It's just the eZ80 that has the unusual middle ground of 24-bit integers.
   
   @patacongo It seems like `blkcnt_t` is the right choice for indices and counts, while `off_t` would be the right choice for relative values.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org