codebje commented on a change in pull request #2861:
URL: https://github.com/apache/incubator-nuttx/pull/2861#discussion_r578358796
##########
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:
Looking at `struct block_operations`:
```c
struct block_operations
{
int (*open)(FAR struct inode *inode);
int (*close)(FAR struct inode *inode);
ssize_t (*read)(FAR struct inode *inode, FAR unsigned char *buffer,
size_t start_sector, unsigned int nsectors);
ssize_t (*write)(FAR struct inode *inode, FAR const unsigned char *buffer,
size_t start_sector, unsigned int nsectors);
int (*geometry)(FAR struct inode *inode, FAR struct geometry
*geometry);
int (*ioctl)(FAR struct inode *inode, int cmd, unsigned long arg);
#ifndef CONFIG_DISABLE_PSEUDOFS_OPERATIONS
int (*unlink)(FAR struct inode *inode);
#endif
};
```
The `start_sector` type for `read` and `write` needs to change, but I don't
believe it's necessary to change the type of `nsectors`. I think all the call
sites for `read` are:
- `fs/fat/fs_fat32util.c` in `fat_hwread` - the maximum for `nsectors` is
the sectors per cluster, at most 128.
- `fs/partition/fs_ptable.c` in `read_partition_block` - the maximum for
`nsectors` is `SIZE_MAX`
- `fs/driver/fs_partition.c` in `part_read` passes its own identical
argument on unchanged
- `fs/procfs/fs_procfs.c` in `procfs_read` uses a `size_t` argument
- `fs/romfs/fs_romfsutil.c` in `romfs_hwread` also has a maximum of
`SIZE_MAX` for its argument
- `fs/littlefs/lfs_vfs.c` in `littlefs_read_block` uses `lfs_size_t` that I
cannot find a typedef for
It's a similar set for `write`.
The partition code also uses `size_t` for sector counts - `struct
partition_state_s` in `fs/partition/partition.h`.
My plan, then, is to change the type of `geo_nsectors` to `blkcnt_t` and the
type of `geo_sectorsize` to `blksize_t`, also making the same type changes to
`partition_state_s`, and to change the type of `start_sector` in `read` and
`write` of `block_operations` to `blkcnt_t`, updating the various drivers that
implement those methods.
How does that sound?
----------------------------------------------------------------
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:
us...@infra.apache.org
