Changeset 78968 in vbox for trunk/src

Timestamp:

Jun 4, 2019 2:48:23 PM (6 years ago)

Author:

vboxsync

Message:

Main/MachienImplMoveVM.cpp: Some more review comments and a couple of cleanups. bugref:8345

Location:

trunk/src/VBox/Main

Files:

• include/MachineImplMoveVM.h (modified) (2 diffs)
• src-server/MachineImplMoveVM.cpp (modified) (45 diffs)

trunk/src/VBox/Main/include/MachineImplMoveVM.h

@@ -74 +74 @@
         }
 
-        void printItem(bool fLog) const;
+#ifdef DEBUG
+        void logItem() const;
+#endif
 
         HRESULT m_code;
@@ -138 +140 @@
     HRESULT moveAllDisks(const std::map<Utf8Str, MEDIUMTASKMOVE>& listOfDisks, const Utf8Str* strTargetFolder = NULL);
     HRESULT restoreAllDisks(const std::map<Utf8Str, MEDIUMTASKMOVE>& listOfDisks);
-    bool isMediumTypeSupportedForMoving(const ComPtr<IMedium> &pMedium);
+    bool isMediumTypeSupportedForMovingThrowsHresult(const ComPtr<IMedium> &pMedium);
 };
 

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

@@ -128 +128 @@
 
 
-void MachineMoveVM::ErrorInfoItem::printItem(bool fLog) const
+#ifdef DEBUG
+void MachineMoveVM::ErrorInfoItem::logItem() const
 {
-    if (fLog)
-    {
-        Log2(("(The error code is %Rrc): %s\n",m_code, m_description.c_str()));
-    }
-    else
-        RTPrintf("(The error code is %Rrc): %s\n",m_code, m_description.c_str());
+    Log2(("(The error code is %Rrc): %s\n", m_code, m_description.c_str()));
 }
+#endif
 
 HRESULT MachineMoveVM::init()
@@ -149 +146 @@
         size_t len = m_targetPath.length() + 2;
         if (len >= RTPATH_MAX)
-        {
-            return m_pMachine->setError(VBOX_E_IPRT_ERROR,
-                                        m_pMachine->tr(Utf8Str("The destination path exceeds "
-                                                               "the maximum value.").c_str()));
-        }
+            return m_pMachine->setError(VBOX_E_IPRT_ERROR, m_pMachine->tr("The destination path exceeds the maximum value."));
 
         /** @todo r=bird: I need to add a Utf8Str method or iprt/cxx/path.h thingy
@@ -174 +167 @@
      */
 
