Changeset 78761 in vbox for trunk/src/VBox/Main/src-server

Timestamp:

May 26, 2019 4:31:12 AM (6 years ago)

Author:

vboxsync

Message:

Main/Machine::moveTo: Some cleanups and a lot of @todo's. bugref:8345

Location:

trunk/src/VBox/Main/src-server

Files:

• MachineImpl.cpp (modified) (2 diffs)
• MachineImplMoveVM.cpp (modified) (13 diffs)

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

@@ -5458 +5458 @@
     DeleteConfigTask *pTask = new DeleteConfigTask(this, pProgress, "DeleteVM", llMediums, llFilesToDelete);
     rc = pTask->createThread();
+    pTask = NULL;
     if (FAILED(rc))
         return rc;
@@ -7154 +7155 @@
     LogFlowThisFuncEnter();
 
-    ComObjPtr<Progress> progress;
-
-    progress.createObject();
-
-    HRESULT rc = S_OK;
-    Utf8Str targetPath = aTargetPath;
-    Utf8Str type = aType;
-
-    /* Initialize our worker task */
-    MachineMoveVM* task = NULL;
-    try
-    {
-        task = new MachineMoveVM(this, targetPath, type, progress);
-    }
-    catch(...)
-    {
-        delete task;
-        return rc;
-    }
-
-    /*
-     * task pointer will be owned by the ThreadTask class.
-     * There is no need to call operator "delete" in the end.
-     */
-    rc = task->init();
-    if (SUCCEEDED(rc))
-    {
-        rc = task->createThread();
-        if (FAILED(rc))
-        {
-            setError(rc, tr("Could not run the thread for the task MachineMoveVM"));
-        }
-
-        /* Return progress to the caller */
-        progress.queryInterfaceTo(aProgress.asOutParam());
+    ComObjPtr<Progress> ptrProgress;
+    HRESULT hrc = ptrProgress.createObject();
+    if (SUCCEEDED(hrc))
+    {
+        /* Initialize our worker task */
+        MachineMoveVM *pTask = NULL;
+        try
+        {
+            pTask = new MachineMoveVM(this, aTargetPath, aType, ptrProgress);
+        }
+        catch (std::bad_alloc &)
+        {
+            return E_OUTOFMEMORY;
+        }
+        /** @todo r=bird: Turned out the init() function throws stuff of type int
+         *        without telling anyone.  Probably unintentionally.  The catching
+         *        here is only temporary until someone can be bothered to fix it
+         *        properly. */
+        try
+        {
+            hrc = pTask->init();
+        }
+        catch (...)
+        {
+            hrc = setError(E_FAIL, tr("Unknown exception thrown by MachineMoveVM::init"));
+        }
+        if (SUCCEEDED(hrc))
+        {
+            hrc = pTask->createThread();
+            pTask = NULL; /* Consumed by createThread(). */
+            if (SUCCEEDED(hrc))
+                ptrProgress.queryInterfaceTo(aProgress.asOutParam());
+            else
+                setError(hrc, tr("Failed to create a worker thread for the MachineMoveVM task"));
+        }
+        else
+            delete pTask;
     }
 
     LogFlowThisFuncLeave();
-    return rc;
+    return hrc;
 
 }

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

