Changeset 30020 in vbox for trunk

Timestamp:

Jun 4, 2010 8:09:58 AM (15 years ago)

Author:

vboxsync

svn:sync-xref-src-repo-rev:

62351

Message:

Guest Control/Main: Get rid of busy waiting, use multi stage progress objects, clean up temporary callback contexts.

Location:

trunk/src/VBox

Files:

• Frontends/VBoxManage/VBoxManageGuestCtrl.cpp (modified) (6 diffs)
• Main/GuestImpl.cpp (modified) (30 diffs)
• Main/include/GuestImpl.h (modified) (1 diff)

trunk/src/VBox/Frontends/VBoxManage/VBoxManageGuestCtrl.cpp

@@ -388 +388 @@
                      * Some data left to output?
                      */
-
                     if (   fWaitForStdOut
                         || fWaitForStdErr)
                     {
-                        CHECK_ERROR_BREAK(guest, GetProcessOutput(uPID, 0 /* aFlags */,
-                                                                  u32TimeoutMS, _64K, ComSafeArrayAsOutParam(aOutputData)));
-                        cbOutputData = aOutputData.size();
-                        if (cbOutputData > 0)
+
+                        rc = guest->GetProcessOutput(uPID, 0 /* aFlags */,
+                                                     u32TimeoutMS, _64K, ComSafeArrayAsOutParam(aOutputData));
+                        if (FAILED(rc))
                         {
-                            /* aOutputData has a platform dependent line ending, standardize on
-                             * Unix style, as RTStrmWrite does the LF -> CR/LF replacement on
-                             * Windows. Otherwise we end up with CR/CR/LF on Windows. */
-                            ULONG cbOutputDataPrint = cbOutputData;
-                            for (BYTE *s = aOutputData.raw(), *d = s;
-                                 s - aOutputData.raw() < (ssize_t)cbOutputData;
-                                 s++, d++)
+                            /* If we got a VBOX_E_IPRT error we handle the error in a more gentle way
+                             * because it contains more accurate info about what went wrong. */
+                            ErrorInfo info(guest);
+                            if (info.isFullAvailable())
                             {
-                                if (*s == '\r')
+                                if (rc == VBOX_E_IPRT_ERROR)
                                 {
-                                    /* skip over CR, adjust destination */
-                                    d--;
-                                    cbOutputDataPrint--;
+                                    RTPrintf("%ls.\n", info.getText().raw());
                                 }
-                                else if (s != d)
-                                    *d = *s;
+                                else
+                                {
+                                    RTPrintf("ERROR: %ls (%Rhrc).\n", info.getText().raw(), info.getResultCode());
+                                }
                             }
-                            RTStrmWrite(g_pStdOut, aOutputData.raw(), cbOutputDataPrint);
+                            cbOutputData = 0;
                         }
-                    }
-
-                    if (cbOutputData <= 0)
+                        else
+                        {
+                            cbOutputData = aOutputData.size();
+                            if (cbOutputData > 0)
+                            {
+                                /* aOutputData has a platform dependent line ending, standardize on
+                                 * Unix style, as RTStrmWrite does the LF -> CR/LF replacement on
+                                 * Windows. Otherwise we end up with CR/CR/LF on Windows. */
+                                ULONG cbOutputDataPrint = cbOutputData;
+                                for (BYTE *s = aOutputData.raw(), *d = s;
+                                     s - aOutputData.raw() < (ssize_t)cbOutputData;
+                                     s++, d++)
+                                {
+                                    if (*s == '\r')
+                                    {
+                                        /* skip over CR, adjust destination */
+                                        d--;
+                                        cbOutputDataPrint--;
+                                    }
+                                    else if (s != d)
+                                        *d = *s;
+                                }
+                                RTStrmWrite(g_pStdOut, aOutputData.raw(), cbOutputDataPrint);
+                            }
+                        }
+                    }
+
+                    if (cbOutputData <= 0) /* No more output data left? */
                     {
                         if (fCompleted)
@@ -431 +452 @@
                     }
 
