You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2023/01/10 19:10:36 UTC
[GitHub] [nuttx] xiaoxiang781216 opened a new pull request, #8075: Minor fs/mmap improvement
xiaoxiang781216 opened a new pull request, #8075:
URL: https://github.com/apache/nuttx/pull/8075
## Summary
- fs/mmap: Remove the duplication rammap handling
- fs/mmap: Support the no backing file for anonymous mapping
- fs/mmap: Suppor the partial unmap for anonymous mapping
## Impact
mmap
## Testing
Pass CI
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pkarashchenko merged pull request #8075: Minor fs/mmap improvement
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged PR #8075:
URL: https://github.com/apache/nuttx/pull/8075
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8075: Minor fs/mmap improvement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8075:
URL: https://github.com/apache/nuttx/pull/8075#discussion_r1066522469
##########
fs/mmap/fs_anonmap.c:
##########
@@ -39,32 +39,69 @@ static int unmap_anonymous(FAR struct task_group_s *group,
FAR void *start,
size_t length)
{
- int ret;
+ FAR void *newaddr;
Review Comment:
But not all toolchain support __attribute__((unused)), so it's better to use UNUSED to suppress the warning on all target.
##########
fs/mmap/fs_anonmap.c:
##########
@@ -39,32 +39,69 @@ static int unmap_anonymous(FAR struct task_group_s *group,
FAR void *start,
size_t length)
{
- int ret;
+ FAR void *newaddr;
Review Comment:
But not all toolchain support __attribute__((unused)), so it's better to use UNUSED to suppress the warning on all targets.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] gustavonihei commented on a diff in pull request #8075: Minor fs/mmap improvement
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on code in PR #8075:
URL: https://github.com/apache/nuttx/pull/8075#discussion_r1066297644
##########
fs/mmap/fs_mmap.c:
##########
@@ -128,45 +105,48 @@ static int file_mmap_(FAR struct file *filep, FAR void *start,
return map_anonymous(&entry, kernel);
}
- if ((flags & MAP_PRIVATE) != 0)
+ if (filep == NULL)
{
-#ifdef CONFIG_FS_RAMMAP
- /* Allocate memory and copy the file into memory. We would, of course,
- * do much better in the KERNEL build using the MMU.
- */
+ return -EBADF;
+ }
- return rammap(filep, &entry, kernel);
-#endif
+ if ((filep->f_oflags & O_WROK) == 0 && prot == PROT_WRITE)
+ {
+ ferr("ERROR: Unsupported options for read-only file descriptor,"
+ "prot=%x flags=%04x\n", prot, flags);
+ return -EACCES;
+ }
+
+ if ((filep->f_oflags & O_RDOK) == 0)
+ {
+ ferr("ERROR: File descriptor does not have read permission\n");
+ return -EACCES;
}
/* Call driver's mmap to get the base address of the file in 'mapped'
* in memory.
*/
- if (filep->f_inode && filep->f_inode->u.i_ops->mmap != NULL)
+ if ((flags & MAP_PRIVATE) == 0 && filep->f_inode &&
+ filep->f_inode->u.i_ops->mmap != NULL)
{
ret = filep->f_inode->u.i_ops->mmap(filep, &entry);
if (ret == OK)
{
- *mapped = (void *)entry.vaddr;
+ *mapped = entry.vaddr;
}
}
else
{
- /* Not directly mappable, probably because the underlying media does
- * not support random access.
+ /* Caller request the privated mapping. Or not directly mappable,
Review Comment:
```suggestion
/* Caller request the private mapping. Or not directly mappable,
```
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] jlaitine commented on a diff in pull request #8075: Minor fs/mmap improvement
Posted by GitBox <gi...@apache.org>.
jlaitine commented on code in PR #8075:
URL: https://github.com/apache/nuttx/pull/8075#discussion_r1068085142
##########
fs/mmap/fs_mmap.c:
##########
@@ -272,11 +252,11 @@ int file_mmap(FAR struct file *filep, FAR void *start, size_t length,
FAR void *mmap(FAR void *start, size_t length, int prot, int flags,
int fd, off_t offset)
{
- FAR struct file *filep;
+ FAR struct file *filep = NULL;
FAR void *mapped;
int ret;
- if (fs_getfilep(fd, &filep) < 0)
+ if (fd != -1 && fs_getfilep(fd, &filep) < 0)
Review Comment:
of course, my bad
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8075: Minor fs/mmap improvement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8075:
URL: https://github.com/apache/nuttx/pull/8075#discussion_r1066523973
##########
fs/mmap/fs_anonmap.c:
##########
@@ -39,32 +39,69 @@ static int unmap_anonymous(FAR struct task_group_s *group,
FAR void *start,
size_t length)
{
- int ret;
+ FAR void *newaddr;
Review Comment:
BTW, I change UNUSED to assignment like:
```
entry->vaddr = newaddr;
```
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] jlaitine commented on a diff in pull request #8075: Minor fs/mmap improvement
Posted by GitBox <gi...@apache.org>.
jlaitine commented on code in PR #8075:
URL: https://github.com/apache/nuttx/pull/8075#discussion_r1067976595
##########
fs/mmap/fs_mmap.c:
##########
@@ -272,11 +252,11 @@ int file_mmap(FAR struct file *filep, FAR void *start, size_t length,
FAR void *mmap(FAR void *start, size_t length, int prot, int flags,
int fd, off_t offset)
{
- FAR struct file *filep;
+ FAR struct file *filep = NULL;
FAR void *mapped;
int ret;
- if (fs_getfilep(fd, &filep) < 0)
+ if (fd != -1 && fs_getfilep(fd, &filep) < 0)
Review Comment:
fd != -1 is not needed, since fs_getfilep returns -EBADF in canse fd is invalid
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] jlaitine commented on a diff in pull request #8075: Minor fs/mmap improvement
Posted by GitBox <gi...@apache.org>.
jlaitine commented on code in PR #8075:
URL: https://github.com/apache/nuttx/pull/8075#discussion_r1067981007
##########
fs/mmap/fs_mmap.c:
##########
@@ -272,11 +252,11 @@ int file_mmap(FAR struct file *filep, FAR void *start, size_t length,
FAR void *mmap(FAR void *start, size_t length, int prot, int flags,
int fd, off_t offset)
{
- FAR struct file *filep;
+ FAR struct file *filep = NULL;
Review Comment:
Initializing here is not needed, since fs_getfilep initializes filep unless it fails, in which case filep is not used.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8075: Minor fs/mmap improvement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8075:
URL: https://github.com/apache/nuttx/pull/8075#discussion_r1068074189
##########
fs/mmap/fs_rammap.c:
##########
@@ -176,7 +175,7 @@ int rammap(FAR struct file *filep, FAR struct mm_map_entry_s *entry,
rdbuffer = kernel ? kmm_malloc(length) : kumm_malloc(length);
if (!rdbuffer)
{
- ferr("ERROR: Region allocation failed, length: %d\n", (int)length);
+ ferr("ERROR: Region allocation failed, length: %zu\n", length);
Review Comment:
yes, since printf is provided by nuttx self. Actually, we just need avoid c99 language feature from compiler, the new feature for library which is already supported by NuttX is fine to use.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8075: Minor fs/mmap improvement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8075:
URL: https://github.com/apache/nuttx/pull/8075#discussion_r1066521324
##########
fs/mmap/fs_mmap.c:
##########
@@ -128,45 +105,48 @@ static int file_mmap_(FAR struct file *filep, FAR void *start,
return map_anonymous(&entry, kernel);
}
- if ((flags & MAP_PRIVATE) != 0)
+ if (filep == NULL)
{
-#ifdef CONFIG_FS_RAMMAP
- /* Allocate memory and copy the file into memory. We would, of course,
- * do much better in the KERNEL build using the MMU.
- */
+ return -EBADF;
+ }
- return rammap(filep, &entry, kernel);
-#endif
+ if ((filep->f_oflags & O_WROK) == 0 && prot == PROT_WRITE)
+ {
+ ferr("ERROR: Unsupported options for read-only file descriptor,"
+ "prot=%x flags=%04x\n", prot, flags);
+ return -EACCES;
+ }
+
+ if ((filep->f_oflags & O_RDOK) == 0)
+ {
+ ferr("ERROR: File descriptor does not have read permission\n");
+ return -EACCES;
}
/* Call driver's mmap to get the base address of the file in 'mapped'
* in memory.
*/
- if (filep->f_inode && filep->f_inode->u.i_ops->mmap != NULL)
+ if ((flags & MAP_PRIVATE) == 0 && filep->f_inode &&
+ filep->f_inode->u.i_ops->mmap != NULL)
{
ret = filep->f_inode->u.i_ops->mmap(filep, &entry);
if (ret == OK)
{
- *mapped = (void *)entry.vaddr;
+ *mapped = entry.vaddr;
}
}
else
{
- /* Not directly mappable, probably because the underlying media does
- * not support random access.
+ /* Caller request the privated mapping. Or not directly mappable,
Review Comment:
Done
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8075: Minor fs/mmap improvement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8075:
URL: https://github.com/apache/nuttx/pull/8075#discussion_r1068074189
##########
fs/mmap/fs_rammap.c:
##########
@@ -176,7 +175,7 @@ int rammap(FAR struct file *filep, FAR struct mm_map_entry_s *entry,
rdbuffer = kernel ? kmm_malloc(length) : kumm_malloc(length);
if (!rdbuffer)
{
- ferr("ERROR: Region allocation failed, length: %d\n", (int)length);
+ ferr("ERROR: Region allocation failed, length: %zu\n", length);
Review Comment:
yes, since printf is provided by nuttx self. Actually, we just need avoid c99 language from compiler, the new feature for library which is already supported by NuttX is fine to use.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8075: Minor fs/mmap improvement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8075:
URL: https://github.com/apache/nuttx/pull/8075#discussion_r1068078518
##########
fs/mmap/fs_mmap.c:
##########
@@ -272,11 +252,11 @@ int file_mmap(FAR struct file *filep, FAR void *start, size_t length,
FAR void *mmap(FAR void *start, size_t length, int prot, int flags,
int fd, off_t offset)
{
- FAR struct file *filep;
+ FAR struct file *filep = NULL;
FAR void *mapped;
int ret;
- if (fs_getfilep(fd, &filep) < 0)
+ if (fd != -1 && fs_getfilep(fd, &filep) < 0)
Review Comment:
But, we need support instead return -EBADF:
ptr = mmap(NULL, length, PROT_WRITEt, MAP_ANONYMOUS, -1, 0);
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] gustavonihei commented on a diff in pull request #8075: Minor fs/mmap improvement
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on code in PR #8075:
URL: https://github.com/apache/nuttx/pull/8075#discussion_r1066296367
##########
fs/mmap/fs_anonmap.c:
##########
@@ -39,32 +39,69 @@ static int unmap_anonymous(FAR struct task_group_s *group,
FAR void *start,
size_t length)
{
- int ret;
+ FAR void *newaddr;
Review Comment:
```suggestion
FAR void *newaddr unused_data;
```
nit: An alternative to the `UNUSED(newaddr)` in line 103 is the `unused` attribute, which indicates that the variable might not be used, preventing a compiler warning.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8075: Minor fs/mmap improvement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8075:
URL: https://github.com/apache/nuttx/pull/8075#discussion_r1068081939
##########
fs/mmap/fs_mmap.c:
##########
@@ -272,11 +252,11 @@ int file_mmap(FAR struct file *filep, FAR void *start, size_t length,
FAR void *mmap(FAR void *start, size_t length, int prot, int flags,
int fd, off_t offset)
{
- FAR struct file *filep;
+ FAR struct file *filep = NULL;
Review Comment:
but the change want to continue in case fd equal -1
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] jlaitine commented on a diff in pull request #8075: Minor fs/mmap improvement
Posted by GitBox <gi...@apache.org>.
jlaitine commented on code in PR #8075:
URL: https://github.com/apache/nuttx/pull/8075#discussion_r1067970433
##########
fs/mmap/fs_rammap.c:
##########
@@ -176,7 +175,7 @@ int rammap(FAR struct file *filep, FAR struct mm_map_entry_s *entry,
rdbuffer = kernel ? kmm_malloc(length) : kumm_malloc(length);
if (!rdbuffer)
{
- ferr("ERROR: Region allocation failed, length: %d\n", (int)length);
+ ferr("ERROR: Region allocation failed, length: %zu\n", length);
Review Comment:
%z is C99 addition AFAIK, is it ok to use here?
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8075: Minor fs/mmap improvement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8075:
URL: https://github.com/apache/nuttx/pull/8075#discussion_r1066522469
##########
fs/mmap/fs_anonmap.c:
##########
@@ -39,32 +39,69 @@ static int unmap_anonymous(FAR struct task_group_s *group,
FAR void *start,
size_t length)
{
- int ret;
+ FAR void *newaddr;
Review Comment:
But not all toolchain support `__attribute__((unused))`, so it's better to use UNUSED to suppress the warning on all targets.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org