Changeset 23332 in vbox for trunk/src/VBox

Timestamp:

Sep 25, 2009 1:26:57 PM (15 years ago)

Author:

vboxsync

Message:

iprt/linux/sysfs.h/cpp: code review, fixing RTLinuxSysFsGetLinkDest[V] termination issue, use RTPathAppend as rtLinuxConstructPath[V] isn't entirely safe, RTLinuxFindDevicePath[V] return value clearification, code style cleanups. Untested.

File:

• trunk/src/VBox/Runtime/r3/linux/sysfs.cpp (modified) (11 diffs)

trunk/src/VBox/Runtime/r3/linux/sysfs.cpp

@@ -50 +50 @@
 #include <errno.h>
 
+/** @todo r=bird: This whole API should be rewritten to use IPRT status codes.
+ *        using errno was a mistake and only results in horrible code. */
+
 
 /**
@@ -56 +59 @@
  *
  * @returns The number of characters returned, or -1 and errno set to ERANGE on
- *        failure.
+ *          failure.
  *
  * @param   pszPrefix  The prefix to prepend if the path is relative.  Must end
@@ -63 +66 @@
  *                     sizeof(@a pszPrefix) characters long
  * @param   cchBuf     The size of the buffer pointed to by @a pszBuf.
- * @param   pszFormat  The name format, either absolute or relative to "/sys/".
+ * @param   pszFormat  The name format, either absolute or relative to the
+ *                     prefix specified by @a pszPrefix.
  * @param   va         The format args.
  */
@@ -74 +78 @@
     AssertReturnStmt(cchBuf > cchPrefix + 1, errno = ERANGE, -1);
 
+    /** @todo While RTStrPrintfV prevents overflows, it doesn't make it easy to
+     *        check for truncations. RTPath should provide some formatters and
+     *        joiners which can take over this rather common task that is
+     *        performed here. */
     size_t cch = RTStrPrintfV(pszBuf, cchBuf, pszFormat, va);
     if (*pszBuf != '/')
@@ -245 +253 @@
         return 0;
 
-    dev_t devNum = 0;
+    dev_t DevNum = 0;
     char szNum[128];
     ssize_t cchNum = RTLinuxSysFsReadStr(fd, szNum, sizeof(szNum));
@@ -266 +274 @@
             {
                 errno = 0;
-                devNum = makedev(u32Maj, u32Min);
+                DevNum = makedev(u32Maj, u32Min);
             }
         }
@@ -274 +282 @@
 
     RTLinuxSysFsClose(fd);
-    return devNum;
+    return DevNum;
 }
 
@@ -282 +290 @@
     va_list va;
     va_start(va, pszFormat);
-    dev_t devNum = RTLinuxSysFsReadDevNumFileV(pszFormat, va);
-    va_end(va);
-    return devNum;
+    dev_t DevNum = RTLinuxSysFsReadDevNumFileV(pszFormat, va);
+    va_end(va);
+    return DevNum;
 }
 
@@ -346 +354 @@
     if (pszName)
     {
-        cchName = strlen(pszName);
+        cchName = strlen(pszName); /* = &szLink[rc] - pszName; */
         if (cchName >= cchBuf)
         {
@@ -352 +360 @@
             return -1;
         }
-        memcpy(pszBuf, pszName, cchName);
+        memcpy(pszBuf, pszName, cchName + 1);
     }
     else