-                    /* process async cancelation */
+                    /* Process async cancelation */
                     if (g_fExecCanceled && !fCanceledAlready)
                     {
@@ -441 +462 @@
                     }
 
-                    /* progress canceled by Main API? */
+                    /* Progress canceled by Main API? */
                     if (   SUCCEEDED(progress->COMGETTER(Canceled(&fCanceled)))
                         && fCanceled)
@@ -447 +468 @@
                         break;
                     }
-
-                    progress->WaitForCompletion(100);
-                }
-
-                /* undo signal handling */
+                }
+
+                /* Undo signal handling */
                 if (fCancelable)
                 {
@@ -465 +484 @@
                         RTPrintf("Process execution canceled!\n");
                 }
-                else if (fCompleted)
+                else if (   fCompleted
+                         && SUCCEEDED(rc))
                 {
                     LONG iRc = false;
@@ -474 +494 @@
                         rc = progress->COMGETTER(ErrorInfo)(execError.asOutParam());
                         com::ErrorInfo info (execError);
-                        RTPrintf("\n\nProcess error details:\n");
-                        GluePrintErrorInfo(info);
-                        RTPrintf("\n");
-                    }
-                    if (fVerbose)
+                        if (SUCCEEDED(rc) && info.isFullAvailable())
+                        {
+                            /* If we got a VBOX_E_IPRT error we handle the error in a more gentle way
+                             * because it contains more accurate info about what went wrong. */
+                            if (info.getResultCode() == VBOX_E_IPRT_ERROR)
+                            {
+                                RTPrintf("%ls.\n", info.getText().raw());
+                            }
+                            else
+                            {
+                                RTPrintf("\n\nProcess error details:\n");
+                                GluePrintErrorInfo(info);
+                                RTPrintf("\n");
+                            }
+                        }
+                        else
+                            AssertMsgFailed(("Full error description is missing!\n"));
+                    }
+                    else if (fVerbose)
                     {
                         ULONG uRetStatus, uRetExitCode, uRetFlags;
-                        CHECK_ERROR_BREAK(guest, GetProcessStatus(uPID, &uRetExitCode, &uRetFlags, &uRetStatus));
-                        RTPrintf("Exit code=%u (Status=%u [%s], Flags=%u)\n", uRetExitCode, uRetStatus, getStatus(uRetStatus), uRetFlags);
-                    }
-                }
-                else /* If neither canceled nor completed we got a hard abort (shouldn't happen). */
+                        rc = guest->GetProcessStatus(uPID, &uRetExitCode, &uRetFlags, &uRetStatus);
+                        if (SUCCEEDED(rc))
+                            RTPrintf("Exit code=%u (Status=%u [%s], Flags=%u)\n", uRetExitCode, uRetStatus, getStatus(uRetStatus), uRetFlags);
+                    }
+                }
+                else
                 {
                     if (fVerbose)

trunk/src/VBox/Main/GuestImpl.cpp

@@ -114 +114 @@
 
 #ifdef VBOX_WITH_GUEST_CONTROL
-    /*
-     * Cleanup must be done *before* AutoUninitSpan to cancel all
-     * all outstanding waits in API functions (which hold AutoCaller
-     * ref counts).
-     */
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-
-    /* Clean up callback data. */
-    CallbackListIter it;
-    for (it = mCallbackList.begin(); it != mCallbackList.end(); it++)
-        destroyCtrlCallbackContext(it);
-
-    /* Clear process list. */
-    mGuestProcessList.clear();
+    /* Scope write lock as much as possible. */
+    {
+        /*
+         * Cleanup must be done *before* AutoUninitSpan to cancel all
+         * all outstanding waits in API functions (which hold AutoCaller
+         * ref counts).
+         */
+        AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+    
+        /* Clean up callback data. */
+        CallbackListIter it;
+        for (it = mCallbackList.begin(); it != mCallbackList.end(); it++)
+            destroyCtrlCallbackContext(it);
+    
+        /* Clear process list. */
+        mGuestProcessList.clear();
+    }
 #endif
 
@@ -493 +496 @@
      * changes to the object state.
      */
+#ifdef DEBUG_andy
     LogFlowFunc(("pvExtension = %p, u32Function = %d, pvParms = %p, cbParms = %d\n",
                  pvExtension, u32Function, pvParms, cbParms));
+#endif
     ComObjPtr<Guest> pGuest = reinterpret_cast<Guest *>(pvExtension);
 
@@ -540 +545 @@
                                 PCALLBACKDATAEXECSTATUS pData)
 {
-    LogFlowFuncEnter();
     int rc = VINF_SUCCESS;
 
@@ -560 +564 @@
 
         /* Was progress canceled before? */
-        BOOL fCancelled;
-        it->pProgress->COMGETTER(Canceled)(&fCancelled);
-
-        /* Do progress handling. */
+        BOOL fCanceled;
+        ComAssert(it->pProgress.isNotNull());
+        it->pProgress->COMGETTER(Canceled)(&fCanceled);
+
         Utf8Str errMsg;
-        HRESULT rc2 = S_OK;
-        switch (pData->u32Status)
-        {
-            case PROC_STS_STARTED:
-                break;
-
-            case PROC_STS_TEN: /* Terminated normally. */
-                if (   !it->pProgress->getCompleted()
-                    && !fCancelled)
+        if (!fCanceled)
+        {            
+            /* Do progress handling. */
+            switch (pData->u32Status)
+            {
+                case PROC_STS_STARTED:
+                    rc = it->pProgress->SetNextOperation(BstrFmt(tr("Waiting for process to exit ...")), 1 /* Weight */);
+                    if (FAILED(rc))
+                        errMsg = Utf8StrFmt(Guest::tr("Cannot enter waiting for process exit stage! rc=%u"),
+                                            rc);
+                    break;
+    
+                case PROC_STS_TEN: /* Terminated normally. */
+                    it->pProgress->notifyComplete(S_OK);
+                    LogFlowFunc(("Proccess (context ID=%u, status=%u) terminated successfully\n",
+                                 pData->hdr.u32ContextID, pData->u32Status));
+                    break;
+    
+                case PROC_STS_TEA: /* Terminated abnormally. */
+                    errMsg = Utf8StrFmt(Guest::tr("Process terminated abnormally with status '%u'"),
+                                        pCBData->u32Flags);
+                    break;
+    
+                case PROC_STS_TES: /* Terminated through signal. */
+                    errMsg = Utf8StrFmt(Guest::tr("Process terminated via signal with status '%u'"),
+                                        pCBData->u32Flags);
+                    break;
+    
+                case PROC_STS_TOK:
+                    errMsg = Utf8StrFmt(Guest::tr("Process timed out and was killed"));
+                    break;
+    
+                case PROC_STS_TOA:
+                    errMsg = Utf8StrFmt(Guest::tr("Process timed out and could not be killed"));
+                    break;
+    
+                case PROC_STS_DWN:
+                    errMsg = Utf8StrFmt(Guest::tr("Process exited because system is shutting down"));
+                    break;
+    
+                case PROC_STS_ERROR:
+                    errMsg = Utf8StrFmt(Guest::tr("Process execution failed with rc=%Rrc"), pCBData->u32Flags);
+                    break;
+    
+                default:
+                    break;
+            }
+            
+            /* Handle process list. */
+            /** @todo What happens on/deal with PID reuse? */
+            /** @todo How to deal with multiple updates at once? */
+            if (pCBData->u32PID > 0)
+            {
+                GuestProcessIter it_proc = getProcessByPID(pCBData->u32PID);
+                if (it_proc == mGuestProcessList.end())
                 {
-                    it->pProgress->notifyComplete(S_OK);
+                    /* Not found, add to list. */
+                    GuestProcess p;
+                    p.mPID = pCBData->u32PID;
+                    p.mStatus = pCBData->u32Status;
+                    p.mExitCode = pCBData->u32Flags; /* Contains exit code. */
+                    p.mFlags = 0;
+        
+                    mGuestProcessList.push_back(p);
                 }
-                break;
-
-            case PROC_STS_TEA: /* Terminated abnormally. */
-                errMsg = Utf8StrFmt(Guest::tr("Process terminated abnormally with status '%u'"),
-                                    pCBData->u32Flags);
-                break;
-
-            case PROC_STS_TES: /* Terminated through signal. */
-                errMsg = Utf8StrFmt(Guest::tr("Process terminated via signal with status '%u'"),
-                                    pCBData->u32Flags);
-                break;
-
-            case PROC_STS_TOK:
-                errMsg = Utf8StrFmt(Guest::tr("Process timed out and was killed"));
-                break;
-
-            case PROC_STS_TOA:
-                errMsg = Utf8StrFmt(Guest::tr("Process timed out and could not be killed"));
-                break;
-
-            case PROC_STS_DWN:
-                errMsg = Utf8StrFmt(Guest::tr("Process exited because system is shutting down"));
-                break;
-
-            default:
-                break;
-        }
-
-        /* Handle process list. */
-        /** @todo What happens on/deal with PID reuse? */
-        /** @todo How to deal with multiple updates at once? */
-        GuestProcessIter it_proc = getProcessByPID(pCBData->u32PID);
-        if (it_proc == mGuestProcessList.end())
-        {
-            /* Not found, add to list. */
-            GuestProcess p;
-            p.mPID = pCBData->u32PID;
-            p.mStatus = pCBData->u32Status;
-            p.mExitCode = pCBData->u32Flags; /* Contains exit code. */
-            p.mFlags = 0;
-
-            mGuestProcessList.push_back(p);
-        }
-        else /* Update list. */
-        {
-            it_proc->mStatus = pCBData->u32Status;
-            it_proc->mExitCode = pCBData->u32Flags; /* Contains exit code. */
-            it_proc->mFlags = 0;
-        }
-
-        if (   !it->pProgress.isNull()
-            && errMsg.length())
-        {
-            if (   !it->pProgress->getCompleted()
-                && !fCancelled)
+                else /* Update list. */
+                {
+                    it_proc->mStatus = pCBData->u32Status;
+                    it_proc->mExitCode = pCBData->u32Flags; /* Contains exit code. */
+                    it_proc->mFlags = 0;
+                }
+            }
+        }
+        else
+            errMsg = Utf8StrFmt(Guest::tr("Process execution canceled"));
+       
+        if (!it->pProgress->getCompleted())
+        {
+            if (   errMsg.length() 
+                || fCanceled) /* If cancelled we have to report E_FAIL! */
             {
-                it->pProgress->notifyComplete(E_FAIL /** @todo Find a better rc! */, COM_IIDOF(IGuest),
+                it->pProgress->notifyComplete(VBOX_E_IPRT_ERROR, COM_IIDOF(IGuest),
                                               (CBSTR)Guest::getComponentName(), errMsg.c_str());
-                LogFlowFunc(("Callback (context ID=%u, status=%u) progress marked as completed\n",
-                             pData->hdr.u32ContextID, pData->u32Status));
+                LogFlowFunc(("Process (context ID=%u, status=%u) reported error: %s\n",
+                             pData->hdr.u32ContextID, pData->u32Status, errMsg.c_str()));
             }
-            else
-                LogFlowFunc(("Callback (context ID=%u, status=%u) progress already marked as completed\n",
-                             pData->hdr.u32ContextID, pData->u32Status));
-        }
-        ASMAtomicWriteBool(&it->fCalled, true);
+        }        
     }
     else
         LogFlowFunc(("Unexpected callback (magic=%u, context ID=%u) arrived\n", pData->hdr.u32Magic, pData->hdr.u32ContextID));
-    LogFlowFuncLeave();
     return rc;
 }