-    try
+    try /** @todo r=bird: What's the point of the try/catch here now? */
     {
         RTFOFF cbTotal = 0;
@@ -183 +176 @@
         int vrc = RTFsQuerySizes(strTargetFolder.c_str(), &cbTotal, &cbFree, &cbBlock, &cbSector);
         if (FAILED(vrc))
-        {
-            return m_pMachine->setErrorBoth(E_FAIL, vrc,
-                                            m_pMachine->tr(Utf8StrFmt("Unable to move machine. Can't get "
-                                                                      "the destination storage size (%s)",
-                                                                      strTargetFolder.c_str()).c_str()));
-        }
+            return m_pMachine->setErrorBoth(VBOX_E_IPRT_ERROR, vrc,
+                                            m_pMachine->tr("Unable to determine free space at move destination ('%s'): %Rrc"),
+                                            strTargetFolder.c_str(), vrc);
 
         RTDIR hDir;
-        Utf8Str strTempFile = strTargetFolder + "test.txt";
         vrc = RTDirOpen(&hDir, strTargetFolder.c_str());
         if (RT_FAILURE(vrc))
             return m_pMachine->setErrorVrc(vrc);
-        else
-        {
-            RTFILE hFile;
-            vrc = RTFileOpen(&hFile, strTempFile.c_str(), RTFILE_O_OPEN_CREATE | RTFILE_O_READWRITE | RTFILE_O_DENY_NONE);
-            if (RT_FAILURE(vrc))
-            {
-                return m_pMachine->setErrorVrc(vrc,
-                                               m_pMachine->tr(Utf8StrFmt("Can't create a test file test.txt in the %s. "
-                                                                         "Check the access rights of the destination "
-                                                                         "folder.", strTargetFolder.c_str()).c_str()));
-            }
-
-            /** @todo r=vvp: Do we need to check each return result here? Looks excessively.
-             *  And it's not so important for the test file. */
-            RTFileClose(hFile);
-            RTFileDelete(strTempFile.c_str());
+
+        Utf8Str strTempFile = strTargetFolder + "test.txt";
+        RTFILE hFile;
+        vrc = RTFileOpen(&hFile, strTempFile.c_str(), RTFILE_O_OPEN_CREATE | RTFILE_O_READWRITE | RTFILE_O_DENY_NONE);
+        if (RT_FAILURE(vrc))
+        {
             RTDirClose(hDir);
-        }
+            return m_pMachine->setErrorVrc(vrc,
+                                           m_pMachine->tr("Can't create a test file test.txt in the %s. Check the access rights of the destination folder."),
+                                           strTargetFolder.c_str());
+        }
+
+        /** @todo r=vvp: Do we need to check each return result here? Looks excessively.
+         *  And it's not so important for the test file.
+         * bird: I'd just do AssertRC on the same line, though the deletion
+         * of the test is a little important. */
+        vrc = RTFileClose(hFile); AssertRC(vrc);
+        RTFileDelete(strTempFile.c_str());
+        vrc = RTDirClose(hDir); AssertRC(vrc);
 
         Log2(("blocks: total %RTfoff, free %RTfoff ", cbTotal, cbFree));
@@ -221 +211 @@
         vrc = RTFsQueryProperties(strTargetFolder.c_str(), &properties);
         if (FAILED(vrc))
-            return m_pMachine->setErrorVrc(vrc);
-
-        Utf8StrFmt info ("disk properties:\n"
-                         "remote: %s \n"
-                         "read only: %s \n"
-                         "compressed: %s \n",
-                         properties.fRemote == true ? "true":"false",
-                         properties.fReadOnly == true ? "true":"false",
-                         properties.fCompressed == true ? "true":"false");
-
-        Log2(("%s \n", info.c_str()));
+            return m_pMachine->setErrorVrc(vrc, "RTFsQueryProperties(%s): %Rrc", strTargetFolder.c_str(), vrc);
+
+        Log2(("disk properties:\n"
+              "remote: %RTbool\n"
+              "read only: %RTbool\n"
+              "compressed: %RTbool\n",
+              properties.fRemote,
+              properties.fReadOnly,
+              properties.fCompressed));
 
         /* Get the original VM path */
@@ -266 +254 @@
         if (   m_type.equals("basic")
             && strLogFolder.contains(strSettingsFilePath))
-        {
             m_vmFolders.insert(std::make_pair(VBox_LogFolder, strLogFolder));
-        }
 
         Utf8Str strStateFilePath;
@@ -275 +261 @@
         hrc = m_pMachine->COMGETTER(State)(&machineState);
         if (FAILED(hrc))
-             return hrc;
+            return hrc;
 
         if (machineState == MachineState_Saved)
@@ -291 +277 @@
         hrc = m_pMachine->COMGETTER(SnapshotFolder)(bstr_snapshotFolder.asOutParam());
         if (FAILED(hrc))
-             return hrc;
+            return hrc;
 
         strSnapshotFolder = bstr_snapshotFolder;
@@ -303 +289 @@
             hrc = m_pMachine->COMGETTER(Id)(bstrSrcMachineId.asOutParam());
             if (FAILED(hrc))
-                 return hrc;
+                return hrc;
 
             ComPtr<IMachine> newSrcMachine;
             hrc = m_pMachine->i_getVirtualBox()->FindMachine(bstrSrcMachineId.raw(), newSrcMachine.asOutParam());
             if (FAILED(hrc))
-                 return hrc;
+                return hrc;
         }
 
@@ -315 +301 @@
          */
 
-        long long neededFreeSpace = 0;
+        int64_t neededFreeSpace = 0;
 
         /* Actual file list */
@@ -329 +315 @@
             hrc = m_pMachine->COMGETTER(SnapshotCount)(&cSnapshots);
             if (FAILED(hrc))
-                 return hrc;
+                return hrc;
 
             if (cSnapshots > 0)
@@ -339 +325 @@
                 hrc = m_pMachine->FindSnapshot(Bstr(id).raw(), pSnapshot.asOutParam());
                 if (FAILED(hrc))
-                     return hrc;
+                    return hrc;
                 hrc = createMachineList(pSnapshot, machineList);
                 if (FAILED(hrc))
-                     return hrc;
+                    return hrc;
             }
         }
