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

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

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



##########
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:
       > 
   > 
   > @patacongo, @xiaoxiang781216 Is this the correct approach?
   
   size_t is intended to be wide enough to hold the biggest addressable object in memory.  So it varies with CPU architecture.  It should have nothing to do with the geometry of media underlying a block device.
   
   This change will have no impact on the hordes of people using 32-bit ARMs.  For them size_t is the same as uint32_t.  For people using 64-bit devices this will shrink from 64- to 32- bits, but I don't think that is an issue; this is not addressable memory, this size_t is determines the maximum number of sectors that can be supported.  For the eZ80, size_t is 24-bits.
   
   For a 24-bit size_t, the use of size_t would limit the maximum size of, say, an SD card or eMMC to 8Gb which is unnecessary.  The size of addressable memory should not affect the maximum size of the supported media.
   
   I don't think the change is needed for geo_sectorsize which I think is 512 on all SD cards and maybe up to 4Kb on some HDDs.  It is only geo_nectors where size_t introduces a real limitation (still size_t is not good choice of types).
   
   There may be other issues in supporting large block devices, but size_t should not be one of them.




----------------------------------------------------------------
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