@@ -654 +666 @@
                              PCALLBACKDATAEXECOUT pData)
 {
-    LogFlowFuncEnter();
     int rc = VINF_SUCCESS;
 
@@ -663 +674 @@
     if (it != mCallbackList.end())
     {
-        Assert(!it->fCalled);
         PCALLBACKDATAEXECOUT pCBData = (CALLBACKDATAEXECOUT*)it->pvData;
         AssertPtr(pCBData);
@@ -687 +697 @@
             pCBData->cbData = 0;
         }
-        ASMAtomicWriteBool(&it->fCalled, true);
+
+        /* Was progress canceled before? */
+        BOOL fCanceled;
+        ComAssert(it->pProgress.isNotNull());
+        it->pProgress->COMGETTER(Canceled)(&fCanceled);
+
+        if (!fCanceled)
+            it->pProgress->notifyComplete(S_OK);
+        else
+            it->pProgress->notifyComplete(VBOX_E_IPRT_ERROR, COM_IIDOF(IGuest),
+                                          (CBSTR)Guest::getComponentName(), Guest::tr("The output operation was cancelled"));
     }
     else
         LogFlowFunc(("Unexpected callback (magic=%u, context ID=%u) arrived\n", pData->hdr.u32Magic, pData->hdr.u32ContextID));