@@ -352 +338 @@
         hrc = queryMediasForAllStates(machineList);
         if (FAILED(hrc))
-             return hrc;
-
-        {
+            return hrc;
+
+        { /** @todo r=bird: What're the scopes for?? */
             uint64_t totalMediumsSize = 0;
 
             for (size_t i = 0; i < m_llMedias.size(); ++i)
             {
-                LONG64  cbSize = 0;
                 MEDIUMTASKCHAINMOVE &mtc = m_llMedias.at(i);
                 for (size_t a = mtc.chain.size(); a > 0; --a)
                 {
                     Bstr bstrLocation;
-                    Utf8Str strLocation;
                     Utf8Str name = mtc.chain[a - 1].strBaseName;
                     ComPtr<IMedium> plMedium = mtc.chain[a - 1].pMedium;
                     hrc = plMedium->COMGETTER(Location)(bstrLocation.asOutParam());
                     if (FAILED(hrc))
-                         return hrc;
-
-                    strLocation = bstrLocation;
+                        return hrc;
+
+                    Utf8Str strLocation = bstrLocation;
 
                     /*if an image is located in the actual VM folder it will be added to the actual list */
+                    /** @todo r=bird: This isn't sane wrt unix paths (should
+                     * be startsWith)... And on windows there is the matter of
+                     * two kinds of slashes. */
                     if (strLocation.contains(strSettingsFilePath))
                     {
+                        LONG64 cbSize = 0;
                         hrc = plMedium->COMGETTER(Size)(&cbSize);
                         if (FAILED(hrc))
-                             return hrc;
+                            return hrc;
 
                         std::pair<std::map<Utf8Str, MEDIUMTASKMOVE>::iterator,bool> ret;
@@ -428 +416 @@
                         Log2(("The state file %s wasn't added into the moved list. Couldn't get the file size.\n",
                               name.c_str()));
-                        return m_pMachine->setErrorVrc(vrc);
+                        return m_pMachine->setErrorVrc(vrc,
+                                                       m_pMachine->tr("Failed to get file size for '%s': %Rrc"),
+                                                       name.c_str(), vrc);
                     }
                 }
@@ -448 +438 @@
                     neededFreeSpace += totalLogSize;
                     if (cbFree - neededFreeSpace <= _1M)
-                    {
-                        return m_pMachine->setError(VBOX_E_IPRT_ERROR,
-                                                    m_pMachine->tr(Utf8StrFmt("There is lack of disk space "
-                                                                              "(%Rrc)", VERR_OUT_OF_RESOURCES).c_str()));
-                    }
+                        return m_pMachine->setError(E_FAIL,
+                                                    m_pMachine->tr("Insufficient disk space availble (%RTfoff needed, %RTfoff free)"),
+                                                    neededFreeSpace, cbFree);
 
                     fileList_t filesList;
@@ -475 +463 @@
                         }
                         else
-                            Log2(("The log file %s wasn't added into the moved list. "
-                                  "Couldn't get the file size.\n", strFile.c_str()));
+                            Log2(("The log file %s wasn't added into the moved list. Couldn't get the file size.\n", strFile.c_str()));
                         ++it;
                     }
@@ -496 +483 @@
             LogRel(("but free space on destination is %RTfoff\n", cbFree));
             return m_pMachine->setError(VBOX_E_IPRT_ERROR,
-                                        m_pMachine->tr(Utf8StrFmt ("There is lack of disk space"
-                                                                   " (%Rrc)", VERR_OUT_OF_RESOURCES).c_str()));
+                                        m_pMachine->tr("Insufficient disk space availble (%RTfoff needed, %RTfoff free)"),
+                                        neededFreeSpace, cbFree);
         }
 
@@ -519 +506 @@
                                     1);
             if (FAILED(hrc))
-            {
-                return m_pMachine->setError(VBOX_E_IPRT_ERROR,
-                                            m_pMachine->tr(Utf8StrFmt("Couldn't correctly setup the progress "
-                                                                      "object for moving VM operation (%Rrc)", hrc).c_str()));
-            }
+                return m_pMachine->setError(hrc,
+                                            m_pMachine->tr("Couldn't correctly setup the progress object for moving VM operation"));
         }
 
