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/19 05:53:26 UTC

[GitHub] [incubator-nuttx] anchao opened a new pull request #3102: mm/iob: add iob_free_queue() interface

anchao opened a new pull request #3102:
URL: https://github.com/apache/incubator-nuttx/pull/3102


   ## Summary
   
   mm/iob: rename the iob_free_queue() to iob_destroy_queue()
   mm/iob: add iob_free_queue() interface
   
   make the semantics more precise
   
   ## Impact
   
   N/A
   
   ## Testing
   
   IOB testing


-- 
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] anchao commented on pull request #3102: mm/iob: add iob_free_queue() interface

Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #3102:
URL: https://github.com/apache/incubator-nuttx/pull/3102#issuecomment-856884174


   Hi @antmerlino 
   
   Agree with you, in the new submission, I changed the name of the function to the `iob_free_qentry_from_queue()` you suggested before, please help to review again, Thank you!


-- 
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] anchao commented on a change in pull request #3102: mm/iob: add iob_free_queue() interface

Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #3102:
URL: https://github.com/apache/incubator-nuttx/pull/3102#discussion_r597812156



##########
File path: mm/iob/iob_destroy_queue.c
##########
@@ -0,0 +1,92 @@
+/****************************************************************************
+ * mm/iob/iob_destroy_queue.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <assert.h>
+
+#include <nuttx/mm/iob.h>
+
+#include "iob.h"
+
+#if CONFIG_IOB_NCHAINS > 0
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#ifndef NULL
+#  define NULL ((FAR void *)0)
+#endif
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: iob_destroy_queue
+ *
+ * Description:
+ *   Destroy all I/O buffer chains from the iob queue.
+ *
+ ****************************************************************************/
+
+void iob_destroy_queue(FAR struct iob_queue_s *qhead,
+                       enum iob_user_e producerid)
+{
+  FAR struct iob_qentry_s *iobq;
+  FAR struct iob_qentry_s *nextq;
+  FAR struct iob_s *iob;
+
+  /* Detach the list from the queue head so first for safety (should be safe
+   * anyway).
+   */
+
+  iobq           = qhead->qh_head;
+  qhead->qh_head = NULL;
+
+  /* Remove each I/O buffer chain from the queue */
+
+  while (iobq)

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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3102: mm/iob: add iob_free_queue() interface

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3102:
URL: https://github.com/apache/incubator-nuttx/pull/3102#discussion_r648507230



##########
File path: Documentation/reference/os/iob.rst
##########
@@ -164,6 +164,7 @@ Public Function Prototypes
   - :c:func:`iob_remove_queue()`
   - :c:func:`iob_peek_queue()`
   - :c:func:`iob_free_queue()`
+  - :c:func:`iob_free_qentry_from_queue()`

Review comment:
       @antmerlino and @anchao what about we name the new API iob_free_qentry directly without _from_queue suffix?




-- 
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] anchao commented on pull request #3102: mm/iob: add iob_free_queue() interface

Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #3102:
URL: https://github.com/apache/incubator-nuttx/pull/3102#issuecomment-822272855


   > @xiaoxiang781216 @anchao
   > 
   > I've now spent some more time looking at this and I still think we need to improve the naming. Firstly, free is the better term to use when we are freeing up a resource to be used again. Destroy makes it sound like the resource doesn't exist after, which is not true.
   > 
   > What you have now named iob_free_queue() - the implementation of which did not exist before, finds the given IOB in the queue and removes it, freeing both the IOB chain itself, and the qentry. Therefore, I propose you name this function one of the following:
   > 
   > * iob_free_qentry_from_queue() <- Long but very descriptive
   > * iob_freeone_queue()
   > * iob_queue_freeone()
   > 
   > I would therefore revert the renaming of the original implementation of iob_free_queue(), as I actually think that name was descriptive of what the function did.
   > 
   > Let me know your thoughts - I'm open to other naming but I don't think this change as it stands is very readable.
   
   
   
   > @xiaoxiang781216 @anchao
   > 
   > I've now spent some more time looking at this and I still think we need to improve the naming. Firstly, free is the better term to use when we are freeing up a resource to be used again. Destroy makes it sound like the resource doesn't exist after, which is not true.
   > 
   > What you have now named iob_free_queue() - the implementation of which did not exist before, finds the given IOB in the queue and removes it, freeing both the IOB chain itself, and the qentry. Therefore, I propose you name this function one of the following:
   > 
   > * iob_free_qentry_from_queue() <- Long but very descriptive
   > * iob_freeone_queue()
   > * iob_queue_freeone()
   > 
   > I would therefore revert the renaming of the original implementation of iob_free_queue(), as I actually think that name was descriptive of what the function did.
   > 
   > Let me know your thoughts - I'm open to other naming but I don't think this change as it stands is very readable.
   
   Hi @antmerlino 
   
   Sorry for reply late, thanks for your suggestion, 
   
   the api naming your mentioned seems very detailed and easy to understand, but which out of tune with other iob api naming...
   how about change the iob_free_queue() to iob_release_queue() ? 
   
   ```
   rename: iob_free_queue() -> iob_release_queue()  // enqueue the entire iob/chain entries to freelist
   new   : iob_free_queue()                         // enqueue the specified iob/chain entries to freelist
   ```