-    LogFlowFuncLeave();
     return rc;
 }
@@ -698 +717 @@
                                         PCALLBACKDATACLIENTDISCONNECTED pData)
 {
-    LogFlowFuncEnter();
     int rc = VINF_SUCCESS;
 
@@ -708 +726 @@
     {
         if (it->mContextID == pData->hdr.u32ContextID)
+        {
+            LogFlowFunc(("Client with context ID=%u disconnected\n", it->mContextID));
             destroyCtrlCallbackContext(it);
-    }
-
-    LogFlowFuncLeave();
+        }
+    }
     return rc;
 }
@@ -746 +765 @@
 void Guest::destroyCtrlCallbackContext(Guest::CallbackListIter it)
 {
-    LogFlowFuncEnter();
     if (it->pvData)
     {
+        LogFlowFunc(("Destroying callback with context ID=%u ...\n", it->mContextID));
+
         RTMemFree(it->pvData);
         it->pvData = NULL;
@@ -755 +775 @@
 
     /* Notify outstanding waits for progress ... */
-    if (it->pProgress && !it->pProgress.isNull())
-    {
-        /* Only cancel if not canceled before! */
-        BOOL fCancelled;
-        if (SUCCEEDED(it->pProgress->COMGETTER(Canceled)(&fCancelled)) && !fCancelled)
-            it->pProgress->Cancel();
+    if (it->pProgress && it->pProgress.isNotNull())
+    {
+        LogFlowFunc(("Handling progress of context ID=%u ...\n", it->mContextID));
+
+        BOOL fCompleted;
+        it->pProgress->COMGETTER(Completed)(&fCompleted);
+        if (!fCompleted)
+        {
+            /* Only cancel if not canceled before! */
+            BOOL fCanceled;
+            if (SUCCEEDED(it->pProgress->COMGETTER(Canceled)(&fCanceled)) && !fCanceled)
+                it->pProgress->Cancel();        
+
+            /* To get waitForCompletion notified we have to notify it if necessary. */
+            it->pProgress->notifyComplete(VBOX_E_IPRT_ERROR, COM_IIDOF(IGuest),
+                                          (CBSTR)Guest::getComponentName(), Guest::tr("The operation was canceled during shutdown"));
+        }        
         /*
          * Do *not NULL pProgress here, because waiting function like executeProcess()
@@ -766 +797 @@
          */
     }
-    LogFlowFuncLeave();
 }
 
@@ -774 +804 @@
 uint32_t Guest::addCtrlCallbackContext(eVBoxGuestCtrlCallbackType enmType, void *pvData, uint32_t cbData, Progress *pProgress)
 {
-    LogFlowFuncEnter();
+    AssertPtr(pProgress);
     uint32_t uNewContext = ASMAtomicIncU32(&mNextContextID);
     if (uNewContext == UINT32_MAX)
@@ -783 +813 @@
     context.mContextID = uNewContext;
     context.mType = enmType;
-    context.fCalled = false;
     context.pvData = pvData;
     context.cbData = cbData;
@@ -807 +836 @@
     }
 #endif
-
-    LogFlowFuncLeave();
     return uNewContext;
 }
@@ -844 +871 @@
     {
         /*
-         * Create progress object.
+         * Create progress object.  Note that this is a multi operation
+         * object to perform the following steps:
+         * - Operation 1 (0): Create/start process.
+         * - Operation 2 (1): Wait for process to exit.
+         * If this progress completed successfully (S_OK), the process
+         * started and exited normally. In any other case an error/exception
+         * occured.
          */
         ComObjPtr <Progress> progress;
@@ -852 +885 @@
             rc = progress->init(static_cast<IGuest*>(this),
                                 BstrFmt(tr("Executing process")),
-                                TRUE);
+                                TRUE,
+                                2,                                      /* Number of operations. */
+                                BstrFmt(tr("Starting process ...")));   /* Description of first stage. */
         }
         if (FAILED(rc)) return rc;
