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 2022/03/26 15:21:54 UTC

[GitHub] [incubator-nuttx] Donny9 opened a new pull request #5854: fs/romfs: fix bug about test case:examples/romfs

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


   
   ## Summary
   Fix bug about don't cache symbloic link file.
   
   This bug report by testcase apps/examples/romfs:
   Mounting ROMFS filesystem at target=/usr/local/share with source=/dev/ram1
   Traversing directory: /usr/local/share
     DIRECTORY: /usr/local/share/adir/
   Traversing directory: /usr/local/share/adir
     FILE: /usr/local/share/adir/anotherfile.txt/
     DIRECTORY: /usr/local/share/adir/subdir/
   Traversing directory: /usr/local/share/adir/subdir
     FILE: /usr/local/share/adir/subdir/subdirfile.txt/
   Continuing directory: /usr/local/share/adir
     FILE: /usr/local/share/adir/yafile.txt/
   Continuing directory: /usr/local/share
     FILE: /usr/local/share/afile.txt/
     FILE: /usr/local/share/hfile/
   ERROR: ldir never found
   Finished with  1 errors
   
   Signed-off-by: Jiuzhu Dong <do...@xiaomi.com>
   ## Impact
   fix bug
   ## Testing
   local test
   


-- 
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 #5854: fs/romfs: fix bug about test case:examples/romfs

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



##########
File path: fs/romfs/fs_romfsutil.c
##########
@@ -436,51 +429,44 @@ static int romfs_cachenode(FAR struct romfs_mountpt_s *rm,
           return ret;
         }
 
-      /* Now we are pointing to the real entry of interest. Is it a
-       * directory? Or a file?
-       */
+      ret = romfs_parsefilename(rm, offset, childname);
+      if (ret < 0)
+        {
+          return ret;
+        }
 