-- 
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] anchao commented on pull request #3102: mm/iob: add iob_free_queue() interface

Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #3102:
URL: https://github.com/apache/incubator-nuttx/pull/3102#issuecomment-804542073


   > Can you expand on why this change is necessary? In particular, I'd like to understand what the difference between free and destroy is. It'd be good to document these differences more thoroughly in the code too.
   
   Hi , @antmerlino 
   
   Thanks for review, 
   In the process of TCP / IP stack optimization, I have a requirement to delete an entry from the iob queue list, but the current implementation of IOB does not have this support, look at the API implementation further, I find that the free semantics seems to be more suitable for deleting an entry, and the current free processing is to destroy the entire iob queue. I think it seems that destroy is more suitable for this operations
   


-- 
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 #3102: mm/iob: add iob_free_queue() interface

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3102:
URL: https://github.com/apache/incubator-nuttx/pull/3102#discussion_r597630740



##########
File path: mm/iob/iob_destroy_queue.c
##########
@@ -0,0 +1,92 @@
+/****************************************************************************
+ * mm/iob/iob_destroy_queue.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <assert.h>
+
+#include <nuttx/mm/iob.h>
+
+#include "iob.h"
+
+#if CONFIG_IOB_NCHAINS > 0
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#ifndef NULL
+#  define NULL ((FAR void *)0)
+#endif
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: iob_destroy_queue
+ *
+ * Description:
+ *   Destroy all I/O buffer chains from the iob queue.
+ *
+ ****************************************************************************/
+
+void iob_destroy_queue(FAR struct iob_queue_s *qhead,
+                       enum iob_user_e producerid)
+{
+  FAR struct iob_qentry_s *iobq;
+  FAR struct iob_qentry_s *nextq;
+  FAR struct iob_s *iob;
+
+  /* Detach the list from the queue head so first for safety (should be safe
+   * anyway).
+   */
+
+  iobq           = qhead->qh_head;
+  qhead->qh_head = NULL;
+
+  /* Remove each I/O buffer chain from the queue */
+
+  while (iobq)

Review comment:
       ```suggestion
     while (iobq != NULL)
   ```
   For pointer validation, being more verbose improves readability.




-- 
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 #3102: mm/iob: add iob_free_queue() interface

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3102:
URL: https://github.com/apache/incubator-nuttx/pull/3102#discussion_r648512641



##########
File path: Documentation/reference/os/iob.rst
##########
@@ -164,6 +164,7 @@ Public Function Prototypes
   - :c:func:`iob_remove_queue()`
   - :c:func:`iob_peek_queue()`
   - :c:func:`iob_free_queue()`
+  - :c:func:`iob_free_qentry_from_queue()`

Review comment:
       @anchao and @antmerlino how about name the new function iob_free_queue_qentry?




-- 
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] antmerlino commented on a change in pull request #3102: mm/iob: add iob_free_queue() interface

Posted by GitBox <gi...@apache.org>.
antmerlino commented on a change in pull request #3102:
URL: https://github.com/apache/incubator-nuttx/pull/3102#discussion_r648528457