@@ -919 +954 @@
                     PCALLBACKDATAEXECSTATUS pData = (PCALLBACKDATAEXECSTATUS)RTMemAlloc(sizeof(CALLBACKDATAEXECSTATUS));
                     AssertReturn(pData, VBOX_E_IPRT_ERROR);
+                    RT_ZERO(*pData);
                     uContextID = addCtrlCallbackContext(VBOXGUESTCTRLCALLBACKTYPE_EXEC_START,
                                                         pData, sizeof(CALLBACKDATAEXECSTATUS), progress);
@@ -974 +1010 @@
                 if (it != mCallbackList.end())
                 {
-                    uint64_t u64Started = RTTimeMilliTS();
-                    while (!it->fCalled)
+                    ComAssert(it->pProgress.isNotNull());
+
+                    /*
+                     * Wait for the first stage (=0) to complete (that is starting the process).
+                     */
+                    PCALLBACKDATAEXECSTATUS pData = NULL;
+                    rc = it->pProgress->WaitForOperationCompletion(0, aTimeoutMS);
+                    if (SUCCEEDED(rc))
                     {
-                        /* Check for timeout. */
-                        unsigned cMsWait;
-                        if (aTimeoutMS == RT_INDEFINITE_WAIT)
-                            cMsWait = 10;
-                        else
+                        /* Was the operation canceled by one of the parties? */
+                        rc = it->pProgress->COMGETTER(Canceled)(&fCanceled);
+                        if (FAILED(rc)) throw rc;
+
+                        if (!fCanceled)
                         {
-                            uint64_t cMsElapsed = RTTimeMilliTS() - u64Started;
-                            if (cMsElapsed >= aTimeoutMS)
-                                break; /* Timed out. */
-                            cMsWait = RT_MIN(10, aTimeoutMS - (uint32_t)cMsElapsed);
+                            AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS); 
+    
+                            pData = (PCALLBACKDATAEXECSTATUS)it->pvData;
+                            Assert(it->cbData == sizeof(CALLBACKDATAEXECSTATUS));
+                            AssertPtr(pData);
+    
+                            /* Did we get some status? */
+                            switch (pData->u32Status)
+                            {
+                                case PROC_STS_STARTED:
+                                    /* Process is (still) running; get PID. */
+                                    *aPID = pData->u32PID;
+                                    break;
+        
+                                /* In any other case the process either already
+                                 * terminated or something else went wrong, so no PID ... */
+                                case PROC_STS_TEN: /* Terminated normally. */
+                                case PROC_STS_TEA: /* Terminated abnormally. */
+                                case PROC_STS_TES: /* Terminated through signal. */
+                                case PROC_STS_TOK:
+                                case PROC_STS_TOA:
+                                case PROC_STS_DWN:
+                                    /* 
+                                     * Process (already) ended, but we want to get the
+                                     * PID anyway to retrieve the output in a later call. 
+                                     */
+                                    *aPID = pData->u32PID;
+                                    break;
+        
+                                case PROC_STS_ERROR:
+                                    vrc = pData->u32Flags; /* u32Flags member contains IPRT error code. */
+                                    break;
+        
+                                case PROC_STS_UNDEFINED:
+                                    vrc = VERR_TIMEOUT;    /* Operation did not complete within time. */
+                                    break;
+
+                                default:
+                                    vrc = VERR_INVALID_PARAMETER; /* Unknown status, should never happen! */
+                                    break;
+                            }
                         }
-
-                        /* Check for manual stop. */
-                        if (!it->pProgress.isNull())
-                        {
-                            rc = it->pProgress->COMGETTER(Canceled)(&fCanceled);
-                            if (FAILED(rc)) throw rc;
-                            if (fCanceled)
-                                break; /* HGCM/guest wants to abort because of status change. */
-
-                        }
-                        /// @todo r=bird: two operation progress object and wait first operation.
-                        /// IProgress::WaitForOperationCompletion.
-                        RTThreadSleep(cMsWait);
+                        else /* Operation was canceled. */
+                            vrc = VERR_CANCELLED;
                     }
