Changeset 34245 in vbox

Timestamp:

Nov 22, 2010 3:01:00 PM (14 years ago)

Author:

vboxsync

Message:

VBoxServiceToolBox.cpp: r=bird: A bunch of review comments. Only the VBoxServiceToolboxMain is critical. Btw., is it Toolbox like in the API or ToolBox like in the file name?

File:

• trunk/src/VBox/Additions/common/VBoxService/VBoxServiceToolBox.cpp (modified) (13 diffs)

trunk/src/VBox/Additions/common/VBoxService/VBoxServiceToolBox.cpp

@@ -39 +39 @@
 #include "VBoxServiceInternal.h"
 #include "VBoxServiceUtils.h"
+
+/** @todo r=bird: Don't use VBoxServiceError here, just use RTMsg**.  */
+/** @todo r=bird: 'static' is a wonderful keyword, please use it as much as
+ *        possible like I've said in the coding guidelines.  Not only does it
+ *        help wrt to linking, but it also helps understanding what's
+ *        internal and external interfaces in a source file! */
 
 
@@ -124 +130 @@
 
 
+/** @todo r=bird: Again, function headers like this are uslesss and better left
+ *        out. */
 /**
  *
@@ -145 +153 @@
      RTGETOPTSTATE GetState;
      RTGetOptInit(&GetState, argc, argv, s_aOptions, RT_ELEMENTS(s_aOptions), 1, 0);
+     /** @todo r=bird: Pass RTGETOPTINIT_FLAGS_OPTS_FIRST, our mkdir shall be
+      *        like the GNU one wrt to option (dash-something) and argument
+      *        order (dirs). */
 
      int rc = VINF_SUCCESS;
@@ -155 +166 @@
      RTFMODE fileMode = 0;
 #else
-     RTFMODE fileMode = S_IRWXU | S_IRWXG | S_IRWXO;
+     RTFMODE fileMode = S_IRWXU | S_IRWXG | S_IRWXO; /** @todo r=bird: We've got RTFS_ defines for these, they are x-platform.  Why 'file' when we're creating directories? */
 #endif
 
@@ -183 +194 @@
                  if (RT_FAILURE(rc))
                      VBoxServiceError("mkdir: Could not build absolute directory!\n");
+                 /** @todo r=bird: you can make multiple directories just by
+                  *  adding them to the mkdir command line:
+                  *     "mkdir foo/ foo/bar foo/bar/wiz"
+                  *  This will now only create foo/bar/wiz, which will fail
+                  *  because the two previous steps weren't executed.  It
+                  *  will also leak memory, but that's not important. (Also,
+                  *  I don't get why we need to call RTPathAbs here as nobody
+                  *  is going to change the current directory.) */
                  break;
              }
+             /** @todo r=bird: Missing handling of the standard options 'V' and
+              *        'h'. */
 
              default:
@@ -207 +228 @@
 
          if (RT_SUCCESS(rc) && fVerbose)
-             VBoxServiceVerbose(0, "mkdir: Created directory '%s', mode 0x%RTfmode\n", szDir, fileMode);
+             VBoxServiceVerbose(0, "mkdir: Created directory '%s', mode 0x%RTfmode\n", szDir, fileMode); /** @todo r=bird: drop the 0x here, use %#RTfmode instead. */
          else if (RT_FAILURE(rc)) /** @todo Add a switch with more helpful error texts! */
          {
@@ -239 +260 @@
          { "--output",    'o', RTGETOPT_REQ_STRING },
          { "--flags",     'f', RTGETOPT_REQ_STRING }
+     /** @todo r=bird: Missing options 'A', 'b', 'e', 'E', 'n', 's', 'T',
+      *        'u', 'v' as found on 'man cat' on a linux system. They must
+      *        not be implemented, just return an apologetic error message. */
      };
+
 
      int ch;
@@ -250 +275 @@
 
      RTFILE hOutput = NIL_RTFILE;
-     uint32_t ofFlags =   RTFILE_O_CREATE_REPLACE /* Output file flags. */
-                        | RTFILE_O_WRITE
-                        | RTFILE_O_DENY_WRITE;
+     uint32_t fFlags = RTFILE_O_CREATE_REPLACE /* Output file flags. */
+                     | RTFILE_O_WRITE
+                     | RTFILE_O_DENY_WRITE;
 
      while (   (ch = RTGetOpt(&GetState, &ValueUnion))
@@ -260 +285 @@
          switch (ch)
          {
+             /** @todo r=bird: You add a flag --no-content-indexed without a
+              * short form (use a #define CAT_OPT_NO_CONTENT_INDEXED 1000 for
+              * iShort). */
+
              case 'f':
                  /* Process flags; no fancy parsing here yet. */
                  if (RTStrIStr(ValueUnion.psz, "noindex"))
-                     ofFlags |= RTFILE_O_NOT_CONTENT_INDEXED;
+                     fFlags |= RTFILE_O_NOT_CONTENT_INDEXED;
                  else
                  {
@@ -272 +301 @@
 
              case 'o':
-                 rc = RTFileOpen(&hOutput, ValueUnion.psz, ofFlags);
+                 rc = RTFileOpen(&hOutput, ValueUnion.psz, fFlags);
                  if (RT_FAILURE(rc))
                      VBoxServiceError("cat: Could not create output file \"%s\"! rc=%Rrc\n",
                                       ValueUnion.psz, rc);
                  break;
+
+                 /** @todo r=bird: Again, there shall be no need for any -i
+                  *        options since all non-options are input files. */
 
              case 'i':
@@ -289 +321 @@
              }
 
+             /** @todo r=bird: Missing handling of the standard options 'V' and
+              *        'h'. */
+
              default:
                  return RTGetOptPrintError(ch, &ValueUnion);
@@ -315 +350 @@
 int VBoxServiceToolboxMain(int argc, char **argv)
 {
+    /** @todo r=bird: The return type of this function is mixed; both RTEXITCODE
+     *  and IPRT status code.  That doesn't cut it.  The RTEXITCODE part should
+     *  be returned separately from the handled-or-unhandled bit.
+     *
+     *  Also, please change VBoxServiceToolboxCat and VBoxServiceToolboxCat to
+     *  return RTEXITCODE and use RTMsg* like RTZipTarCmd (and later
+     *  RTZipGzipCmd). */
     int rc = VERR_NOT_FOUND;
     if (argc > 0) /* Do we have at least a main command? */
@@ -326 +368 @@
                  || !strcmp(argv[0], "vbox_mkdir"))
         {
-            rc = VBoxServiceToolboxMkDir(argc, argv);
+            rc = VBoxServiceToolboxCat(argc, argv);
         }
     }