@@ -693 +677 @@
                 if (RT_FAILURE(vrc))
                 {
+                    /** @todo r=bird: tr() cannot translate formatted messages like this!
+                     * I would suggest you add methods to the task for doing
+                     * both of these things in one go:
+                     *  appendAndSetErrorBothV(HRESULT,int,const char *,va_list)
+                     *  appendAndSetErrorBoth(HRESULT,int,const char *,...)
+                     *  appendAndSetErrorVrc(int,const char *,...)
+                     */
+
                     Utf8StrFmt errorDesc("Could not create snapshots folder '%s' (%Rrc)", strTrgSnapshotFolder.c_str(), vrc);
                     taskMoveVM->m_errorsList.push_back(ErrorInfoItem(VBOX_E_IPRT_ERROR, errorDesc.c_str()));
@@ -709 +701 @@
                 /* Move to next sub-operation. */
                 hrc = taskMoveVM->m_pProgress->SetNextOperation(BstrFmt(machine->tr("Copy the save state file '%s' ..."),
-                                                            RTPathFilename(sst.strSaveStateFile.c_str())).raw(), sst.uWeight);
+                                                                        RTPathFilename(sst.strSaveStateFile.c_str())).raw(),
+                                                                sst.uWeight);
                 if (FAILED(hrc))
                     throw hrc;
@@ -717 +710 @@
                 if (RT_FAILURE(vrc))
                 {
+                    /** @todo r=bird: See first error push_back in MachineMoveVM::i_MoveVMThreadTask */
                     Utf8StrFmt errorDesc("Could not copy state file '%s' to '%s' (%Rrc)",
                                          sst.strSaveStateFile.c_str(), strTrgSaveState.c_str(), vrc);
@@ -755 +749 @@
 
             hrc = taskMoveVM->m_pProgress->SetNextOperation(BstrFmt(machine->tr("Copy Machine settings file '%s' ..."),
-                                                (*machineConfFile).machineUserData.strName.c_str()).raw(), 1);
+                                                                    (*machineConfFile).machineUserData.strName.c_str()).raw(),
+                                                            1);
             if (FAILED(hrc))
                 throw hrc;
@@ -767 +762 @@
                 if (RT_FAILURE(vrc))
                 {
+                    /** @todo r=bird: See first error push_back in MachineMoveVM::i_MoveVMThreadTask */
                     Utf8StrFmt errorDesc("Could not create a home machine folder '%s' (%Rrc)",
                                          strTargetSettingsFilePath.c_str(), vrc);
@@ -795 +791 @@
             if (RT_FAILURE(vrc))
             {
+                /** @todo r=bird: See first error push_back in MachineMoveVM::i_MoveVMThreadTask */
                 Utf8StrFmt errorDesc("Could not copy the setting file '%s' to '%s' (%Rrc)",
                                      strSettingsFilePath.c_str(), strTargetSettingsFilePath.stripFilename().c_str(), vrc);
@@ -802 +799 @@
             }
 
-            Log2(("The setting file %s has been copied into the folder %s\n", strSettingsFilePath.c_str(),
-                        strTargetSettingsFilePath.stripFilename().c_str()));
+            Log2(("The setting file %s has been copied into the folder %s\n",
+                  strSettingsFilePath.c_str(), strTargetSettingsFilePath.stripFilename().c_str()));
 
             /* save new file in case of restoring */
@@ -813 +810 @@
         /* Moving Machine log files */
         {
-            Log2(("Copy machine log files \n"));
+            Log2(("Copy machine log files\n"));
 
             if (taskMoveVM->m_vmFolders[VBox_LogFolder].isNotEmpty())
@@ -829 +826 @@
                         if (RT_FAILURE(vrc))
                         {
+                            /** @todo r=bird: See first error push_back in MachineMoveVM::i_MoveVMThreadTask */
                             Utf8StrFmt errorDesc("Could not create log folder '%s' (%Rrc)",
                                                  strTargetLogFolderPath.c_str(), vrc);
@@ -851 +849 @@
                         /* Move to next sub-operation. */
                         hrc = taskMoveVM->m_pProgress->SetNextOperation(BstrFmt(machine->tr("Copying the log file '%s' ..."),
-                                                                               RTPathFilename(strFullSourceFilePath.c_str())).raw(),
+                                                                                RTPathFilename(strFullSourceFilePath.c_str())).raw(),
                                                                        1);
                         if (FAILED(hrc))
@@ -860 +858 @@
                         if (RT_FAILURE(vrc))
                         {
+                            /** @todo r=bird: See first error push_back in MachineMoveVM::i_MoveVMThreadTask */
                             Utf8StrFmt errorDesc("Could not copy the log file '%s' to '%s' (%Rrc)",
                                                  strFullSourceFilePath.c_str(),
@@ -1143 +1142 @@
                 strTargetImageName.append(RTPATH_DELIMITER).append(strLocation);
                 rc = m_pProgress->SetNextOperation(BstrFmt(machine->tr("Moving medium '%ls' ..."),
-                                                       bstrSrcName.raw()).raw(),
-                                                       mt.uWeight);
+                                                           bstrSrcName.raw()).raw(),
+                                                   mt.uWeight);
                 if (FAILED(rc)) throw rc;
             }
@@ -1151 +1150 @@
                 strTargetImageName = mt.strBaseName;//Should contain full path to the image
                 rc = m_pProgress->SetNextOperation(BstrFmt(machine->tr("Moving medium '%ls' back..."),
-                                                       bstrSrcName.raw()).raw(),
-                                                       mt.uWeight);
+                                                           bstrSrcName.raw()).raw(),
+                                                   mt.uWeight);
                 if (FAILED(rc)) throw rc;
             }
