fs/driver: Replace heap allocation of temporary device names #17839
Conversation
jingfei195887
requested review from
Donny9,
pkarashchenko and
xiaoxiang781216
as code owners
github-actions
bot
added
Area: File System
There was a problem hiding this comment.
Pull request overview
This PR refactors the unique device name generation for MTD and block device proxies by replacing heap-allocated device names with stack-allocated local buffers. The change modifies unique_blkdev and unique_chardev functions to accept a buffer parameter instead of returning an allocated string, eliminating the need for heap allocation and deallocation.
Changes:
- Modified
unique_blkdevandunique_chardevto use caller-provided buffers instead of allocating strings - Updated error handling to return integer status codes instead of NULL pointers
- Simplified cleanup logic by removing goto labels and heap deallocation calls
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| fs/driver/fs_mtdproxy.c | Changed unique_blkdev to accept buffer parameter, updated mtd_proxy to use stack-allocated device name array |
| fs/driver/fs_blockproxy.c | Changed unique_chardev to accept buffer parameter, updated block_proxy to use stack-allocated device name (incorrectly left as pointer) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int block_proxy(FAR struct file *filep, FAR const char *blkdev, int oflags) | ||
| { | ||
| struct file temp; | ||
| FAR char *chardev; |
There was a problem hiding this comment.
The variable 'chardev' is declared as a pointer but is never assigned a buffer. This will cause undefined behavior when passed to unique_chardev. This should be changed to a local array like 'char chardev[16]' to match the pattern used in fs_mtdproxy.c.
| FAR char *chardev; | |
| FAR char chardev[16]; |
|
|
||
| chardev = unique_chardev(); | ||
| if (chardev == NULL) | ||
| ret = unique_chardev(chardev, sizeof(chardev)); |
There was a problem hiding this comment.
The sizeof(chardev) will return the size of a pointer (typically 4 or 8 bytes), not the size of a buffer. This will lead to potential buffer overflow when snprintf tries to write the device name. The buffer should be declared as a local array with a fixed size.
| ret = nx_unlink(blkdev); | ||
| if (ret < 0) | ||
| { | ||
| ferr("ERROR: Failed to unlink %s: %d\n", blkdev, ret); | ||
| } | ||
|
|
||
| return ret; |
There was a problem hiding this comment.
The return value logic is incorrect. After successful open_blockdriver at line 177, the function should return OK to indicate success. However, line 190 reassigns ret to the result of nx_unlink. If nx_unlink fails, the function will return an error code even though the main operation (opening the block driver proxy) succeeded. The function should save the success state or simply return OK after logging any unlink errors.
|
|
||
| errout_with_chardev: | ||
| fs_heap_free(chardev); | ||
| return ret; |
There was a problem hiding this comment.
The return value logic is incorrect. When nx_unlink succeeds and returns OK (0), the function will return OK, which is correct. However, when nx_unlink fails, the function will return the error from unlink instead of OK, even though the main operation (opening the proxy) succeeded. The function should return OK on success regardless of the unlink result, and only log errors from unlink.
| return ret; | |
| /* Return success because the main operation (opening the proxy) succeeded, | |
| * regardless of any unlink failure above. | |
| */ | |
| return OK; |
| { | ||
| struct file temp; | ||
| FAR char *chardev; | ||
| bool readonly; |
There was a problem hiding this comment.
The variable 'readonly' is declared but never used in this function. This appears to be dead code that should be removed.
| bool readonly; |
| * | ||
| ****************************************************************************/ |
There was a problem hiding this comment.
The documentation comment is outdated. It still describes the old behavior where the function allocated and returned a device name string that needed to be freed. The comment should be updated to reflect that the function now takes a buffer and size parameters and returns an integer status code.
| * | ||
| ****************************************************************************/ |
There was a problem hiding this comment.
The documentation comment is outdated. It still describes the old behavior where the function allocated and returned a device name string that needed to be freed. The comment should be updated to reflect that the function now takes a buffer and size parameters and returns an integer status code.
jingfei195887
force-pushed
the
optimize_proxy
branch
2 times, most recently
from
14c0f33 to
5e8ccee
Compare
jerpelea
changed the title
To simplify the handling of Block devices and MTD devices, the unique_chardev and unique_blkdev functions now use local variable names instead of allocating device names from the heap. Signed-off-by: jingfei <[email protected]>
jingfei195887
force-pushed
the
optimize_proxy
branch
from
5e8ccee to
2709601
Compare
| ****************************************************************************/ | ||
|
|
||
| static FAR char *unique_blkdev(void) | ||
| static FAR int unique_blkdev(FAR char *devbuf, size_t len) |
| if (ret < 0) | ||
| { | ||
| ferr("ERROR: Failed to dup2%s: %d\n", chardev, ret); | ||
| goto errout_with_bchdev; |
3 participants
Summary
To simplify the handling of Block devices and MTD devices, the unique_chardev and unique_blkdev functions now use local variable names instead of allocating device names from the heap.
Impact
Very minor optimization.
Testing
Run the examples/mtdpart in sim:nsh for test an test is passed.