-                }
-
-                /* Was the whole thing canceled? */
-                if (!fCanceled)
-                {
-                    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-
-                    PCALLBACKDATAEXECSTATUS pData = (PCALLBACKDATAEXECSTATUS)it->pvData;
-                    Assert(it->cbData == sizeof(CALLBACKDATAEXECSTATUS));
-                    AssertPtr(pData);
-
-                    if (it->fCalled)
-                    {
-                        /* Did we get some status? */
-                        switch (pData->u32Status)
-                        {
-                            case PROC_STS_STARTED:
-                                /* Process is (still) running; get PID. */
-                                *aPID = pData->u32PID;
-                                break;
-
-                            /* In any other case the process either already
-                             * terminated or something else went wrong, so no PID ... */
-                            case PROC_STS_TEN: /* Terminated normally. */
-                            case PROC_STS_TEA: /* Terminated abnormally. */
-                            case PROC_STS_TES: /* Terminated through signal. */
-                            case PROC_STS_TOK:
-                            case PROC_STS_TOA:
-                            case PROC_STS_DWN:
-                                /*
-                                 * Process (already) ended, but we want to get the
-                                 * PID anyway to retrieve the output in a later call.
-                                 */
-                                *aPID = pData->u32PID;
-                                break;
-
-                            case PROC_STS_ERROR:
-                                vrc = pData->u32Flags; /* u32Flags member contains IPRT error code. */
-                                break;
-
-                            default:
-                                vrc = VERR_INVALID_PARAMETER; /* Unknown status, should never happen! */
-                                break;
-                        }
-                    }
-                    else /* If callback not called within time ... well, that's a timeout! */
+                    else /* Operation did not complete within time. */
                         vrc = VERR_TIMEOUT;
-
-                     /*
+    
+                    /*
                      * Do *not* remove the callback yet - we might wait with the IProgress object on something
                      * else (like end of process) ...
@@ -1081 +1104 @@
                                           tr("The guest did not respond within time (%ums)"), aTimeoutMS);
                         }
+                        else if (vrc == VERR_CANCELLED)
+                        {
+                            rc = setError(VBOX_E_IPRT_ERROR,
+                                          tr("The execution operation was canceled"));
+                        }
                         else if (vrc == VERR_PERMISSION_DENIED)
                         {
@@ -1088 +1116 @@
                         else
                         {
-                            if (pData->u32Status == PROC_STS_ERROR)
+                            if (pData && pData->u32Status == PROC_STS_ERROR)
                                 rc = setError(VBOX_E_IPRT_ERROR,
                                               tr("Process could not be started: %Rrc"), pData->u32Flags);
@@ -1094 +1122 @@
                                 rc = setError(E_UNEXPECTED,
                                               tr("The service call failed with error %Rrc"), vrc);
-                        }
+                        }               
                     }
                     else /* Execution went fine. */
@@ -1102 +1130 @@
                     }
                 }
