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/10/19 11:35:45 UTC
[GitHub] [incubator-nuttx] anjiahao1 opened a new pull request #4698: mq_open: add long file name check and parameter check
anjiahao1 opened a new pull request #4698:
URL: https://github.com/apache/incubator-nuttx/pull/4698
## Summary
add long file name check and parameter check
## Impact
## Testing
dailytest
--
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] [incubator-nuttx] gustavonihei commented on a change in pull request #4698: mq_open: add long file name check and parameter check
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4698:
URL: https://github.com/apache/incubator-nuttx/pull/4698#discussion_r732725894
##########
File path: include/nuttx/mqueue.h
##########
@@ -385,14 +385,21 @@ void nxmq_free_msgq(FAR struct mqueue_inode_s *msgq);
* attr - The mq_maxmsg attribute is used at the time that the message
* queue is created to determine the maximum number of
* messages that may be placed in the message queue.
+ * pmsgq - This parameter is a address of a pointer
Review comment:
```suggestion
* pmsgq - This parameter is a address of a pointer
```
nit: alignment to the dash of the previous argument
--
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] [incubator-nuttx] gustavonihei commented on a change in pull request #4698: mq_open: add long file name check and parameter check
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4698:
URL: https://github.com/apache/incubator-nuttx/pull/4698#discussion_r732120751
##########
File path: sched/mqueue/mq_msgqalloc.c
##########
@@ -49,50 +49,58 @@
* attr - The mq_maxmsg attribute is used at the time that the message
* queue is created to determine the maximum number of
* messages that may be placed in the message queue.
+ * msgq - This parameter is a address of a pointer
*
* Returned Value:
- * The allocated and initialized message queue structure or NULL in the
- * event of a failure.
+ * IF the function runs successfully，will return Zero(OK),else will
+ * return a error code.
+ *
+ * EINVAL attr is a null pointer and attr->mq_mqssize or attr->mq_maxmsg
+ * is an invalid value.
+ * ENOMEM No memery to alloc.
*
****************************************************************************/
-FAR struct mqueue_inode_s *nxmq_alloc_msgq(FAR struct mq_attr *attr)
+int nxmq_alloc_msgq(FAR struct mq_attr *attr,
+ FAR struct mqueue_inode_s **msgq)
{
- FAR struct mqueue_inode_s *msgq;
Review comment:
Instead of manipulation the function argument throughout the its body, how about keeping the code as it is and just copying the local variable pointer to the newly allocated `struct mqueue_inode_s` before returning?
I think it will result in a much cleaner 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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4698: mq_open: add long file name check and parameter check
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4698:
URL: https://github.com/apache/incubator-nuttx/pull/4698#discussion_r732127474
##########
File path: sched/mqueue/mq_msgqalloc.c
##########
@@ -49,50 +49,58 @@
* attr - The mq_maxmsg attribute is used at the time that the message
* queue is created to determine the maximum number of
* messages that may be placed in the message queue.
+ * msgq - This parameter is a address of a pointer
*
* Returned Value:
- * The allocated and initialized message queue structure or NULL in the
- * event of a failure.
+ * IF the function runs successfully，will return Zero(OK),else will
+ * return a error code.
+ *
+ * EINVAL attr is a null pointer and attr->mq_mqssize or attr->mq_maxmsg
+ * is an invalid value.
+ * ENOMEM No memery to alloc.
Review comment:
```suggestion
* EINVAL attr is NULL or either attr->mq_mqssize or attr->mq_maxmsg
* have an invalid value
* ENOMEM Failed to allocate in-memory resources for the operation
```
--
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] [incubator-nuttx] gustavonihei commented on a change in pull request #4698: mq_open: add long file name check and parameter check
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4698:
URL: https://github.com/apache/incubator-nuttx/pull/4698#discussion_r732125121
##########
File path: sched/mqueue/mq_msgqalloc.c
##########
@@ -49,50 +49,58 @@
* attr - The mq_maxmsg attribute is used at the time that the message
* queue is created to determine the maximum number of
* messages that may be placed in the message queue.
+ * msgq - This parameter is a address of a pointer
*
* Returned Value:
- * The allocated and initialized message queue structure or NULL in the
- * event of a failure.
+ * IF the function runs successfully，will return Zero(OK),else will
+ * return a error code.
Review comment:
```suggestion
* Zero (OK) is returned on success. Otherwise, a negated errno value is
* returned to indicate the nature of the failure.
```
This phrase is widespread in NuttX codebase, how about reusing it to improve the consistency?
--
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] [incubator-nuttx] gustavonihei commented on a change in pull request #4698: mq_open: add long file name check and parameter check
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4698:
URL: https://github.com/apache/incubator-nuttx/pull/4698#discussion_r732120751
##########
File path: sched/mqueue/mq_msgqalloc.c
##########
@@ -49,50 +49,58 @@
* attr - The mq_maxmsg attribute is used at the time that the message
* queue is created to determine the maximum number of
* messages that may be placed in the message queue.
+ * msgq - This parameter is a address of a pointer
*
* Returned Value:
- * The allocated and initialized message queue structure or NULL in the
- * event of a failure.
+ * IF the function runs successfully，will return Zero(OK),else will
+ * return a error code.
+ *
+ * EINVAL attr is a null pointer and attr->mq_mqssize or attr->mq_maxmsg
+ * is an invalid value.
+ * ENOMEM No memery to alloc.
*
****************************************************************************/
-FAR struct mqueue_inode_s *nxmq_alloc_msgq(FAR struct mq_attr *attr)
+int nxmq_alloc_msgq(FAR struct mq_attr *attr,
+ FAR struct mqueue_inode_s **msgq)
{
- FAR struct mqueue_inode_s *msgq;
Review comment:
Instead of manipulation the function argument throughout the its body, how about keeping the code as it is and just copying the local variable pointer to the newly allocated `struct mqueue_inode_s` before returning?
I think it will result in a much cleaner code. Besides, the diff will also be much shorter.
--
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] [incubator-nuttx] gustavonihei commented on pull request #4698: mq_open: add long file name check and parameter check
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #4698:
URL: https://github.com/apache/incubator-nuttx/pull/4698#issuecomment-947621601
> The description says "add long file name" but I can't find the relevant diff that does that. Seems that the patch here only changes the signature of the internal function.
Indeed, there are no added checking routines besides the NULL-check of the new function output parameter.
Overall, the changes the PR brings seem correct, but I agree with @Ouss4, they seem completely unrelated to the PR description.
--
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] [incubator-nuttx] Ouss4 commented on a change in pull request #4698: mq_open: add long file name check and parameter check
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #4698:
URL: https://github.com/apache/incubator-nuttx/pull/4698#discussion_r732100553
##########
File path: fs/mqueue/mq_open.c
##########
@@ -289,10 +289,9 @@ static int file_mq_vopen(FAR struct file *mq, FAR const char *mq_name,
* be created with a reference count of zero.
*/
- msgq = (FAR struct mqueue_inode_s *)nxmq_alloc_msgq(attr);
- if (!msgq)
+ ret = nxmq_alloc_msgq(attr, &msgq);
+ if (ret < 0)
{
- ret = -ENOSPC;
Review comment:
The return value has changed, was that intentional?
Standards specifies that `ENOSPC` will be returned in case we have insufficient space, now we seem to return `ENOMEM`.
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4698: mq_open: add long file name check and parameter check
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4698:
URL: https://github.com/apache/incubator-nuttx/pull/4698#discussion_r732862848
##########
File path: sched/mqueue/mq_msgqalloc.c
##########
@@ -49,25 +49,31 @@
* attr - The mq_maxmsg attribute is used at the time that the message
* queue is created to determine the maximum number of
* messages that may be placed in the message queue.
+ * pmsgq - This parameter is a address of a pointer
*
* Returned Value:
- * The allocated and initialized message queue structure or NULL in the
- * event of a failure.
+ * Zero (OK) is returned on success. Otherwise, a negated errno value is
+ * returned to indicate the nature of the failure.
+ *
+ * EINVAL attr is NULL or either attr->mq_mqssize or attr->mq_maxmsg
+ * have an invalid value
+ * ENOSPC There is insufficient space for the creation of the new
+ * message queue
*
****************************************************************************/
-FAR struct mqueue_inode_s *nxmq_alloc_msgq(FAR struct mq_attr *attr)
+int nxmq_alloc_msgq(FAR struct mq_attr *attr,
+ FAR struct mqueue_inode_s **pmsgq)
{
FAR struct mqueue_inode_s *msgq;
-
Review comment:
revert this change
--
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] [incubator-nuttx] gustavonihei commented on a change in pull request #4698: mq_open: add long file name check and parameter check
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4698:
URL: https://github.com/apache/incubator-nuttx/pull/4698#discussion_r732120751
##########
File path: sched/mqueue/mq_msgqalloc.c
##########
@@ -49,50 +49,58 @@
* attr - The mq_maxmsg attribute is used at the time that the message
* queue is created to determine the maximum number of
* messages that may be placed in the message queue.
+ * msgq - This parameter is a address of a pointer
*
* Returned Value:
- * The allocated and initialized message queue structure or NULL in the
- * event of a failure.
+ * IF the function runs successfully，will return Zero(OK),else will
+ * return a error code.
+ *
+ * EINVAL attr is a null pointer and attr->mq_mqssize or attr->mq_maxmsg
+ * is an invalid value.
+ * ENOMEM No memery to alloc.
*
****************************************************************************/
-FAR struct mqueue_inode_s *nxmq_alloc_msgq(FAR struct mq_attr *attr)
+int nxmq_alloc_msgq(FAR struct mq_attr *attr,
+ FAR struct mqueue_inode_s **msgq)
{
- FAR struct mqueue_inode_s *msgq;
Review comment:
Instead of manipulating the function argument throughout the its body, how about keeping the code as it is and just copying the local variable pointer to the newly allocated `struct mqueue_inode_s` before returning?
I think it will result in a much cleaner code. Besides, the diff will also be much shorter.
--
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] [incubator-nuttx] gustavonihei commented on a change in pull request #4698: mq_open: add long file name check and parameter check
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4698:
URL: https://github.com/apache/incubator-nuttx/pull/4698#discussion_r732130116
##########
File path: sched/mqueue/mq_msgqalloc.c
##########
@@ -49,50 +49,58 @@
* attr - The mq_maxmsg attribute is used at the time that the message
* queue is created to determine the maximum number of
* messages that may be placed in the message queue.
+ * msgq - This parameter is a address of a pointer
*
* Returned Value:
- * The allocated and initialized message queue structure or NULL in the
- * event of a failure.
+ * IF the function runs successfully，will return Zero(OK),else will
+ * return a error code.
+ *
+ * EINVAL attr is a null pointer and attr->mq_mqssize or attr->mq_maxmsg
+ * is an invalid value.
+ * ENOMEM No memery to alloc.
Review comment:
```suggestion
* EINVAL attr is NULL or either attr->mq_mqssize or attr->mq_maxmsg
* have an invalid value
* ENOSPC There is insufficient space for the creation of the new message queue
```
Considering @Ouss4 comment, changing ENOMEM to ENOSPC with documentation from OpenGroup: https://pubs.opengroup.org/onlinepubs/009695399/functions/mq_open.html
--
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] [incubator-nuttx] gustavonihei commented on a change in pull request #4698: mq_open: add long file name check and parameter check
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4698:
URL: https://github.com/apache/incubator-nuttx/pull/4698#discussion_r732130116
##########
File path: sched/mqueue/mq_msgqalloc.c
##########
@@ -49,50 +49,58 @@
* attr - The mq_maxmsg attribute is used at the time that the message
* queue is created to determine the maximum number of
* messages that may be placed in the message queue.
+ * msgq - This parameter is a address of a pointer
*
* Returned Value:
- * The allocated and initialized message queue structure or NULL in the
- * event of a failure.
+ * IF the function runs successfully，will return Zero(OK),else will
+ * return a error code.
+ *
+ * EINVAL attr is a null pointer and attr->mq_mqssize or attr->mq_maxmsg
+ * is an invalid value.
+ * ENOMEM No memery to alloc.
Review comment:
```suggestion
* EINVAL attr is NULL or either attr->mq_mqssize or attr->mq_maxmsg
* have an invalid value
* ENOSPC There is insufficient space for the creation of the new message queue
```
Considering @Ouss4 comment, changing `ENOMEM` to `ENOSPC` with documentation from OpenGroup: https://pubs.opengroup.org/onlinepubs/009695399/functions/mq_open.html
--
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] [incubator-nuttx] gustavonihei merged pull request #4698: mq_open: add long file name check and parameter check
Posted by GitBox <gi...@apache.org>.
gustavonihei merged pull request #4698:
URL: https://github.com/apache/incubator-nuttx/pull/4698
--
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] [incubator-nuttx] gustavonihei commented on a change in pull request #4698: mq_open: add long file name check and parameter check
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4698:
URL: https://github.com/apache/incubator-nuttx/pull/4698#discussion_r732127474
##########
File path: sched/mqueue/mq_msgqalloc.c
##########
@@ -49,50 +49,58 @@
* attr - The mq_maxmsg attribute is used at the time that the message
* queue is created to determine the maximum number of
* messages that may be placed in the message queue.
+ * msgq - This parameter is a address of a pointer
*
* Returned Value:
- * The allocated and initialized message queue structure or NULL in the
- * event of a failure.
+ * IF the function runs successfully，will return Zero(OK),else will
+ * return a error code.
+ *
+ * EINVAL attr is a null pointer and attr->mq_mqssize or attr->mq_maxmsg
+ * is an invalid value.
+ * ENOMEM No memery to alloc.
Review comment:
```suggestion
* EINVAL attr is NULL or either attr->mq_mqssize or attr->mq_maxmsg
* have an invalid value
* ENOMEM Failed to allocate in-memory resources for the operation
```
Suggestion for improving the documentation
--
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