@@ -1277 +1276 @@
     if (RT_SUCCESS(vrc))
     {
+        /** @todo r=bird: RTDIRENTRY is big and this function is doing
+         * unrestrained recursion of arbritrary depth.  Four things:
+         *      - Add a depth counter parameter and refuse to go deeper than
+         *        a certain reasonable limit.
+         *      - Split this method into a main and a worker, placing
+         *        RTDIRENTRY on the stack in the main and passing it onto to
+         *        worker as a parameter.
+         *      - RTDirRead may fail for reasons other than
+         *        VERR_NO_MORE_FILES.  For instance someone could create an
+         *        entry with a name longer than RTDIRENTRY have space to
+         *        store (windows host with UTF-16 encoding shorter than 255
+         *        chars, but UTF-8 encoding longer than 260).
+         *      - enmType can be RTDIRENTRYTYPE_UNKNOWN if the file system or
+         *        the host doesn't return the information.  See
+         *        RTDIRENTRY::enmType.  Use RTDirQueryUnknownType() to get the
+         *        actual type. */
         RTDIRENTRY DirEntry;
         while (RT_SUCCESS(RTDirRead(hDir, &DirEntry, NULL)))
@@ -1303 +1318 @@
     else if (vrc == VERR_FILE_NOT_FOUND)
     {
+        /** @todo r=bird: See first error push_back in MachineMoveVM::i_MoveVMThreadTask */
         Utf8StrFmt errorDesc("Folder '%s' doesn't exist (%Rrc)", strRootFolder.c_str(), vrc);
         m_errorsList.push_back(ErrorInfoItem(VERR_FILE_NOT_FOUND, errorDesc.c_str()));
@@ -1310 +1326 @@
     else
     {
+        /** @todo r=bird: See first error push_back in MachineMoveVM::i_MoveVMThreadTask */
         Utf8StrFmt errorDesc("Could not open folder '%s' (%Rrc)", strRootFolder.c_str(), vrc);
         m_errorsList.push_back(ErrorInfoItem(VBOX_E_IPRT_ERROR, errorDesc.c_str()));
@@ -1334 +1351 @@
             if (RT_FAILURE(vrc))
             {
+                /** @todo r=bird: See first error push_back in MachineMoveVM::i_MoveVMThreadTask */
                 Utf8StrFmt errorDesc("Could not delete file '%s' (%Rrc)", listOfFiles.at(i).c_str(), hrc);
                 m_errorsList.push_back(ErrorInfoItem(VBOX_E_IPRT_ERROR, errorDesc.c_str()));
@@ -1343 +1361 @@
         }
     }
-    catch(HRESULT aRc)
+    catch (HRESULT aRc)  /** @todo r=bird: Pointless as you do nada beyond this point, so just return instead of throw hrc above! */
     {
         hrc = aRc;
     }
-    catch (...)
+    catch (...) /** @todo appendAndSetErrorXxxx could make this unnecessary too! */
     {
         hrc = VirtualBoxBase::handleUnexpectedExceptions(m_pMachine, RT_SRC_POS);
@@ -1381 +1399 @@
                 else
                 {
-                    Utf8StrFmt errorDesc("Could not get the size of file '%s' (%Rrc)", fullPath.c_str(), vrc);
+                    /** @todo r=bird: See first error push_back in MachineMoveVM::i_MoveVMThreadTask */
+                    Utf8StrFmt errorDesc("Could not get the size of file '%s': %Rrc", fullPath.c_str(), vrc);
                     m_errorsList.push_back(ErrorInfoItem(VBOX_E_IPRT_ERROR, errorDesc.c_str()));
 
@@ -1413 +1432 @@
                                          std::vector< ComObjPtr<Machine> > &aMachineList) const
 {
-    HRESULT rc = S_OK;
     Bstr name;
-    rc = pSnapshot->COMGETTER(Name)(name.asOutParam());
+    HRESULT rc = pSnapshot->COMGETTER(Name)(name.asOutParam());
     if (FAILED(rc)) return rc;
 
@@ -1475 +1493 @@
             try
             {
-                bool fPass = isMediumTypeSupportedForMoving(pMedium);
-                if(!fPass)
+                bool fPass = isMediumTypeSupportedForMovingThrowsHresult(pMedium);
+                if (!fPass)
                 {
-                    Log2(("Skipping file %s because of this medium type hasn't been supported for moving.\n",
-                          Utf8Str(bstrLocation.raw()).c_str()));
+                    Log2(("Skipping file %ls because of this medium type hasn't been supported for moving.\n",
+                          bstrLocation.raw()));
                     continue;
                 }
-            } catch (HRESULT aRc)
+            }
+            catch (HRESULT aRc)
             {
                 return aRc;
@@ -1512 +1531 @@
                 MEDIUMTASKMOVE mt;// = {false, "basename", NULL, 0, 0};
                 mt.strBaseName = bstrLocation;
-                Utf8Str strFolder = m_vmFolders[VBox_SnapshotFolder];
-                if (strFolder.isNotEmpty() && mt.strBaseName.contains(strFolder))
-                {
+                Utf8Str const &strFolder = m_vmFolders[VBox_SnapshotFolder];
+                if (strFolder.isNotEmpty() && mt.strBaseName.contains(strFolder)) /** @todo r=bird: contains? Shouldn't it be startsWith and take mixed slashes (windows) and stuff into account? */
                     mt.fSnapshot = true;
-                }
                 else
                     mt.fSnapshot = false;
@@ -1567 +1584 @@
         if (RT_FAILURE(vrc))
         {
+            /** @todo r=bird: See first error push_back in MachineMoveVM::i_MoveVMThreadTask */
             Utf8StrFmt errorDesc("Could not get file size of '%s' (%Rrc)", sst.strSaveStateFile.c_str(), vrc);
             m_errorsList.push_back(ErrorInfoItem(VBOX_E_IPRT_ERROR, errorDesc.c_str()));
@@ -1605 +1623 @@
 }
 
-bool MachineMoveVM::isMediumTypeSupportedForMoving(const ComPtr<IMedium> &pMedium)
+/** @todo r=bird: An option better than throwing stuff here would be to use S_OK
+ *        for true, S_FALSE for false and other values for errors.
+ *        UnattendedImpl.cpp and others does this. */
+bool MachineMoveVM::isMediumTypeSupportedForMovingThrowsHresult(const ComPtr<IMedium> &pMedium)
 {
-    HRESULT rc = S_OK;
+    HRESULT rc = S_OK; /** @todo r=bird: This mess with my brain.  Assigning S_OK and then first use is receiving the return
+                        * value of a call.  That's plain copy&past lazyness and deliberate code obfuscation. */
     bool fSupported = true;
     Bstr bstrLocation;
@@ -1613 +1635 @@
     if (FAILED(rc))
     {
-        fSupported = false;
+        fSupported = false; /** @todo r=bird: Why waste time setting fSupport here?? */
         throw rc;
     }
@@ -1646 +1668 @@
     {
         Log2(("Skipping medium %s. VHDX format is supported in \"read-only\" mode only. \n",
-              Utf8Str(bstrLocation.raw()).c_str()));
-        fSupported = false;
+              Utf8Str(bstrLocation.raw()).c_str())); /** @todo r=bird: You should log UTF-16 string directly using \%ls. */
+        fSupported = false; /** @todo return immediately? */
     }
 
@@ -1668 +1690 @@
 
         //only ISO image is moved. Otherwise adding some information into log file
+        /** @todo r=bird: Log2 is only doing debug logging, so perhaps LogRel()
+         *        here or fix the comment. */
         bool fIso = ext.endsWith(".iso", Utf8Str::CaseInsensitive);
         if (fIso == false)