##########
File path: Documentation/reference/os/iob.rst
##########
@@ -164,6 +164,7 @@ Public Function Prototypes
   - :c:func:`iob_remove_queue()`
   - :c:func:`iob_peek_queue()`
   - :c:func:`iob_free_queue()`
+  - :c:func:`iob_free_qentry_from_queue()`

Review comment:
       @xiaoxiang781216 works for me




-- 
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 #3102: mm/iob: add iob_free_queue() interface

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3102:
URL: https://github.com/apache/incubator-nuttx/pull/3102#discussion_r597630878



##########
File path: mm/iob/iob_destroy_queue.c
##########
@@ -0,0 +1,92 @@
+/****************************************************************************
+ * mm/iob/iob_destroy_queue.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <assert.h>
+
+#include <nuttx/mm/iob.h>
+
+#include "iob.h"
+
+#if CONFIG_IOB_NCHAINS > 0
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#ifndef NULL
+#  define NULL ((FAR void *)0)
+#endif
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: iob_destroy_queue
+ *
+ * Description:
+ *   Destroy all I/O buffer chains from the iob queue.
+ *
+ ****************************************************************************/
+
+void iob_destroy_queue(FAR struct iob_queue_s *qhead,
+                       enum iob_user_e producerid)
+{
+  FAR struct iob_qentry_s *iobq;
+  FAR struct iob_qentry_s *nextq;
+  FAR struct iob_s *iob;
+
+  /* Detach the list from the queue head so first for safety (should be safe
+   * anyway).
+   */
+
+  iobq           = qhead->qh_head;
+  qhead->qh_head = NULL;
+
+  /* Remove each I/O buffer chain from the queue */
+
+  while (iobq)
+    {
+      /* Remove the I/O buffer chain from the head of the queue and
+       * discard the queue container.
+       */
+
+      iob = iobq->qe_head;
+      DEBUGASSERT(iob);

Review comment:
       ```suggestion
         DEBUGASSERT(iob != NULL);
   ```
   For pointer validation, being more verbose improves readability.




-- 
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] antmerlino commented on pull request #3102: mm/iob: add iob_free_queue() interface

Posted by GitBox <gi...@apache.org>.
antmerlino commented on pull request #3102:
URL: https://github.com/apache/incubator-nuttx/pull/3102#issuecomment-803602850


   > 
   > ## Summary
   > 
   > mm/iob: rename the iob_free_queue() to iob_destroy_queue()
   > mm/iob: add iob_free_queue() interface
   > 
   > make the semantics more precise
   > ## Impact
   > 
   > N/A
   > ## Testing
   > 
   > IOB testing
   
   @anchao 
   
   Can you expand on why this change is necessary? In particular, I'd like to understand what the difference between free and destroy is. It'd be good to document these differences more thoroughly in the code too.


-- 
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 pull request #3102: mm/iob: add iob_free_queue() interface

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3102:
URL: https://github.com/apache/incubator-nuttx/pull/3102#issuecomment-805483633


   @antmerlino do you have more prefer name for the new API? or merge this patch as 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] xiaoxiang781216 merged pull request #3102: mm/iob: add iob_free_queue() interface

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #3102:
URL: https://github.com/apache/incubator-nuttx/pull/3102


   


-- 
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] antmerlino commented on pull request #3102: mm/iob: add iob_free_queue() interface

Posted by GitBox <gi...@apache.org>.
antmerlino commented on pull request #3102:
URL: https://github.com/apache/incubator-nuttx/pull/3102#issuecomment-805917456


   @xiaoxiang781216 @anchao 
   
   I've now spent some more time looking at this and I still think we need to improve the naming.  Firstly, free is the better term to use when we are freeing up a resource to be used again. Destroy makes it sound like the resource doesn't exist after, which is not true. 
   
   What you have now named iob_free_queue() - the implementation of which did not exist before, finds the given IOB in the queue and removes it, freeing both the IOB chain itself, and the qentry. Therefore, I propose you name this function one of the following:
   
   - iob_free_qentry_from_queue() <- Long but very descriptive
   - iob_freeone_queue()
   - iob_queue_freeone()
   
   I would therefore revert the renaming of the original implementation of iob_free_queue(), as I actually think that name was descriptive of what the function did.
   
   Let me know your thoughts - I'm open to other naming but I don't think this change as it stands is very readable.


-- 
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