-                else /* Operation was canceled. */
-                {
-                    rc = setError(VBOX_E_IPRT_ERROR,
-                                  tr("The operation was canceled"));
-                }
+                else /* Callback context not found; should never happen! */
+                    AssertMsg(it != mCallbackList.end(), ("Callback context with ID %u not found!", uContextID));
             }
             else /* HGCM related error codes .*/
@@ -1163 +1188 @@
         /*
          * Create progress object.
-         * Note that we need at least a local progress object here in order
-         * to get notified when someone cancels the operation.
+         * This progress object, compared to the one in executeProgress() above,
+         * is only local and is used to determine whether the operation finished
+         * or got cancelled.
          */
         ComObjPtr <Progress> progress;
@@ -1183 +1209 @@
         PCALLBACKDATAEXECOUT pData = (CALLBACKDATAEXECOUT*)RTMemAlloc(sizeof(CALLBACKDATAEXECOUT));
         AssertReturn(pData, VBOX_E_IPRT_ERROR);
+        RT_ZERO(*pData);
         uint32_t uContextID = addCtrlCallbackContext(VBOXGUESTCTRLCALLBACKTYPE_EXEC_OUTPUT,
                                                      pData, sizeof(CALLBACKDATAEXECOUT), progress);
@@ -1232 +1259 @@
             if (it != mCallbackList.end())
             {
-                uint64_t u64Started = RTTimeMilliTS();
-                while (!it->fCalled)
-                {
-                    /* Check for timeout. */
-                    unsigned cMsWait;
-                    if (aTimeoutMS == RT_INDEFINITE_WAIT)
-                        cMsWait = 10;
-                    else
-                    {
-                        uint64_t cMsElapsed = RTTimeMilliTS() - u64Started;
-                        if (cMsElapsed >= aTimeoutMS)
-                            break; /* Timed out. */
-                        cMsWait = RT_MIN(10, aTimeoutMS - (uint32_t)cMsElapsed);
-                    }
-
-                    /* Check for manual stop. */
-                    if (!it->pProgress.isNull())
-                    {
-                        rc = it->pProgress->COMGETTER(Canceled)(&fCanceled);
-                        if (FAILED(rc)) throw rc;
-                        if (fCanceled)
-                            break; /* Client wants to abort. */
-                    }
-                    RTThreadSleep(cMsWait);
-                }
-
-                /* Was the whole thing canceled? */
+                ComAssert(it->pProgress.isNotNull());
+
+                /* Wait until operation completed. */
+                rc = it->pProgress->WaitForCompletion(aTimeoutMS);
+                if (FAILED(rc)) throw rc;
+                
+                /* Was the operation canceled by one of the parties? */
+                rc = it->pProgress->COMGETTER(Canceled)(&fCanceled);
+                if (FAILED(rc)) throw rc;
+
                 if (!fCanceled)
                 {
-                    if (it->fCalled)
+                    BOOL fCompleted;
+                    if (   SUCCEEDED(it->pProgress->COMGETTER(Completed)(&fCompleted))
+                        && fCompleted)
                     {
                         AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
@@ -1287 +1299 @@
                 }
                 else /* Operation was canceled. */
+                {
                     vrc = VERR_CANCELLED;
+                }
 
                 if (RT_FAILURE(vrc))
@@ -1294 +1308 @@
                     {
                         /* This is not an error we want to report to COM. */
+                        rc = NO_ERROR;
                     }
                     else if (vrc == VERR_TIMEOUT)
@@ -1303 +1318 @@
                     {
                         rc = setError(VBOX_E_IPRT_ERROR,
-                                      tr("The operation was canceled"));
+                                      tr("The output operation was canceled"));
                     }
                     else
@@ -1316 +1331 @@
                     destroyCtrlCallbackContext(it);
                 }
+
+                /* Remove callback context (not used anymore). */
+                mCallbackList.erase(it);
             }
             else /* PID lookup failed. */

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

@@ -122 +122 @@
     struct CallbackContext
     {
+        /** Associated context ID. */
         uint32_t                    mContextID;
         eVBoxGuestCtrlCallbackType  mType;
+        /** Pointer to user-supplied data. */        
         void                       *pvData;
+        /** Size of user-supplied data. */
         uint32_t                    cbData;
-        /** Atomic flag whether callback was called. */
-        volatile bool               fCalled;
         /** Pointer to user-supplied IProgress. */
         ComObjPtr<Progress>         pProgress;