@@ -153 +153 @@
     {
         size_t len = m_targetPath.length() + 2;
-        if (len >=RTPATH_MAX)
-        {
+        if (len >= RTPATH_MAX)
+        {
+            /** @todo r=bird: Why the leading space in errorDesc? */
+            /** @todo "The destination path exceeds the maximum length" should
+             *        suffice here, I think. */
+            /** @todo r=bird: missing tr(). */
+            /** @todo r=bird: Actually, why bother with these error strings here? Nobody
+             *        will ever see them, given that the caller deletes the task when
+             *        init() fails. */
             Utf8Str errorDesc(" The destination path isn't correct. The length of path exceeded the maximum value.");
             errorsList.push_back(ErrorInfoItem(VBOX_E_IPRT_ERROR, errorDesc.c_str()));
+            /** @todo r=bird: What on earth is this?  It will cause the caller to leak the
+             *        task structure, unless it's catching exception (which it wasn't, but
+             *        I made it is now).  I sure hope this wasn't intentional... */
             throw m_pMachine->setError(VBOX_E_IPRT_ERROR, m_pMachine->tr(errorDesc.c_str()));
         }
 
+        /** @todo r=bird: I need to add a Utf8Str method or iprt/cxx/path.h thingy
+         *        for doing this.  We need this often and code like this doesn't
+         *        need to be repeated and re-optimized in each instance... */
         char* path = new char [len];
         RTStrCopy(path, len, m_targetPath.c_str());
@@ -178 +191 @@
     try
     {
-        Utf8Str info;
-        int vrc = 0;
-
         RTFOFF cbTotal = 0;
         RTFOFF cbFree = 0;
@@ -186 +196 @@
         uint32_t cbSector = 0;
 
-        vrc = RTFsQuerySizes(strTargetFolder.c_str(), &cbTotal, &cbFree, &cbBlock, &cbSector);
+        int vrc = RTFsQuerySizes(strTargetFolder.c_str(), &cbTotal, &cbFree, &cbBlock, &cbSector);
         if (FAILED(vrc))
         {
+            /** @todo r=bird: What's this RTPrint doing here?!? */
             RTPrintf("strTargetFolder is %s\n", strTargetFolder.c_str());
             Utf8StrFmt errorDesc("Unable to move machine. Can't get the destination storage size (%s)",
@@ -201 +212 @@
         vrc = RTDirOpen(&hDir, strTargetFolder.c_str());
         if (FAILED(vrc))
-            throw rc = vrc;
+            throw rc = vrc; /** @todo r=bird: RTDirOpen returns a VBox status code. That is _not_ the same as a COM HRESULT! */
         else
         {
@@ -213 +224 @@
                 errorsList.push_back(ErrorInfoItem(HRESULT(vrc), errorDesc.c_str()));
                 rc = m_pMachine->setErrorBoth(VBOX_E_FILE_ERROR, vrc, m_pMachine->tr(errorDesc.c_str()));
-            }
-
-            RTFileClose(hFile);
+                /** @todo r=bird: What happens to this error and 'rc'?  Should it be ignored? */
+            }
+
+            RTFileClose(hFile); /** @todo r=bird: Whey close if opening fail? */
             RTFileDelete(strTempFile.c_str());
             RTDirClose(hDir);
         }
 
-        long long totalFreeSpace = cbFree;
-        long long totalSpace = cbTotal;
-        info = Utf8StrFmt("blocks: total %lld, free %u ", cbTotal, cbFree);
+        /** @todo r=bird: Why the need for 'info' here?  LogRelFunc can do the exact
+         *        same thing without touching the heap.  I don't get what this is
+         *        about...  There is also excessive log duplication here as well as
+         *        seemingly unnecessary trailing whitespace. */
+        Utf8Str info = Utf8StrFmt("blocks: total %RTfoff, free %RTfoff ", cbTotal, cbFree);
         LogRelFunc(("%s \n", info.c_str()));
-        LogRelFunc(("total space (Kb) %lld (Mb) %lld (Gb) %lld\n",
-               totalSpace/1024, totalSpace/(1024*1024), totalSpace/(1024*1024*1024)));
-        LogRelFunc(("total free space (Kb) %lld (Mb) %lld (Gb) %lld\n",
-               totalFreeSpace/1024, totalFreeSpace/(1024*1024), totalFreeSpace/(1024*1024*1024)));
+        LogRelFunc(("total space (Kb) %RTfoff (Mb) %RTfoff (Gb) %RTfoff\n", cbTotal/_1K, cbTotal/_1M, cbTotal/_1G));
+        LogRelFunc(("total free space (Kb) %RTfoff (Mb) %RTfoff (Gb) %RTfoff\n", cbFree/_1K, cbFree/_1M, cbFree/_1G));
 
         RTFSPROPERTIES properties;
@@ -245 +257 @@
         Utf8Str strSettingsFilePath;
         Bstr bstr_settingsFilePath;
-        m_pMachine->COMGETTER(SettingsFilePath)(bstr_settingsFilePath.asOutParam());
+        m_pMachine->COMGETTER(SettingsFilePath)(bstr_settingsFilePath.asOutParam()); /** @todo r=bird: check the status code. */
         strSettingsFilePath = bstr_settingsFilePath;
         strSettingsFilePath.stripFilename();
@@ -277 +289 @@
         rc = m_pMachine->COMGETTER(State)(&machineState);
         if (FAILED(rc)) throw rc;
-        else if (machineState == MachineState_Saved)
+        if (machineState == MachineState_Saved)
         {
             m_pMachine->COMGETTER(StateFilePath)(bstr_stateFilePath.asOutParam());
@@ -340 +352 @@
 
         /* The lists llMedias and llSaveStateFiles are filled in the queryMediasForAllStates() */
-        queryMediasForAllStates(machineList);
+        queryMediasForAllStates(machineList); /** @todo r=bird: As a rule status codes needs checking every time. */
 
         {
@@ -429 +441 @@
                 {
                     neededFreeSpace += totalLogSize;
-                    if (totalFreeSpace - neededFreeSpace <= 1024*1024)
+                    if (cbFree - neededFreeSpace <= _1M)
                     {
                         throw VERR_OUT_OF_RESOURCES;//less than 1Mb free space on the target location
@@ -435 +447 @@
 
                     fileList_t filesList;
-                    getFilesList(strFolder, filesList);
+                    getFilesList(strFolder, filesList); /** @todo r=bird: return code check */
                     cit_t it = filesList.m_list.begin();
-                    while(it != filesList.m_list.end())
+                    while (it != filesList.m_list.end())
                     {
                         Utf8Str strFile = it->first.c_str();
@@ -467 +479 @@
         LogRelFunc(("Total space needed is %lld bytes\n", neededFreeSpace));
         /* Check a target location on enough room */
-        if (totalFreeSpace - neededFreeSpace <= 1024*1024)
-        {
-            LogRelFunc(("but free space on destination is %lld\n", totalFreeSpace));
+        if (cbFree - neededFreeSpace <= _1M)
+        {
+            LogRelFunc(("but free space on destination is %RTfoff\n", cbFree));
+            /** @todo r=bird: You're throwing a VBox status code here, but only catching
+             *        HRESULT.  So, this will go to the caller or someone else up the
+             *        stack, which is unacceptable throw behaviour. */
             throw VERR_OUT_OF_RESOURCES;//less than 1Mb free space on the target location
         }
 
         /* Add step for .vbox machine setting file */
-        {
+        { /** @todo what's the scoping for? */
             ++uCount;
             uTotalWeight += 1;
@@ -488 +503 @@
         {
             rc = m_pProgress->init(m_pMachine->i_getVirtualBox(),
-                                 static_cast<IMachine*>(m_pMachine) /* aInitiator */,
-                                 Bstr(m_pMachine->tr("Moving Machine")).raw(),
-                                 true /* fCancellable */,
-                                 uCount,
-                                 uTotalWeight,
-                                 Bstr(m_pMachine->tr("Initialize Moving")).raw(),
-                                 1);
+                                   static_cast<IMachine*>(m_pMachine) /* aInitiator */,
+                                   Utf8Str(m_pMachine->tr("Moving Machine")),
+                                   true /* fCancellable */,
+                                   uCount,
+                                   uTotalWeight,
+                                   Utf8Str(m_pMachine->tr("Initialize Moving")),
+                                   1);
             if (FAILED(rc))
             {
@@ -510 +525 @@
     catch(HRESULT hrc)
     {
+/** @todo r=bird: Seriously, why throw any error when you can just return
+ *        them.  This is just copying the thrown message and passing it onto
+ *        the return statemnt a few lines later.  (The log statemnt doesn't
+ *        really count as it doesn't log the status.) */
         rc = hrc;
     }