@@ -373 +381 @@
 
 
-static ssize_t rtLinuxFindDevicePathRecursive(dev_t devNum, RTFMODE fMode,
-                                              const char *pszBasePath,
+static ssize_t rtLinuxFindDevicePathRecursive(dev_t DevNum, RTFMODE fMode, const char *pszBasePath,
                                               char *pszBuf, size_t cchBuf)
 {
-    PRTDIR pDir = NULL;
-    RTDIRENTRYEX entry = { {0} };
-    int vrc = VINF_SUCCESS;
-    ssize_t rc;
-
-    vrc = RTDirOpen(&pDir, pszBasePath);
-    if (RT_SUCCESS(vrc))
-        while (true)
+    /*
+     * Check assumptions made by the code below.
+     */
+    size_t const cchBasePath = strlen(pszBasePath);
+    AssertReturnStmt(cchBasePath < RTPATH_MAX - 10U, errno = ENAMETOOLONG, -1);
+
+    ssize_t rcRet;
+    PRTDIR  pDir;
+    int rc = RTDirOpen(&pDir, pszBasePath);
+    if (RT_SUCCESS(rc))
+    {
+        char szPath[RTPATH_MAX]; /** @todo 4K per recursion - can easily be optimized away by passing it along pszBasePath
+                                           and only remember the length. */
+        memcpy(szPath, pszBasePath, cchBasePath + 1);
+
+        for (;;)
         {
-            vrc = RTDirReadEx(pDir, &entry, NULL, RTFSOBJATTRADD_UNIX);
-            if (RT_FAILURE(vrc))
+            RTDIRENTRYEX Entry;
+            rc = RTDirReadEx(pDir, &Entry, NULL, RTFSOBJATTRADD_UNIX);
+            if (RT_FAILURE(rc))
             {
-                errno =   (vrc == VERR_NO_MORE_FILES) ? ENOENT
-                        : (vrc == VERR_BUFFER_OVERFLOW) ? EOVERFLOW
-                        : EIO;
-                rc = -1;
+                errno = rc == VERR_NO_MORE_FILES
+                      ? ENOENT
+                      : rc == VERR_BUFFER_OVERFLOW
+                      ? EOVERFLOW
+                      : EIO;
+                rcRet = -1;
                 break;
             }
-            if (RTFS_IS_SYMLINK(entry.Info.Attr.fMode))
+            if (RTFS_IS_SYMLINK(Entry.Info.Attr.fMode)) /* paranoia */
                 continue;
-            if (   (entry.Info.Attr.u.Unix.Device == devNum)
-                && (   (   (fMode == RTFS_TYPE_DEV_CHAR)
-                        && RTFS_IS_DEV_CHAR(entry.Info.Attr.fMode))
-                    || (   (fMode == RTFS_TYPE_DEV_BLOCK)
-                        && RTFS_IS_DEV_BLOCK(entry.Info.Attr.fMode))))
+
+            /* Do the matching. */
+            if (   Entry.Info.Attr.u.Unix.Device == DevNum
+                && (Entry.Info.Attr.fMode & RTFS_TYPE_MASK) == fMode)
             {
-                rc = rtLinuxConstructPath(pszBuf, cchBuf, pszBasePath, "%s",
-                                          entry.szName);
+                rcRet = rtLinuxConstructPath(pszBuf, cchBuf, pszBasePath, "%s", Entry.szName);
                 break;
             }
-            if (!RTFS_IS_DIRECTORY(entry.Info.Attr.fMode))
+
+            /* Recurse into subdirectories. */
+            if (!RTFS_IS_DIRECTORY(Entry.Info.Attr.fMode))
                 continue;
-            if (entry.szName[0] == '.')
+            if (Entry.szName[0] == '.')
                 continue;
-            char szPath[RTPATH_MAX];
-            /** @todo this is a temporary hack, as RTDirReadEx currently
-             * doesn't know about symbolic links */
-            rc = rtLinuxConstructPath(szPath, sizeof(szPath), pszBasePath,
-                                      "%s", entry.szName);
-            if (rc < 0)
+
+            szPath[cchBasePath] = '\0';
+            rc = RTPathAppend(szPath, sizeof(szPath) - 1, Entry.szName); /* -1: for slash */
+            if (RT_FAILURE(rc))
+            {
+                errno = ENAMETOOLONG;
+                rcRet = -1;
                 break;
+            }
+#if 1 /** @todo This is a temporary hack, as RTDirReadEx in 3.0 doesn't know about symbolic links. */
             struct stat Stat = { 0 };
             if (   lstat(szPath, &Stat) < 0
                 || S_ISLNK(Stat.st_mode))
                 continue;
-            /* @todo ends here */
-            rc = rtLinuxConstructPath(szPath, sizeof(szPath), pszBasePath,
-                                      "%s/", entry.szName);
-            if (rc < 0)
-                break;
-            rc = rtLinuxFindDevicePathRecursive(devNum, fMode, szPath,
-                                                pszBuf, cchBuf);
-            if ((rc >= 0) || errno != ENOENT)
+#endif
+            strcat(&szPath[cchBasePath], "/");
+            rcRet = rtLinuxFindDevicePathRecursive(DevNum, fMode, szPath, pszBuf, cchBuf);
+            if (rcRet >= 0 || errno != ENOENT)
                 break;
         }
+        RTDirClose(pDir);
+    }
     else
     {
-        rc = -1;
-        errno = RTErrConvertToErrno(vrc);
-    }
-    RTDirClose(pDir);
-    return rc;
-}
-
-
-RTDECL(ssize_t) RTLinuxFindDevicePathV(dev_t devNum, RTFMODE fMode,
-                                       char *pszBuf, size_t cchBuf,
+        rcRet = -1;
+        errno = RTErrConvertToErrno(rc);
+    }
+    return rcRet;
+}
+
+
+RTDECL(ssize_t) RTLinuxFindDevicePathV(dev_t DevNum, RTFMODE fMode, char *pszBuf, size_t cchBuf,
                                        const char *pszSuggestion, va_list va)
 {
     AssertReturnStmt(cchBuf >= 2, errno = EINVAL, -1);
-    AssertReturnStmt(   (fMode == RTFS_TYPE_DEV_CHAR)
-                     || (fMode == RTFS_TYPE_DEV_BLOCK),
+    AssertReturnStmt(   fMode == RTFS_TYPE_DEV_CHAR
+                     || fMode == RTFS_TYPE_DEV_BLOCK,
                      errno = EINVAL, -1);
 
-    if (!pszSuggestion)
-        return rtLinuxFindDevicePathRecursive(devNum, fMode, "/dev/",
-                                              pszBuf, cchBuf);
-
-    /*
-     * Construct the filename and read the link.
-     */
-    char szFilename[RTPATH_MAX];
-    int rc = rtLinuxConstructPathV(szFilename, sizeof(szFilename), "/dev/",
-                                   pszSuggestion, va);
-    if (rc == -1)
-        return -1;
-
-    /*
-     * Check whether the caller's suggestion was right.
-     */
-    /** @todo Should we just be using POSIX stat here? */
-    RTFSOBJINFO info = {0};
-    int vrc = RTPathQueryInfo(szFilename, &info, RTFSOBJATTRADD_UNIX);
-    if (RT_FAILURE(vrc))
-    {
-        errno =   (vrc == VERR_PATH_NOT_FOUND) ? ENOENT
-                : (vrc == VERR_FILE_NOT_FOUND) ? ENOENT
-                : EIO;
-        return -1;
-    }
-    if (   (info.Attr.u.Unix.Device == devNum)
-        && (   (   (fMode == RTFS_TYPE_DEV_CHAR)
-                && RTFS_IS_DEV_CHAR(info.Attr.fMode))
-            || (   (fMode == RTFS_TYPE_DEV_BLOCK)
-                && RTFS_IS_DEV_BLOCK(info.Attr.fMode))))
-    {
-        size_t cchPath = strlen(szFilename) + 1;
-        if (cchPath > cchBuf)
+    if (pszSuggestion)
+    {
+        /*
+         * Construct the filename and read the link.
+         */
+        char szFilename[RTPATH_MAX];
+        int rc = rtLinuxConstructPathV(szFilename, sizeof(szFilename), "/dev/", pszSuggestion, va);
+        if (rc == -1)
+            return -1;
+
+        /*
+         * Check whether the caller's suggestion was right.
+         */
+        RTFSOBJINFO Info;
+        rc = RTPathQueryInfo(szFilename, &Info, RTFSOBJATTRADD_UNIX);
+        if (RT_FAILURE(rc))
         {
-            errno = EOVERFLOW;
+/**@todo r=bird: This isn't exactly "tried before searching", it's more like "tried instead of searching"... */
+            errno = rc == VERR_PATH_NOT_FOUND || rc == VERR_FILE_NOT_FOUND
+                  ? ENOENT
+                  : EIO;
             return -1;
         }
-        strcpy(pszBuf, szFilename);
-        return cchPath;
-    }
-
-    /* If the suggestion was wrong, try the brute force method */
-    return rtLinuxFindDevicePathRecursive(devNum, fMode, "/dev/",
-                                          pszBuf, cchBuf);
-}
-
-
-RTDECL(ssize_t) RTLinuxFindDevicePath(dev_t devNum, RTFMODE fMode,
-                                      char *pszBuf, size_t cchBuf,
+        if (   Info.Attr.u.Unix.Device == DevNum
+            && (Info.Attr.fMode & RTFS_TYPE_MASK) == fMode)
+        {
+            size_t cchPath = strlen(szFilename);
+            if (cchPath >= cchBuf)
+            {
+                errno = EOVERFLOW;
+                return -1;
+            }
+            memcpy(pszBuf, szFilename, cchPath + 1);
+            return cchPath;
+        }
+
+        /* The suggestion was wrong, fall back on the brute force attack. */
+    }
+
+    return rtLinuxFindDevicePathRecursive(DevNum, fMode, "/dev/", pszBuf, cchBuf);
+}
+
+
+RTDECL(ssize_t) RTLinuxFindDevicePath(dev_t DevNum, RTFMODE fMode, char *pszBuf, size_t cchBuf,
                                       const char *pszSuggestion, ...)
 {
     va_list va;
     va_start(va, pszSuggestion);
-    int rc = RTLinuxFindDevicePathV(devNum, fMode, pszBuf, cchBuf,
-                                    pszSuggestion, va);
+    int rc = RTLinuxFindDevicePathV(DevNum, fMode, pszBuf, cchBuf, pszSuggestion, va);
     va_end(va);
     return rc;
 }
+