-      if (IS_DIRECTORY(next) || IS_FILE(next))
+      if (strcmp(childname, ".") != 0 && strcmp(childname, "..") != 0)
         {
-          ret = romfs_parsefilename(rm, offset, childname);
-          if (ret < 0)
+          if (child == NULL || nodeinfo->rn_count == num - 1)
             {
-              return ret;
-            }
+              FAR void *tmp;
 
-          if (strcmp(childname, ".") != 0 && strcmp(childname, "..") != 0)
-            {
-              if (child == NULL || nodeinfo->rn_count == num - 1)
+              tmp = kmm_realloc(nodeinfo->rn_child,
+                                (num + NODEINFO_NINCR) *
+                                sizeof(FAR struct romfs_nodeinfo_s *));

Review comment:
       @pkarashchenko but FAR is required here, otherwise the sizeof may return value smaller than expected.




-- 
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] pkarashchenko merged pull request #5854: fs/romfs: fix bug about test case:examples/romfs

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


   


-- 
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 #5854: fs/romfs: fix bug about test case:examples/romfs

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



##########
File path: fs/romfs/fs_romfsutil.c
##########
@@ -436,51 +429,44 @@ static int romfs_cachenode(FAR struct romfs_mountpt_s *rm,
           return ret;
         }
 
-      /* Now we are pointing to the real entry of interest. Is it a
-       * directory? Or a file?
-       */
+      ret = romfs_parsefilename(rm, offset, childname);
+      if (ret < 0)
+        {
+          return ret;
+        }
 
-      if (IS_DIRECTORY(next) || IS_FILE(next))
+      if (strcmp(childname, ".") != 0 && strcmp(childname, "..") != 0)
         {
-          ret = romfs_parsefilename(rm, offset, childname);
-          if (ret < 0)
+          if (child == NULL || nodeinfo->rn_count == num - 1)
             {
-              return ret;
-            }
+              FAR void *tmp;
 
-          if (strcmp(childname, ".") != 0 && strcmp(childname, "..") != 0)
-            {
-              if (child == NULL || nodeinfo->rn_count == num - 1)
+              tmp = kmm_realloc(nodeinfo->rn_child,
+                                (num + NODEINFO_NINCR) *
+                                sizeof(FAR struct romfs_nodeinfo_s *));

Review comment:
       but FAR is required here, otherwise the sizeof may return value smaller than expected.




-- 
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] Donny9 commented on a change in pull request #5854: fs/romfs: fix bug about test case:examples/romfs

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



##########
File path: fs/romfs/fs_romfsutil.c
##########
@@ -436,51 +429,44 @@ static int romfs_cachenode(FAR struct romfs_mountpt_s *rm,
           return ret;
         }
 
-      /* Now we are pointing to the real entry of interest. Is it a
-       * directory? Or a file?
-       */
+      ret = romfs_parsefilename(rm, offset, childname);
+      if (ret < 0)
+        {
+          return ret;
+        }
 
-      if (IS_DIRECTORY(next) || IS_FILE(next))
+      if (strcmp(childname, ".") != 0 && strcmp(childname, "..") != 0)
         {
-          ret = romfs_parsefilename(rm, offset, childname);
-          if (ret < 0)
+          if (child == NULL || nodeinfo->rn_count == num - 1)
             {
-              return ret;
-            }
+              FAR void *tmp;
 
-          if (strcmp(childname, ".") != 0 && strcmp(childname, "..") != 0)
-            {
-              if (child == NULL || nodeinfo->rn_count == num - 1)
+              tmp = kmm_realloc(nodeinfo->rn_child,
+                                (num + NODEINFO_NINCR) *
+                                sizeof(FAR struct romfs_nodeinfo_s *));
+              if (tmp == NULL)
                 {
-                  FAR void *tmp;
-
-                  tmp = kmm_realloc(nodeinfo->rn_child,
-                                    (num + NODEINFO_NINCR) *
-                                    sizeof(FAR struct romfs_nodeinfo_s *));
-                  if (tmp == NULL)
-                    {
-                      return -ENOMEM;
-                    }
-
-                  nodeinfo->rn_child = tmp;
-                  memset(nodeinfo->rn_child + num, 0, NODEINFO_NINCR *
-                         sizeof(FAR struct romfs_nodeinfo_s *));
-                  num += NODEINFO_NINCR;
+                  return -ENOMEM;
                 }
 
-              child = &nodeinfo->rn_child[nodeinfo->rn_count++];
-              if (IS_DIRECTORY(next))
-                {
-                  linkoffset = info;
-                }
+              nodeinfo->rn_child = tmp;
+              memset(nodeinfo->rn_child + num, 0, NODEINFO_NINCR *
+                     sizeof(FAR struct romfs_nodeinfo_s *));

Review comment:
       Done
   

##########
File path: fs/romfs/fs_romfsutil.c
##########
@@ -436,51 +429,44 @@ static int romfs_cachenode(FAR struct romfs_mountpt_s *rm,
           return ret;
         }
 
-      /* Now we are pointing to the real entry of interest. Is it a
-       * directory? Or a file?
-       */
+      ret = romfs_parsefilename(rm, offset, childname);
+      if (ret < 0)
+        {
+          return ret;
+        }
 
-      if (IS_DIRECTORY(next) || IS_FILE(next))
+      if (strcmp(childname, ".") != 0 && strcmp(childname, "..") != 0)
         {
-          ret = romfs_parsefilename(rm, offset, childname);
-          if (ret < 0)
+          if (child == NULL || nodeinfo->rn_count == num - 1)
             {
-              return ret;
-            }
+              FAR void *tmp;
 
-          if (strcmp(childname, ".") != 0 && strcmp(childname, "..") != 0)
-            {
-              if (child == NULL || nodeinfo->rn_count == num - 1)
+              tmp = kmm_realloc(nodeinfo->rn_child,
+                                (num + NODEINFO_NINCR) *
+                                sizeof(FAR struct romfs_nodeinfo_s *));

Review comment:
       Done

##########
File path: fs/romfs/fs_romfsutil.c
##########
@@ -108,44 +108,37 @@ static inline int romfs_checkentry(FAR struct romfs_mountpt_s *rm,
       return ret;
     }
 
-  /* Now we are pointing to the real entry of interest. Is it a
-   * directory? Or a file?
+  /* Get the name of the directory entry. */
+
+  ret = romfs_parsefilename(rm, offset, name);
+  if (ret < 0)
+    {
+      return ret;
+    }
+
+  /* Then check if this the name segment we are looking for.  The
+   * string comparison is awkward because there is no terminator
+   * on entryname (there is a terminator on name, however)
    */
 
-  if (IS_DIRECTORY(next) || IS_FILE(next))
+  if (memcmp(entryname, name, entrylen) == 0 &&
+      strlen(name) == entrylen)

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] [incubator-nuttx] Donny9 commented on a change in pull request #5854: fs/romfs: fix bug about test case:examples/romfs

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



##########
File path: fs/romfs/fs_romfsutil.c
##########
@@ -436,51 +429,44 @@ static int romfs_cachenode(FAR struct romfs_mountpt_s *rm,
           return ret;
         }
 
-      /* Now we are pointing to the real entry of interest. Is it a
-       * directory? Or a file?
-       */
+      ret = romfs_parsefilename(rm, offset, childname);
+      if (ret < 0)
+        {
+          return ret;
+        }
 
-      if (IS_DIRECTORY(next) || IS_FILE(next))
+      if (strcmp(childname, ".") != 0 && strcmp(childname, "..") != 0)
         {
-          ret = romfs_parsefilename(rm, offset, childname);
-          if (ret < 0)
+          if (child == NULL || nodeinfo->rn_count == num - 1)
             {
-              return ret;
-            }
+              FAR void *tmp;
 
-          if (strcmp(childname, ".") != 0 && strcmp(childname, "..") != 0)
-            {
-              if (child == NULL || nodeinfo->rn_count == num - 1)
+              tmp = kmm_realloc(nodeinfo->rn_child,
+                                (num + NODEINFO_NINCR) *
+                                sizeof(FAR struct romfs_nodeinfo_s *));
+              if (tmp == NULL)
                 {
-                  FAR void *tmp;
-
-                  tmp = kmm_realloc(nodeinfo->rn_child,
-                                    (num + NODEINFO_NINCR) *
-                                    sizeof(FAR struct romfs_nodeinfo_s *));
-                  if (tmp == NULL)
-                    {
-                      return -ENOMEM;
-                    }
-
-                  nodeinfo->rn_child = tmp;
-                  memset(nodeinfo->rn_child + num, 0, NODEINFO_NINCR *
-                         sizeof(FAR struct romfs_nodeinfo_s *));
-                  num += NODEINFO_NINCR;
+                  return -ENOMEM;
                 }
 
-              child = &nodeinfo->rn_child[nodeinfo->rn_count++];
-              if (IS_DIRECTORY(next))
-                {
-                  linkoffset = info;
-                }
+              nodeinfo->rn_child = tmp;

Review comment:
       void * don't need cast.




-- 
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] pkarashchenko commented on a change in pull request #5854: fs/romfs: fix bug about test case:examples/romfs

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



##########
File path: fs/romfs/fs_romfsutil.c
##########
@@ -436,51 +429,44 @@ static int romfs_cachenode(FAR struct romfs_mountpt_s *rm,
           return ret;
         }
 
-      /* Now we are pointing to the real entry of interest. Is it a
-       * directory? Or a file?
-       */
+      ret = romfs_parsefilename(rm, offset, childname);
+      if (ret < 0)
+        {
+          return ret;
+        }
 
-      if (IS_DIRECTORY(next) || IS_FILE(next))
+      if (strcmp(childname, ".") != 0 && strcmp(childname, "..") != 0)
         {
-          ret = romfs_parsefilename(rm, offset, childname);
-          if (ret < 0)
+          if (child == NULL || nodeinfo->rn_count == num - 1)
             {
-              return ret;
-            }
+              FAR void *tmp;
 
-          if (strcmp(childname, ".") != 0 && strcmp(childname, "..") != 0)
-            {
-              if (child == NULL || nodeinfo->rn_count == num - 1)
+              tmp = kmm_realloc(nodeinfo->rn_child,
+                                (num + NODEINFO_NINCR) *
+                                sizeof(FAR struct romfs_nodeinfo_s *));

Review comment:
       I'm fine just to remove `FAR` from `sizeof` and I can do other changes in a separate PR




-- 
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] pkarashchenko commented on a change in pull request #5854: fs/romfs: fix bug about test case:examples/romfs

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



##########
File path: fs/romfs/fs_romfsutil.c
##########
@@ -436,51 +429,44 @@ static int romfs_cachenode(FAR struct romfs_mountpt_s *rm,
           return ret;
         }
 
-      /* Now we are pointing to the real entry of interest. Is it a
-       * directory? Or a file?
-       */
+      ret = romfs_parsefilename(rm, offset, childname);
+      if (ret < 0)
+        {
+          return ret;
+        }
 
-      if (IS_DIRECTORY(next) || IS_FILE(next))
+      if (strcmp(childname, ".") != 0 && strcmp(childname, "..") != 0)
         {
-          ret = romfs_parsefilename(rm, offset, childname);
-          if (ret < 0)
+          if (child == NULL || nodeinfo->rn_count == num - 1)
             {
-              return ret;
-            }
+              FAR void *tmp;
 
-          if (strcmp(childname, ".") != 0 && strcmp(childname, "..") != 0)
-            {
-              if (child == NULL || nodeinfo->rn_count == num - 1)
+              tmp = kmm_realloc(nodeinfo->rn_child,
+                                (num + NODEINFO_NINCR) *
+                                sizeof(FAR struct romfs_nodeinfo_s *));
+              if (tmp == NULL)
                 {
-                  FAR void *tmp;
-
-                  tmp = kmm_realloc(nodeinfo->rn_child,
-                                    (num + NODEINFO_NINCR) *
-                                    sizeof(FAR struct romfs_nodeinfo_s *));
-                  if (tmp == NULL)
-                    {
-                      return -ENOMEM;
-                    }
-
-                  nodeinfo->rn_child = tmp;
-                  memset(nodeinfo->rn_child + num, 0, NODEINFO_NINCR *
-                         sizeof(FAR struct romfs_nodeinfo_s *));
-                  num += NODEINFO_NINCR;
+                  return -ENOMEM;
                 }
 
-              child = &nodeinfo->rn_child[nodeinfo->rn_count++];
-              if (IS_DIRECTORY(next))
-                {
-                  linkoffset = info;
-                }
+              nodeinfo->rn_child = tmp;
+              memset(nodeinfo->rn_child + num, 0, NODEINFO_NINCR *
+                     sizeof(FAR struct romfs_nodeinfo_s *));

Review comment:
       > @pkarashchenko but FAR is required here, otherwise the sizeof may return value smaller than expected.
   
   Yes. Seems to be my mistake. I will submit a change to fix 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.

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] pkarashchenko commented on a change in pull request #5854: fs/romfs: fix bug about test case:examples/romfs

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



##########
File path: fs/romfs/fs_romfsutil.c
##########
@@ -436,51 +429,44 @@ static int romfs_cachenode(FAR struct romfs_mountpt_s *rm,
           return ret;
         }
 
-      /* Now we are pointing to the real entry of interest. Is it a
-       * directory? Or a file?
-       */
+      ret = romfs_parsefilename(rm, offset, childname);
+      if (ret < 0)
+        {
+          return ret;
+        }
 
-      if (IS_DIRECTORY(next) || IS_FILE(next))
+      if (strcmp(childname, ".") != 0 && strcmp(childname, "..") != 0)
         {
-          ret = romfs_parsefilename(rm, offset, childname);
-          if (ret < 0)
+          if (child == NULL || nodeinfo->rn_count == num - 1)
             {
-              return ret;
-            }
+              FAR void *tmp;
 
-          if (strcmp(childname, ".") != 0 && strcmp(childname, "..") != 0)
-            {
-              if (child == NULL || nodeinfo->rn_count == num - 1)
+              tmp = kmm_realloc(nodeinfo->rn_child,
+                                (num + NODEINFO_NINCR) *
+                                sizeof(FAR struct romfs_nodeinfo_s *));
+              if (tmp == NULL)
                 {
-                  FAR void *tmp;
-
-                  tmp = kmm_realloc(nodeinfo->rn_child,
-                                    (num + NODEINFO_NINCR) *
-                                    sizeof(FAR struct romfs_nodeinfo_s *));
-                  if (tmp == NULL)
-                    {
-                      return -ENOMEM;
-                    }
-
-                  nodeinfo->rn_child = tmp;
-                  memset(nodeinfo->rn_child + num, 0, NODEINFO_NINCR *
-                         sizeof(FAR struct romfs_nodeinfo_s *));
-                  num += NODEINFO_NINCR;
+                  return -ENOMEM;
                 }
 
-              child = &nodeinfo->rn_child[nodeinfo->rn_count++];
-              if (IS_DIRECTORY(next))
-                {
-                  linkoffset = info;
-                }
+              nodeinfo->rn_child = tmp;
+              memset(nodeinfo->rn_child + num, 0, NODEINFO_NINCR *
+                     sizeof(FAR struct romfs_nodeinfo_s *));

Review comment:
       ```suggestion
                        sizeof(struct romfs_nodeinfo_s *));
   ```

##########
File path: fs/romfs/fs_romfsutil.c
##########
@@ -436,51 +429,44 @@ static int romfs_cachenode(FAR struct romfs_mountpt_s *rm,
           return ret;
         }
 
-      /* Now we are pointing to the real entry of interest. Is it a
-       * directory? Or a file?
-       */
+      ret = romfs_parsefilename(rm, offset, childname);
+      if (ret < 0)
+        {
+          return ret;
+        }
 
-      if (IS_DIRECTORY(next) || IS_FILE(next))
+      if (strcmp(childname, ".") != 0 && strcmp(childname, "..") != 0)
         {
-          ret = romfs_parsefilename(rm, offset, childname);
-          if (ret < 0)
+          if (child == NULL || nodeinfo->rn_count == num - 1)
             {
-              return ret;
-            }
+              FAR void *tmp;
 
-          if (strcmp(childname, ".") != 0 && strcmp(childname, "..") != 0)
-            {
-              if (child == NULL || nodeinfo->rn_count == num - 1)
+              tmp = kmm_realloc(nodeinfo->rn_child,
+                                (num + NODEINFO_NINCR) *
+                                sizeof(FAR struct romfs_nodeinfo_s *));
+              if (tmp == NULL)
                 {
-                  FAR void *tmp;
-
-                  tmp = kmm_realloc(nodeinfo->rn_child,
-                                    (num + NODEINFO_NINCR) *
-                                    sizeof(FAR struct romfs_nodeinfo_s *));
-                  if (tmp == NULL)
-                    {
-                      return -ENOMEM;
-                    }
-
-                  nodeinfo->rn_child = tmp;
-                  memset(nodeinfo->rn_child + num, 0, NODEINFO_NINCR *
-                         sizeof(FAR struct romfs_nodeinfo_s *));
-                  num += NODEINFO_NINCR;
+                  return -ENOMEM;
                 }
 
-              child = &nodeinfo->rn_child[nodeinfo->rn_count++];
-              if (IS_DIRECTORY(next))
-                {
-                  linkoffset = info;
-                }
+              nodeinfo->rn_child = tmp;

Review comment:
       ```suggestion
                 nodeinfo->rn_child = (FAR struct romfs_nodeinfo_s **)tmp;
   ```

##########
File path: fs/romfs/fs_romfsutil.c
##########
@@ -108,44 +108,37 @@ static inline int romfs_checkentry(FAR struct romfs_mountpt_s *rm,
       return ret;
     }
 
-  /* Now we are pointing to the real entry of interest. Is it a
-   * directory? Or a file?
+  /* Get the name of the directory entry. */
+
+  ret = romfs_parsefilename(rm, offset, name);
+  if (ret < 0)
+    {
+      return ret;
+    }
+
+  /* Then check if this the name segment we are looking for.  The
+   * string comparison is awkward because there is no terminator
+   * on entryname (there is a terminator on name, however)
    */
 
-  if (IS_DIRECTORY(next) || IS_FILE(next))
+  if (memcmp(entryname, name, entrylen) == 0 &&
+      strlen(name) == entrylen)

Review comment:
       ```suggestion
     if (strlen(name) == entrylen &&
         memcmp(entryname, name, entrylen) == 0)
   ```

##########
File path: fs/romfs/fs_romfsutil.c
##########
@@ -436,51 +429,44 @@ static int romfs_cachenode(FAR struct romfs_mountpt_s *rm,
           return ret;
         }
 
-      /* Now we are pointing to the real entry of interest. Is it a
-       * directory? Or a file?
-       */
+      ret = romfs_parsefilename(rm, offset, childname);
+      if (ret < 0)
+        {
+          return ret;
+        }
 
-      if (IS_DIRECTORY(next) || IS_FILE(next))
+      if (strcmp(childname, ".") != 0 && strcmp(childname, "..") != 0)
         {
-          ret = romfs_parsefilename(rm, offset, childname);
-          if (ret < 0)
+          if (child == NULL || nodeinfo->rn_count == num - 1)
             {
-              return ret;
-            }
+              FAR void *tmp;
 
-          if (strcmp(childname, ".") != 0 && strcmp(childname, "..") != 0)
-            {
-              if (child == NULL || nodeinfo->rn_count == num - 1)
+              tmp = kmm_realloc(nodeinfo->rn_child,
+                                (num + NODEINFO_NINCR) *
+                                sizeof(FAR struct romfs_nodeinfo_s *));

Review comment:
       ```suggestion
                                   sizeof(struct romfs_nodeinfo_s *));
   ```
   actually we can replace it even with `void *`, but change to be `sizeof(*nodeinfo->rn_child)` seems to be even more reasonable.




-- 
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] pkarashchenko commented on a change in pull request #5854: fs/romfs: fix bug about test case:examples/romfs

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



##########
File path: fs/romfs/fs_romfsutil.c
##########
@@ -436,51 +429,44 @@ static int romfs_cachenode(FAR struct romfs_mountpt_s *rm,
           return ret;
         }
 
-      /* Now we are pointing to the real entry of interest. Is it a
-       * directory? Or a file?
-       */
+      ret = romfs_parsefilename(rm, offset, childname);
+      if (ret < 0)
+        {
+          return ret;
+        }
 
-      if (IS_DIRECTORY(next) || IS_FILE(next))
+      if (strcmp(childname, ".") != 0 && strcmp(childname, "..") != 0)
         {
-          ret = romfs_parsefilename(rm, offset, childname);
-          if (ret < 0)
+          if (child == NULL || nodeinfo->rn_count == num - 1)
             {
-              return ret;
-            }
+              FAR void *tmp;
 
-          if (strcmp(childname, ".") != 0 && strcmp(childname, "..") != 0)
-            {
-              if (child == NULL || nodeinfo->rn_count == num - 1)
+              tmp = kmm_realloc(nodeinfo->rn_child,
+                                (num + NODEINFO_NINCR) *
+                                sizeof(FAR struct romfs_nodeinfo_s *));
+              if (tmp == NULL)
                 {
-                  FAR void *tmp;
-
-                  tmp = kmm_realloc(nodeinfo->rn_child,
-                                    (num + NODEINFO_NINCR) *
-                                    sizeof(FAR struct romfs_nodeinfo_s *));
-                  if (tmp == NULL)
-                    {
-                      return -ENOMEM;
-                    }
-
-                  nodeinfo->rn_child = tmp;
-                  memset(nodeinfo->rn_child + num, 0, NODEINFO_NINCR *
-                         sizeof(FAR struct romfs_nodeinfo_s *));
-                  num += NODEINFO_NINCR;
+                  return -ENOMEM;
                 }
 
-              child = &nodeinfo->rn_child[nodeinfo->rn_count++];
-              if (IS_DIRECTORY(next))
-                {
-                  linkoffset = info;
-                }
+              nodeinfo->rn_child = tmp;

Review comment:
       ok. I just checked the code and we mix if both with cast and without cast (I mean cases when return of `malloc`, `zalloc`, `realloc` and others is assigned to a non-void pointer).
   I'm fine to keep it as it is now.




-- 
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 #5854: fs/romfs: fix bug about test case:examples/romfs

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



##########
File path: fs/romfs/fs_romfsutil.c
##########
@@ -436,51 +429,44 @@ static int romfs_cachenode(FAR struct romfs_mountpt_s *rm,
           return ret;
         }
 
-      /* Now we are pointing to the real entry of interest. Is it a
-       * directory? Or a file?
-       */
+      ret = romfs_parsefilename(rm, offset, childname);
+      if (ret < 0)
+        {
+          return ret;
+        }
 
-      if (IS_DIRECTORY(next) || IS_FILE(next))
+      if (strcmp(childname, ".") != 0 && strcmp(childname, "..") != 0)
         {
-          ret = romfs_parsefilename(rm, offset, childname);
-          if (ret < 0)
+          if (child == NULL || nodeinfo->rn_count == num - 1)
             {
-              return ret;
-            }
+              FAR void *tmp;
 
-          if (strcmp(childname, ".") != 0 && strcmp(childname, "..") != 0)
-            {
-              if (child == NULL || nodeinfo->rn_count == num - 1)
+              tmp = kmm_realloc(nodeinfo->rn_child,
+                                (num + NODEINFO_NINCR) *
+                                sizeof(FAR struct romfs_nodeinfo_s *));
+              if (tmp == NULL)
                 {
-                  FAR void *tmp;
-
-                  tmp = kmm_realloc(nodeinfo->rn_child,
-                                    (num + NODEINFO_NINCR) *
-                                    sizeof(FAR struct romfs_nodeinfo_s *));
-                  if (tmp == NULL)
-                    {
-                      return -ENOMEM;
-                    }
-
-                  nodeinfo->rn_child = tmp;
-                  memset(nodeinfo->rn_child + num, 0, NODEINFO_NINCR *
-                         sizeof(FAR struct romfs_nodeinfo_s *));
-                  num += NODEINFO_NINCR;
+                  return -ENOMEM;
                 }
 
-              child = &nodeinfo->rn_child[nodeinfo->rn_count++];
-              if (IS_DIRECTORY(next))
-                {
-                  linkoffset = info;
-                }
+              nodeinfo->rn_child = tmp;
+              memset(nodeinfo->rn_child + num, 0, NODEINFO_NINCR *
+                     sizeof(FAR struct romfs_nodeinfo_s *));

Review comment:
       @pkarashchenko but FAR is required here, otherwise the sizeof may return value smaller than expected.




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