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 2021/03/15 22:16:07 UTC
[GitHub] [incubator-nuttx] gustavonihei opened a new pull request #3070: fs/mmap: Return EACCES for incompatible file descriptors
gustavonihei opened a new pull request #3070:
URL: https://github.com/apache/incubator-nuttx/pull/3070
## Summary
This PR intends to improve `mmap` compliance to OpenGroup specification.
The standard expects that `mmap` return EACCES error code when a given file descriptor lacks the proper permission.
## Impact
`mmap` performs a more strict validation of input parameters. So there will be an impact to application that relied on this not-compliant behavior.
## Testing
Some LTP tests for `mmap` are now passing.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3070: fs/mmap: Return EACCES for incompatible file descriptors
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3070:
URL: https://github.com/apache/incubator-nuttx/pull/3070#discussion_r594823722
##########
File path: fs/mmap/fs_mmap.c
##########
@@ -142,6 +148,30 @@ FAR void *mmap(FAR void *start, size_t length, int prot, int flags,
}
#endif
+ if (fs_getfilep(fd, &filep) < 0)
Review comment:
```suggestion
ret = fs_getfilep(fd, &filep);
if (ret < 0)
{
ferr("ERROR: Invalid file descriptor, fd=%d\n", fd);
goto errout;
}
```
It isn't good to eat the error code.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3070: fs/mmap: Return EACCES for incompatible file descriptors
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3070:
URL: https://github.com/apache/incubator-nuttx/pull/3070#discussion_r595389644
##########
File path: fs/mmap/fs_mmap.c
##########
@@ -142,6 +148,30 @@ FAR void *mmap(FAR void *start, size_t length, int prot, int flags,
}
#endif
+ if (fs_getfilep(fd, &filep) < 0)
Review comment:
> but caller should expect that the implementation may return the additional errno which not specify in the spec.
I agree as long as it is compatible with the "Returned values" in the documentation, which was not the case for the EAGAIN returned from the `fs_getfilep`.
> it depends on the error category. If it's programming error, we can assert it for debugging purpose. But assert is an abuse for the runtime error and make the thing more worse.
Good, we also agree on this, and that was my motivation for using DEBUGASSERT. Receiving **-EAGAIN** from `fs_getfilep` at `mmap` would be due to a programming error.
If the `mmap` caller were to read **EAGAIN** from errno, the natural handling would be to perform a new `mmap` attempt at a later moment. But, according to the description at `fs_getfilep`, it would never have a different outcome, because it is not a runtime error.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3070: fs/mmap: Return EACCES for incompatible file descriptors
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3070:
URL: https://github.com/apache/incubator-nuttx/pull/3070#discussion_r595389644
##########
File path: fs/mmap/fs_mmap.c
##########
@@ -142,6 +148,30 @@ FAR void *mmap(FAR void *start, size_t length, int prot, int flags,
}
#endif
+ if (fs_getfilep(fd, &filep) < 0)
Review comment:
> but caller should expect that the implementation may return the additional errno which not specify in the spec.
I agree as long as it compatible with the "Returned values" in the documentation, which was not the case for the EAGAIN returned from the `fs_getfilep`.
> it depends on the error category. If it's programming error, we can assert it for debugging purpose. But assert is an abuse for the runtime error and make the thing more worse.
Good, we also agree on this, and that was my motivation for using DEBUGASSERT. Receiving **-EAGAIN** from `fs_getfilep` at `mmap` would be due to a programming error.
If the `mmap` caller were to read **EAGAIN** from errno, the natural handling would be to perform a new `mmap` attempt at a later moment. But, according to the description at `fs_getfilep`, it would never have a different outcome, because it is not a runtime error.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3070: fs/mmap: Return EACCES for incompatible file descriptors
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3070:
URL: https://github.com/apache/incubator-nuttx/pull/3070#discussion_r595389644
##########
File path: fs/mmap/fs_mmap.c
##########
@@ -142,6 +148,30 @@ FAR void *mmap(FAR void *start, size_t length, int prot, int flags,
}
#endif
+ if (fs_getfilep(fd, &filep) < 0)
Review comment:
> but caller should expect that the implementation may return the additional errno which not specify in the spec.
I agree as long as it compatible with the "Returned values" in the documentation, which was not the case for the EAGAIN returned from the `fs_getfilep`.
> it depends on the error category. If it's programming error, we can assert it for debugging purpose. But assert is an abuse for the runtime error and make the thing more worse.
Agreed, and that was my motivation for using DEBUGASSERT. Receiving **-EAGAIN** from `fs_getfilep` at `mmap` would be due to a programming error.
If the `mmap` caller were to read **EAGAIN** from errno, the natural handling would be to perform a new `mmap` attempt at a later moment. But, according to the description at `fs_getfilep`, it would never have a different outcome, because it is not a runtime error.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3070: fs/mmap: Return EACCES for incompatible file descriptors
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3070:
URL: https://github.com/apache/incubator-nuttx/pull/3070#discussion_r595384092
##########
File path: fs/mmap/fs_mmap.c
##########
@@ -142,6 +148,30 @@ FAR void *mmap(FAR void *start, size_t length, int prot, int flags,
}
#endif
+ if (fs_getfilep(fd, &filep) < 0)
Review comment:
but caller should expect that the implementation may return the additional errno which not specify in the spec.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3070: fs/mmap: Return EACCES for incompatible file descriptors
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3070:
URL: https://github.com/apache/incubator-nuttx/pull/3070#discussion_r595095530
##########
File path: fs/mmap/fs_mmap.c
##########
@@ -142,6 +148,30 @@ FAR void *mmap(FAR void *start, size_t length, int prot, int flags,
}
#endif
+ if (fs_getfilep(fd, &filep) < 0)
Review comment:
errno is very useful to identify the failure location without enabling/adding debug log. It make the debug a bit harder if we eat the return code and then return the new fixed error code.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3070: fs/mmap: Return EACCES for incompatible file descriptors
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3070:
URL: https://github.com/apache/incubator-nuttx/pull/3070#discussion_r595389644
##########
File path: fs/mmap/fs_mmap.c
##########
@@ -142,6 +148,30 @@ FAR void *mmap(FAR void *start, size_t length, int prot, int flags,
}
#endif
+ if (fs_getfilep(fd, &filep) < 0)
Review comment:
> but caller should expect that the implementation may return the additional errno which not specify in the spec.
I agree as long as it compatible with the "Returned values" in the documentation, which was not the case for the EAGAIN returned from the `fs_getfilep`.
> it depends on the error category. If it's programming error, we can assert it for debugging purpose. But assert is an abuse for the runtime error and make the thing more worse.
Agreed, and that was my motivation for using DEBUGASSERT. Receiving **-EAGAIN** from `fs_getfilep` at `mmap` would be due to a programming error.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3070: fs/mmap: Return EACCES for incompatible file descriptors
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3070:
URL: https://github.com/apache/incubator-nuttx/pull/3070#discussion_r595389644
##########
File path: fs/mmap/fs_mmap.c
##########
@@ -142,6 +148,30 @@ FAR void *mmap(FAR void *start, size_t length, int prot, int flags,
}
#endif
+ if (fs_getfilep(fd, &filep) < 0)
Review comment:
> but caller should expect that the implementation may return the additional errno which not specify in the spec.
I agree as long as it compatible with the "Returned values" in the documentation, which was not the case for the EAGAIN returned from the `fs_getfilep`.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #3070: fs/mmap: Return EACCES for incompatible file descriptors
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #3070:
URL: https://github.com/apache/incubator-nuttx/pull/3070
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3070: fs/mmap: Return EACCES for incompatible file descriptors
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3070:
URL: https://github.com/apache/incubator-nuttx/pull/3070#discussion_r595387476
##########
File path: fs/mmap/fs_mmap.c
##########
@@ -142,6 +148,30 @@ FAR void *mmap(FAR void *start, size_t length, int prot, int flags,
}
#endif
+ if (fs_getfilep(fd, &filep) < 0)
Review comment:
> I agree, but at the same time we should avoid simply propagating errors upward that are not compatible to the function API.
>
> If you agree that returning EAGAIN is an error that should not happen here and does not make sense to propagate it, we could force it to fail in a DEBUGASSERT.
> This way it won't impact production code and it will help the debugging during development.
> What do you think?
it depends on the error category. If it's programming error, we can assert it for debugging purpose. But assert is an abuse for the runtime error.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3070: fs/mmap: Return EACCES for incompatible file descriptors
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3070:
URL: https://github.com/apache/incubator-nuttx/pull/3070#discussion_r595071043
##########
File path: fs/mmap/fs_mmap.c
##########
@@ -142,6 +148,30 @@ FAR void *mmap(FAR void *start, size_t length, int prot, int flags,
}
#endif
+ if (fs_getfilep(fd, &filep) < 0)
Review comment:
`fs_getfilep` either returns **-EBADF** or **-EAGAIN**.
According to the `mmap` specification, although EAGAIN is a possible return value, it is useful for indicating that a file is currently locked and the API user should try `mmap` again later:
```
EAGAIN The file has been locked, or too much memory has been
locked (see setrlimit(2)).
```
But, `fs_getfilep` returns **-EAGAIN** for a unrecoverable error scenario:
So, I think it does not make sense to simply make `mmap` propagate the **-EAGAIN** error from `fs_getfilep`. Instead, I chose to handle the error and raise a new one that fits to `mmap` API. After all, for the `mmap` API consumer,
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3070: fs/mmap: Return EACCES for incompatible file descriptors
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3070:
URL: https://github.com/apache/incubator-nuttx/pull/3070#discussion_r595071043
##########
File path: fs/mmap/fs_mmap.c
##########
@@ -142,6 +148,30 @@ FAR void *mmap(FAR void *start, size_t length, int prot, int flags,
}
#endif
+ if (fs_getfilep(fd, &filep) < 0)
Review comment:
`fs_getfilep` either returns **-EBADF** or **-EAGAIN**.
According to the `mmap` specification, although EAGAIN is a possible return value, it is useful for indicating that a file is currently locked and the API user should try `mmap` again later.
But, `fs_getfilep` returns **-EAGAIN** for a unrecoverable error scenario:
So, I think it does not make sense to simply make `mmap` propagate the error from `fs_getfilep`. Instead, I chose to handle the error and raise a new one that fits to `mmap` API.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3070: fs/mmap: Return EACCES for incompatible file descriptors
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3070:
URL: https://github.com/apache/incubator-nuttx/pull/3070#discussion_r595387476
##########
File path: fs/mmap/fs_mmap.c
##########
@@ -142,6 +148,30 @@ FAR void *mmap(FAR void *start, size_t length, int prot, int flags,
}
#endif
+ if (fs_getfilep(fd, &filep) < 0)
Review comment:
> I agree, but at the same time we should avoid simply propagating errors upward that are not compatible to the function API.
>
> If you agree that returning EAGAIN is an error that should not happen here and does not make sense to propagate it, we could force it to fail in a DEBUGASSERT.
> This way it won't impact production code and it will help the debugging during development.
> What do you think?
it depends on the error category. If it's programming error, we can assert it for debugging purpose. But assert is an abuse for the runtime error and make the thing more worse.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3070: fs/mmap: Return EACCES for incompatible file descriptors
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3070:
URL: https://github.com/apache/incubator-nuttx/pull/3070#discussion_r595102512
##########
File path: fs/mmap/fs_mmap.c
##########
@@ -142,6 +148,30 @@ FAR void *mmap(FAR void *start, size_t length, int prot, int flags,
}
#endif
+ if (fs_getfilep(fd, &filep) < 0)
Review comment:
I agree, but at the same time we should avoid simply propagating errors upward that are not compatible to the function API.
If you agree that returning EAGAIN is an error that should not happen here and does not make sense to propagate it, we could force it to fail in a DEBUGASSERT.
This way it won't impact production code and it will help the debugging during development.
What do you think?
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3070: fs/mmap: Return EACCES for incompatible file descriptors
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3070:
URL: https://github.com/apache/incubator-nuttx/pull/3070#discussion_r594823722
##########
File path: fs/mmap/fs_mmap.c
##########
@@ -142,6 +148,30 @@ FAR void *mmap(FAR void *start, size_t length, int prot, int flags,
}
#endif
+ if (fs_getfilep(fd, &filep) < 0)
Review comment:
```suggestion
ret = fs_getfilep(fd, &filep);
if (ret < 0)
{
ferr("ERROR: Invalid file descriptor, fd=%d\n", fd);
goto errout;
}
```
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3070: fs/mmap: Return EACCES for incompatible file descriptors
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3070:
URL: https://github.com/apache/incubator-nuttx/pull/3070#discussion_r595114251
##########
File path: fs/mmap/fs_mmap.c
##########
@@ -142,6 +148,30 @@ FAR void *mmap(FAR void *start, size_t length, int prot, int flags,
}
#endif
+ if (fs_getfilep(fd, &filep) < 0)
Review comment:
> errno is very useful to identify the failure location without enabling/adding debug log.
Actually, `errno` is not for debugging purposes, instead it is a error notification method for the consumer of the function API. That is why I insist that we shall only propagate error codes that are compatible to the API. All the other errors should be handled locally, even if the chosen handling is to ignore it.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3070: fs/mmap: Return EACCES for incompatible file descriptors
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3070:
URL: https://github.com/apache/incubator-nuttx/pull/3070#discussion_r595071043
##########
File path: fs/mmap/fs_mmap.c
##########
@@ -142,6 +148,30 @@ FAR void *mmap(FAR void *start, size_t length, int prot, int flags,
}
#endif
+ if (fs_getfilep(fd, &filep) < 0)
Review comment:
`fs_getfilep` either returns **-EBADF** or **-EAGAIN**.
According to the `mmap` specification, although EAGAIN is a possible return value, it is useful for indicating that a file is currently locked and the API user should try `mmap` again later:
```
EAGAIN The file has been locked, or too much memory has been
locked (see setrlimit(2)).
```
But, `fs_getfilep` returns **-EAGAIN** for a unrecoverable error scenario:
So, I think it does not make sense to simply make `mmap` propagate the **-EAGAIN** error from `fs_getfilep`. Instead, I chose to handle the error and raise a new one that fits to `mmap` API.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3070: fs/mmap: Return EACCES for incompatible file descriptors
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3070:
URL: https://github.com/apache/incubator-nuttx/pull/3070#discussion_r595071043
##########
File path: fs/mmap/fs_mmap.c
##########
@@ -142,6 +148,30 @@ FAR void *mmap(FAR void *start, size_t length, int prot, int flags,
}
#endif
+ if (fs_getfilep(fd, &filep) < 0)
Review comment:
`fs_getfilep` either returns **-EBADF** or **-EAGAIN**.
According to the `mmap` specification, although EAGAIN is a possible return value, it is useful for indicating that a file is currently locked and the API user should try `mmap` again later:
```
EAGAIN The file has been locked, or too much memory has been
locked (see setrlimit(2)).
```
But, `fs_getfilep` returns **-EAGAIN** for a unrecoverable error scenario:
So, I think it does not make sense to simply make `mmap` propagate the error from `fs_getfilep`. Instead, I chose to handle the error and raise a new one that fits to `mmap` API. After all, for the `mmap` API consumer,
----------------------------------------------------------------
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