Index: /trunk/src/VBox/Main/include/GuestImpl.h =================================================================== --- /trunk/src/VBox/Main/include/GuestImpl.h (revision 35454) +++ /trunk/src/VBox/Main/include/GuestImpl.h (revision 35455) @@ -179,6 +179,8 @@ CallbackMapIter getCtrlCallbackContextByID(uint32_t u32ContextID); GuestProcessMapIter getProcessByPID(uint32_t u32PID); + void notifyCtrlCallbackContext(Guest::CallbackMapIter it, const char *pszText); void destroyCtrlCallbackContext(CallbackMapIter it); uint32_t addCtrlCallbackContext(eVBoxGuestCtrlCallbackType enmType, void *pvData, uint32_t cbData, Progress* pProgress); + HRESULT waitForProcessStatusChange(ULONG uPID, PULONG puRetStatus, PULONG puRetExitCode, ULONG uTimeoutMS); # endif Index: /trunk/src/VBox/Main/src-client/GuestCtrlImpl.cpp =================================================================== --- /trunk/src/VBox/Main/src-client/GuestCtrlImpl.cpp (revision 35454) +++ /trunk/src/VBox/Main/src-client/GuestCtrlImpl.cpp (revision 35455) @@ -296,5 +296,5 @@ size_t cbTransfered = 0; size_t cbRead; - SafeArray aInputData(_1M); + SafeArray aInputData(_64K); while ( SUCCEEDED(execProgress->COMGETTER(Completed(&fCompleted))) && !fCompleted) @@ -305,5 +305,5 @@ { vrc = RTFileRead(fileSource, (uint8_t*)aInputData.raw(), - RT_MIN(cbToRead, _1M), &cbRead); + RT_MIN(cbToRead, _64K), &cbRead); /* * Some other error occured? There might be a chance that RTFileRead @@ -326,5 +326,5 @@ ULONG uFlags = ProcessInputFlag_None; /* Did we reach the end of the content we want to transfer (last chunk)? */ - if ( (cbRead < _1M) + if ( (cbRead < _64K) /* ... or does the user want to cancel? */ || ( SUCCEEDED(aTask->progress->COMGETTER(Canceled(&fCanceled))) @@ -358,4 +358,8 @@ break; + /* Did the user cancel the operation above? */ + if (fCanceled) + break; + /* Progress canceled by Main API? */ if ( SUCCEEDED(execProgress->COMGETTER(Canceled(&fCanceled))) @@ -370,5 +374,65 @@ if (SUCCEEDED(rc)) - aTask->progress->notifyComplete(S_OK); + { + /* + * If we got here this means the started process either was completed, + * canceled or we simply got all stuff transferred. + */ + ULONG uRetStatus, uRetExitCode; + rc = pGuest->waitForProcessStatusChange(uPID, &uRetStatus, &uRetExitCode, 10 * 1000 /* 10s timeout. */); + if (FAILED(rc)) + { + rc = TaskGuest::setProgressErrorInfo(rc, aTask->progress, pGuest); + } + else + { + if ( uRetExitCode != 0 + || uRetStatus != PROC_STS_TEN) + { + rc = TaskGuest::setProgressErrorInfo(VBOX_E_IPRT_ERROR, aTask->progress, + Guest::tr("Guest reported error %u while copying file \"%s\" to \"%s\""), + uRetExitCode, aTask->strSource.c_str(), aTask->strDest.c_str()); + } + } + } + + if (SUCCEEDED(rc)) + { + if (fCanceled) + { + /* + * In order to make the progress object to behave nicely, we also have to + * notify the object with a complete event when it's canceled. + */ + aTask->progress->notifyComplete(VBOX_E_IPRT_ERROR, + COM_IIDOF(IGuest), + Guest::getStaticComponentName(), + Guest::tr("Copying file \"%s\" canceled"), aTask->strSource.c_str()); + } + else + { + /* + * Even if we succeeded until here make sure to check whether we really transfered + * everything. + */ + if (!cbTransfered) + { + /* If nothing was transfered this means "vbox_cat" wasn't able to write + * to the destination -> access denied. */ + rc = TaskGuest::setProgressErrorInfo(VBOX_E_IPRT_ERROR, aTask->progress, + Guest::tr("Access denied when copying file \"%s\" to \"%s\""), + aTask->strSource.c_str(), aTask->strDest.c_str()); + } + else if (cbTransfered < cbSize) + { + /* If we did not copy all let the user know. */ + rc = TaskGuest::setProgressErrorInfo(VBOX_E_IPRT_ERROR, aTask->progress, + Guest::tr("Copying file \"%s\" failed (%u/%u bytes transfered)"), + aTask->strSource.c_str(), cbTransfered, cbSize); + } + else /* Yay, all went fine! */ + aTask->progress->notifyComplete(S_OK); + } + } } } @@ -1080,8 +1144,20 @@ case VBOXGUESTCTRLCALLBACKTYPE_EXEC_OUTPUT: { - PCALLBACKDATAEXECOUT pItData = (CALLBACKDATAEXECOUT*)it2->second.pvData; + PCALLBACKDATAEXECOUT pItData = (PCALLBACKDATAEXECOUT)it2->second.pvData; AssertPtr(pItData); if (pItData->u32PID == pCBData->u32PID) - destroyCtrlCallbackContext(it2); + notifyCtrlCallbackContext(it2, errMsg.c_str()); + break; + } + + /* When waiting for injecting process input while the process is destroyed, + * make sure we also destroy the actual waiting operation (internal progress object) + * in order to not block the caller. */ + case VBOXGUESTCTRLCALLBACKTYPE_EXEC_INPUT_STATUS: + { + PCALLBACKDATAEXECINSTATUS pItData = (PCALLBACKDATAEXECINSTATUS)it2->second.pvData; + AssertPtr(pItData); + if (pItData->u32PID == pCBData->u32PID) + notifyCtrlCallbackContext(it2, errMsg.c_str()); break; } @@ -1093,9 +1169,7 @@ } - HRESULT hr2 = it->second.pProgress->notifyComplete(VBOX_E_IPRT_ERROR, - COM_IIDOF(IGuest), - Guest::getStaticComponentName(), - "%s", errMsg.c_str()); - AssertComRC(hr2); + /* Let the caller know what went wrong ... */ + notifyCtrlCallbackContext(it, errMsg.c_str()); + LogFlowFunc(("Process (CID=%u, status=%u) reported error: %s\n", pData->hdr.u32ContextID, pData->u32Status, errMsg.c_str())); @@ -1189,23 +1263,10 @@ pCBData->cbProcessed = pData->cbProcessed; - /* Was progress canceled before? */ - BOOL fCanceled; - ComAssert(!it->second.pProgress.isNull()); - if (SUCCEEDED(it->second.pProgress->COMGETTER(Canceled)(&fCanceled)) && fCanceled) - { - it->second.pProgress->notifyComplete(VBOX_E_IPRT_ERROR, - COM_IIDOF(IGuest), - Guest::getStaticComponentName(), - Guest::tr("The input operation was canceled")); - } - else - { - BOOL fCompleted; - if ( SUCCEEDED(it->second.pProgress->COMGETTER(Completed)(&fCompleted)) - && !fCompleted) - { - /* If we previously got completed notification, don't trigger again. */ - it->second.pProgress->notifyComplete(S_OK); - } + /* Only trigger completion once. */ + BOOL fCompleted; + if ( SUCCEEDED(it->second.pProgress->COMGETTER(Completed)(&fCompleted)) + && !fCompleted) + { + it->second.pProgress->notifyComplete(S_OK); } } @@ -1225,5 +1286,5 @@ { LogFlowFunc(("Client with CID=%u disconnected\n", it->first)); - destroyCtrlCallbackContext(it); + notifyCtrlCallbackContext(it, Guest::tr("Client disconnected")); } return rc; @@ -1254,9 +1315,20 @@ } + /* Remove callback context (not used anymore). */ + mCallbackMap.erase(it); +} + +/* No locking here; */ +void Guest::notifyCtrlCallbackContext(Guest::CallbackMapIter it, const char *pszText) +{ + AssertPtr(pszText); + LogFlowFunc(("Handling callback with CID=%u ...\n", it->first)); + /* Notify outstanding waits for progress ... */ if ( it->second.pProgress && !it->second.pProgress.isNull()) { - LogFlowFunc(("Handling progress for CID=%u ...\n", it->first)); + LogFlowFunc(("Notifying progress for CID=%u (Reason: %s) ...\n", + it->first, pszText)); /* @@ -1268,11 +1340,4 @@ if (!fCompleted) { - LogFlowFunc(("Progress of CID=%u *not* completed, cancelling ...\n", it->first)); - - /* Only cancel if not canceled before! */ - BOOL fCanceled; - if (SUCCEEDED(it->second.pProgress->COMGETTER(Canceled)(&fCanceled)) && !fCanceled) - it->second.pProgress->Cancel(); - /* * To get waitForCompletion completed (unblocked) we have to notify it if necessary (only @@ -1285,5 +1350,5 @@ COM_IIDOF(IGuest), Guest::getStaticComponentName(), - Guest::tr("The operation was canceled because client is shutting down")); + pszText); } /* @@ -1352,4 +1417,37 @@ #endif return uNewContext; +} + +HRESULT Guest::waitForProcessStatusChange(ULONG uPID, PULONG puRetStatus, PULONG puRetExitCode, ULONG uTimeoutMS) +{ + AssertPtr(puRetStatus); + AssertPtr(puRetExitCode); + + if (uTimeoutMS == 0) + uTimeoutMS = UINT32_MAX; + + uint64_t u64StartMS = RTTimeMilliTS(); + + HRESULT hRC; + ULONG uRetFlagsIgnored; + do + { + /* + * Do some busy waiting within the specified time period (if any). + */ + if ( uTimeoutMS != UINT32_MAX + && RTTimeMilliTS() - u64StartMS > uTimeoutMS) + { + hRC = setError(VBOX_E_IPRT_ERROR, + tr("The process (PID %u) did not change its status within time (%ums)"), + uPID, uTimeoutMS); + break; + } + hRC = GetProcessStatus(uPID, puRetExitCode, &uRetFlagsIgnored, puRetStatus); + if (FAILED(hRC)) + break; + RTThreadSleep(100); + } while(*puRetStatus == PROC_STS_STARTED && SUCCEEDED(hRC)); + return hRC; } #endif /* VBOX_WITH_GUEST_CONTROL */ @@ -1736,19 +1834,21 @@ *aBytesWritten = 0; - /* Search for existing PID. */ - GuestProcessMapIterConst itProc = getProcessByPID(aPID); - if (itProc != mGuestProcessMap.end()) - { - /* PID exists; check if process is still running. */ - if (itProc->second.mStatus != PROC_STS_STARTED) + { + /* Take read lock to prevent races. */ + AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS); + + /* Search for existing PID. */ + GuestProcessMapIterConst itProc = getProcessByPID(aPID); + if (itProc != mGuestProcessMap.end()) { + /* PID exists; check if process is still running. */ + if (itProc->second.mStatus != PROC_STS_STARTED) + rc = setError(VBOX_E_IPRT_ERROR, + Guest::tr("Cannot inject input to not running process (PID %u)"), aPID); + } + else rc = setError(VBOX_E_IPRT_ERROR, - tr("Process (PID %u) does not run anymore! Status: %ld, Flags: %u, Exit Code: %u"), - aPID, itProc->second.mStatus, itProc->second.mFlags, itProc->second.mExitCode); - } - } - else - rc = setError(VBOX_E_IPRT_ERROR, - tr("Process (PID %u) not found!"), aPID); + Guest::tr("Cannot inject input to non-existent process (PID %u)"), aPID); + } if (SUCCEEDED(rc)) @@ -1756,17 +1856,18 @@ /* * Create progress object. - * This progress object, compared to the one in executeProgress() above - * is only local and is used to determine whether the operation finished - * or got canceled. + * This progress object, compared to the one in executeProgress() above, + * is only single-stage local and is used to determine whether the operation + * finished or got canceled. */ - ComObjPtr progress; - rc = progress.createObject(); + ComObjPtr pProgress; + rc = pProgress.createObject(); if (SUCCEEDED(rc)) { - rc = progress->init(static_cast(this), - Bstr(tr("Setting input for process")).raw(), - TRUE /* Cancelable */); + rc = pProgress->init(static_cast(this), + Bstr(tr("Setting input for process")).raw(), + TRUE /* Cancelable */); } - if (FAILED(rc)) return rc; + if (FAILED(rc)) throw rc; + ComAssert(!pProgress.isNull()); /* Adjust timeout. */ @@ -1775,12 +1876,15 @@ PCALLBACKDATAEXECINSTATUS pData = (PCALLBACKDATAEXECINSTATUS)RTMemAlloc(sizeof(CALLBACKDATAEXECINSTATUS)); + if (NULL == pData) throw rc; AssertReturn(pData, VBOX_E_IPRT_ERROR); RT_ZERO(*pData); + /* Save PID + output flags for later use. */ pData->u32PID = aPID; pData->u32Flags = aFlags; + /* Add job to callback contexts. */ uint32_t uContextID = addCtrlCallbackContext(VBOXGUESTCTRLCALLBACKTYPE_EXEC_INPUT_STATUS, - pData, sizeof(CALLBACKDATAEXECINSTATUS), progress); + pData, sizeof(CALLBACKDATAEXECINSTATUS), pProgress); Assert(uContextID > 0); @@ -1838,60 +1942,43 @@ if (FAILED(rc)) throw rc; - /* Was the operation canceled by one of the parties? */ - rc = it->second.pProgress->COMGETTER(Canceled)(&fCanceled); - if (FAILED(rc)) throw rc; - - if (!fCanceled) - { - BOOL fCompleted; - if ( SUCCEEDED(it->second.pProgress->COMGETTER(Completed)(&fCompleted)) - && fCompleted) - { - PCALLBACKDATAEXECINSTATUS pStatusData = (PCALLBACKDATAEXECINSTATUS)it->second.pvData; - Assert(it->second.cbData == sizeof(CALLBACKDATAEXECINSTATUS)); - AssertPtr(pStatusData); - - *aBytesWritten = pStatusData->cbProcessed; - } - } - else /* Operation was canceled. */ - vrc = VERR_CANCELLED; - - if (RT_FAILURE(vrc)) - { - if (vrc == VERR_CANCELLED) - { - rc = setError(VBOX_E_IPRT_ERROR, - tr("The input operation was canceled")); - } - else - { - rc = setError(E_UNEXPECTED, - tr("The service call failed with error %Rrc"), vrc); - } - } + /* Was the call completed within time? */ + LONG uResult; + if ( SUCCEEDED(it->second.pProgress->COMGETTER(ResultCode)(&uResult)) + && uResult == S_OK) + { + PCALLBACKDATAEXECINSTATUS pStatusData = (PCALLBACKDATAEXECINSTATUS)it->second.pvData; + AssertPtr(pStatusData); + Assert(it->second.cbData == sizeof(CALLBACKDATAEXECINSTATUS)); + + *aBytesWritten = pStatusData->cbProcessed; + } + else if ( SUCCEEDED(it->second.pProgress->COMGETTER(Canceled)(&fCanceled)) + && fCanceled) + { + rc = setError(VBOX_E_IPRT_ERROR, + tr("The input operation was canceled by the guest")); + } + else + rc = setError(VBOX_E_IPRT_ERROR, + tr("The input operation was not acknowledged from guest within time (%ums)"), aTimeoutMS); { AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS); - /* - * Destroy locally used progress object. - */ + /* Destroy locally used progress object. */ destroyCtrlCallbackContext(it); } - - /* Remove callback context (not used anymore). */ - mCallbackMap.erase(it); } else /* PID lookup failed. */ rc = setError(VBOX_E_IPRT_ERROR, - tr("Process (PID %u) not found!"), aPID); + tr("Process (PID %u) not found"), aPID); } else /* HGCM operation failed. */ rc = setError(E_UNEXPECTED, - tr("The HGCM call failed with error %Rrc"), vrc); + tr("The HGCM call failed (%Rrc)"), vrc); /* Cleanup. */ - progress->uninit(); - progress.setNull(); + if (!pProgress.isNull()) + pProgress->uninit(); + pProgress.setNull(); } } @@ -1938,5 +2025,5 @@ rc = progress->init(static_cast(this), Bstr(tr("Getting output of process")).raw(), - TRUE); + TRUE /* Cancelable */); } if (FAILED(rc)) return rc; @@ -2072,12 +2159,7 @@ { AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS); - /* - * Destroy locally used progress object. - */ + /* Destroy locally used progress object. */ destroyCtrlCallbackContext(it); } - - /* Remove callback context (not used anymore). */ - mCallbackMap.erase(it); } else /* PID lookup failed. */