Changeset 73901 in vbox for trunk

Timestamp:

Aug 27, 2018 12:49:05 AM (6 years ago)

Author:

vboxsync

Message:

ApplianceImplExport.cpp: Fixed scm; added three r=bird todos in related code. bugref:9152

File:

• trunk/src/VBox/Main/src-server/ApplianceImplExport.cpp (modified) (3 diffs)

trunk/src/VBox/Main/src-server/ApplianceImplExport.cpp

@@ -863 +863 @@
                 ++itSkipped;
             }
- 
+
             skipped = vsdescThis->i_findByType(VirtualSystemDescriptionType_HardDiskImage);
             itSkipped = skipped.begin();
@@ -912 +912 @@
     HRESULT rc;
     RT_NOREF(aFormat);
-    try
+    try /** @todo r=bird: This try..catch(HRESULT) stuff here totally messes up anyones
+         * understanding of what i_setUpProgress and createThread() might throw.  You really
+         * think that they must throw some 'HRESULT' stuff too, right. Otherwise, why would
+         * anyone put their invocations inside these try..catch statements?  But the, WTF
+         * do they return status codes?  ARRRRRRRRRRRRRRG!
+         *
+         * Please rewrite this and all similar places to use nested if (SUCCEEDED(hrc)) rather
+         * than unmaintainable code like this.
+         */
     {
         rc = i_setUpProgress(aProgress,
                              BstrFmt(tr("Export appliance '%s'"), aLocInfo.strPath.c_str()),
                              (aLocInfo.storageType == VFSType_File) ? WriteFile : WriteS3);
- 
+/** @todo r=bird: You ignore the status code here... You can simply return if this fails, because
+ * you don't have any state to clean up. */
+
         /* Initialize our worker task */
         TaskOPC* task = NULL;
@@ -926 +936 @@
         catch(...)
         {
+/** @todo r=bird: Task will always be NULL here.  Why? Because the assignment of 'task' is
+ * the very last thing that happens above.  Traditionally, when you 'delete (TaskOPC *)NULL'
+ * the CRT kills your process.  (On windows, though, this may differ depending on whether we're
+ * executing this in a COM worker thread or a IPRT/main worker thread.  We may survive in the
+ * former, but not the latter.)
+ *
+ * P.S. Please make your editor strip trailing spaces on lines you've modified.
+ */
             delete task;
             throw rc = setError(VBOX_E_OBJECT_NOT_FOUND,