Copilot commented on code in PR #17845:
URL: https://github.com/apache/nuttx/pull/17845#discussion_r2682829971
##########
drivers/mtd/ftl.c:
##########
@@ -561,6 +562,18 @@ static ssize_t ftl_flush(FAR void *priv, FAR const uint8_t
*buffer,
int nbytes;
int ret;
+ if (dev->mtd->erase == NULL && dev->lptable == NULL)
+ {
+ ret = MTD_BWRITE(dev->mtd, startblock, nblocks, buffer);
+ if (ret != nblocks)
+ {
+ ferr("ERROR: Direct write block %" PRIdOFF " failed: %d\n",
+ startblock, ret);
Review Comment:
Inconsistent error handling for partial writes. When MTD_BWRITE returns a
partial write count (a value >= 0 but less than nblocks), this code logs an
error but still returns the partial count. However, the rest of ftl_flush
(e.g., lines 647-650, 685-688, 735-738) returns -EIO for any write that doesn't
complete fully. This inconsistency could lead to different behavior where the
caller might interpret a partial write count as success when it should be
treated as an error.
The code should return a negative error code (like -EIO) when MTD_BWRITE
doesn't write all requested blocks, consistent with the rest of the function.
```suggestion
startblock, ret);
/* Normalize partial writes to an error, consistent with
* the rest of ftl_flush() behavior.
*/
if (ret >= 0)
{
ret = -EIO;
}
```
##########
drivers/mtd/ftl.c:
##########
@@ -409,9 +410,9 @@ static ssize_t ftl_mtd_erase(FAR struct ftl_struct_s *dev,
off_t startblock)
}
ret = MTD_ERASE(dev->mtd, dev->lptable[startblock], 1);
- if (ret == 1)
+ if (ret >= 0 || ret == -ENOSYS)
Review Comment:
The new condition `ret >= 0 || ret == -ENOSYS` is too permissive. It treats
any non-negative return value as success, including 0. However, when erasing 1
block (as specified in the MTD_ERASE call at line 412), a return value of 0
indicates that no blocks were erased, which likely means the block is invalid
or beyond the device boundary. This should trigger bad block handling rather
than being treated as success.
The condition should check for the expected success value: `ret == 1 || ret
== -ENOSYS` to maintain the same strictness as the original code while adding
support for devices without erase capability.
```suggestion
if (ret == 1 || ret == -ENOSYS)
```
##########
drivers/mtd/ftl.c:
##########
@@ -561,6 +562,18 @@ static ssize_t ftl_flush(FAR void *priv, FAR const uint8_t
*buffer,
int nbytes;
int ret;
+ if (dev->mtd->erase == NULL && dev->lptable == NULL)
+ {
+ ret = MTD_BWRITE(dev->mtd, startblock, nblocks, buffer);
+ if (ret != nblocks)
+ {
+ ferr("ERROR: Direct write block %" PRIdOFF " failed: %d\n",
Review Comment:
Incorrect format specifier for ssize_t. The variable `ret` is assigned from
MTD_BWRITE which returns ssize_t, but the error message uses %d format
specifier. This should be %zd to match the ssize_t type and avoid potential
truncation or incorrect output on 64-bit systems.
```suggestion
ssize_t ret;
if (dev->mtd->erase == NULL && dev->lptable == NULL)
{
ret = MTD_BWRITE(dev->mtd, startblock, nblocks, buffer);
if (ret != nblocks)
{
ferr("ERROR: Direct write block %" PRIdOFF " failed: %zd\n",
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
