Index: /trunk/src/VBox/Frontends/VBoxManage/VBoxManageGuestCtrl.cpp =================================================================== --- /trunk/src/VBox/Frontends/VBoxManage/VBoxManageGuestCtrl.cpp (revision 35915) +++ /trunk/src/VBox/Frontends/VBoxManage/VBoxManageGuestCtrl.cpp (revision 35916) @@ -228,12 +228,13 @@ /** - * Initializes the VM, that is checks whether it's up and - * running, if it can be locked (shared only) and returns a - * valid IGuest pointer on success. + * Initializes the VM for IGuest operations. + * + * That is, checks whether it's up and running, if it can be locked (shared + * only) and returns a valid IGuest pointer on success. * * @return IPRT status code. * @param pArg Our command line argument structure. - * @param pszNameOrId The VM's name or UUID to use. - * @param pGuest Pointer where to store the IGuest interface. + * @param pszNameOrId The VM's name or UUID. + * @param pGuest Where to return the IGuest interface pointer. */ static int ctrlInitVM(HandlerArg *pArg, const char *pszNameOrId, ComPtr *pGuest) @@ -280,9 +281,9 @@ } +/* */ static int handleCtrlExecProgram(HandlerArg *a) { /* - * Check the syntax. We can deduce the correct syntax from the number of - * arguments. + * Parse arguments. */ if (a->argc < 2) /* At least the command we want to execute in the guest should be present :-). */ @@ -301,24 +302,23 @@ }; - int ch; - RTGETOPTUNION ValueUnion; - RTGETOPTSTATE GetState; + int ch; + RTGETOPTUNION ValueUnion; + RTGETOPTSTATE GetState; RTGetOptInit(&GetState, a->argc, a->argv, s_aOptions, RT_ELEMENTS(s_aOptions), 1, 0); - Utf8Str Utf8Cmd; - uint32_t uFlags = 0; - /* Note: this uses IN_BSTR as it must be BSTR on COM and CBSTR on XPCOM */ + Utf8Str Utf8Cmd; + uint32_t fFlags = 0; com::SafeArray args; com::SafeArray env; - Utf8Str Utf8UserName; - Utf8Str Utf8Password; - uint32_t u32TimeoutMS = 0; - bool fWaitForExit = false; - bool fWaitForStdOut = false; - bool fWaitForStdErr = false; - bool fVerbose = false; - - int vrc = VINF_SUCCESS; - bool fUsageOK = true; + Utf8Str Utf8UserName; + Utf8Str Utf8Password; + uint32_t u32TimeoutMS = 0; + bool fWaitForExit = false; + bool fWaitForStdOut = false; + bool fWaitForStdErr = false; + bool fVerbose = false; + + int vrc = VINF_SUCCESS; + bool fUsageOK = true; while ( (ch = RTGetOpt(&GetState, &ValueUnion)) && RT_SUCCESS(vrc)) @@ -360,8 +360,14 @@ case 'f': /* Flags */ + /** @todo r=bird: As stated before, this generic flags stuff + * does NOT make sense! The IgnoreOprphanedProcesses + * features should have been added as: + * { "--ignore-operhaned-processes", DEFINE, RTGETOPT_REQ_NOTHING } + * + * Please, remove -f in 4.1, replace it with above. */ /** @todo Needs a bit better processing as soon as we have more flags. */ /** @todo Add a hidden flag. */ if (!RTStrICmp(ValueUnion.psz, "ignoreorphanedprocesses")) - uFlags |= ExecuteProcessFlag_IgnoreOrphanedProcesses; + fFlags |= ExecuteProcessFlag_IgnoreOrphanedProcesses; else fUsageOK = false; @@ -386,4 +392,11 @@ case 'w': /* Wait for ... */ { + /** @todo r=bird: Same as for -f: Use individual options for + * indicating what to wait for. This is unix tradition and + * much simpler to write code for. It also avoids people + * finding out that the following sequence does not work + * (contrary to what one would expect): + * -w stdout,stderr + * */ if (!RTStrICmp(ValueUnion.psz, "exit")) fWaitForExit = true; @@ -405,4 +418,15 @@ case VINF_GETOPT_NOT_OPTION: { + /** @todo r=bird: Guess what the following does: + * VBoxManage guestcontrol exec myvm /bin/ls 1 2 3 4; + * + * In 4.1 this SHALL be changed to treat 1 2 3 4 as arguments + * to /bin/ls. Drop --arguments and replace it with --image + * . The --image defaults to the first argument if + * not specified. Users should be encouraged to use '--' to + * separate 'exec' options from options to the guest + * program: + * VBoxManage guestcontrol myvm exec --image /bin/busybox -- ln -s /foo /bar + */ /* The actual command we want to execute on the guest. */ Utf8Cmd = ValueUnion.psz; @@ -415,15 +439,23 @@ } - if (!fUsageOK) + if (!fUsageOK) /** @todo r=bird: there is no clean up, so just return directly on failure with a specific message. */ return errorSyntax(USAGE_GUESTCONTROL, "Incorrect parameters"); if (Utf8Cmd.isEmpty()) - return errorSyntax(USAGE_GUESTCONTROL, - "No command to execute specified!"); + return errorSyntax(USAGE_GUESTCONTROL, "No command to execute specified!"); if (Utf8UserName.isEmpty()) - return errorSyntax(USAGE_GUESTCONTROL, - "No user name specified!"); - + return errorSyntax(USAGE_GUESTCONTROL, "No user name specified!"); + + /** @todo r=bird: You don't check vrc here, so if RTGetOptArgvFromString + * or RTGetOptArgvFromString failed above, you'll ignore it. + * + * Just simplify the argument parsing to not use unnecessary state + * variables and instead return directly on error with a specific error + * message. (See fUsageOk comment above.) */ + + /* + * + */ HRESULT rc = S_OK; ComPtr guest; @@ -431,7 +463,4 @@ if (RT_SUCCESS(vrc)) { - ComPtr progress; - ULONG uPID = 0; - if (fVerbose) { @@ -446,10 +475,15 @@ /* Execute the process. */ - rc = guest->ExecuteProcess(Bstr(Utf8Cmd).raw(), uFlags, + ComPtr progress; + ULONG uPID = 0; + rc = guest->ExecuteProcess(Bstr(Utf8Cmd).raw(), + fFlags, ComSafeArrayAsInParam(args), ComSafeArrayAsInParam(env), Bstr(Utf8UserName).raw(), - Bstr(Utf8Password).raw(), u32TimeoutMS, - &uPID, progress.asOutParam()); + Bstr(Utf8Password).raw(), + u32TimeoutMS, + &uPID, + progress.asOutParam()); if (FAILED(rc)) vrc = ctrlPrintError(guest, COM_IIDOF(IGuest)); @@ -490,7 +524,7 @@ /* Wait for process to exit ... */ - BOOL fCompleted = FALSE; - BOOL fCanceled = FALSE; - int cMilliesSleep = 0; + BOOL fCompleted = FALSE; + BOOL fCanceled = FALSE; + int cMilliesSleep = 0; while (SUCCEEDED(progress->COMGETTER(Completed(&fCompleted)))) { @@ -517,4 +551,11 @@ if (cbOutputData > 0) { +/** @todo r=bird: cat'ing binary data from the guest is not going to work + * reliably if we do conversions like this. Should probably just + * write the output as it is by default, but bypassing RTStrWrite and + * it's automatic translation. Adding exec options to convert unix2dos + * and dos2unix. Use a VFS I/O stream filter for doing this, it's a + * generic problem and the new VFS APIs will handle it more + * transparently. (requires writing dos2unix/unix2dos filters ofc) */ /* aOutputData has a platform dependent line ending, standardize on * Unix style, as RTStrmWrite does the LF -> CR/LF replacement on @@ -565,7 +606,5 @@ if ( SUCCEEDED(progress->COMGETTER(Canceled(&fCanceled))) && fCanceled) - { break; - } /* Did we run out of time? */ @@ -578,4 +617,7 @@ /* Don't hog the CPU in a busy loop! */ +/** @todo r=bird: I believe I already mentioned that this problem is better + * solved by using WaitForCompletion and GetProcessOutput with timeouts. The + * 1ms hack above is not what I had in mind. This quick fix must go away. */ if (cbOutputData <= 0) { @@ -586,5 +628,5 @@ else cMilliesSleep = 0; - } + } /* while */ /* Undo signal handling */ @@ -592,4 +634,5 @@ ctrlSignalHandlerUninstall(); + /* Report status back to the user. */ if (fCanceled) { @@ -603,7 +646,5 @@ CHECK_ERROR_RET(progress, COMGETTER(ResultCode)(&iRc), rc); if (FAILED(iRc)) - { vrc = ctrlPrintProgressError(progress); - } else if (fVerbose) { @@ -612,4 +653,16 @@ if (SUCCEEDED(rc)) RTPrintf("Exit code=%u (Status=%u [%s], Flags=%u)\n", uRetExitCode, uRetStatus, ctrlExecGetStatus(uRetStatus), uRetFlags); + /** @todo r=bird: The guest application exit code + * should by default be returned by the command. + * Non-normal exits and exit codes outside + * the portable range (0..127) should be + * translated to more portable values. + * + * Alternatively, we could return say exit code 16 if + * the guest command failed, 17 if it terminated on a + * signal, 18 if it abended and exit code 19 if it + * timed out. This is possibly a better solution. + * + * Think about people using VBoxManage for scripting. */ } } @@ -624,7 +677,7 @@ } - if (RT_FAILURE(vrc)) - rc = VBOX_E_IPRT_ERROR; - return SUCCEEDED(rc) ? 0 : 1; + if (RT_FAILURE(vrc) || FAILED(rc)) + return RTEXITCODE_FAILURE; + return RTEXITCODE_SUCCESS; } @@ -709,5 +762,5 @@ * @param pszFilter Search filter (e.g. *.pdf). * @param pszDest Destination directory. - * @param uFlags Copy flags. + * @param fFlags Copy flags. * @param pcObjects Where to store the overall objects to * copy found. @@ -716,5 +769,5 @@ static int ctrlCopyDirectoryRead(const char *pszRootDir, const char *pszSubDir, const char *pszFilter, const char *pszDest, - uint32_t uFlags, uint32_t *pcObjects, PRTLISTNODE pList) + uint32_t fFlags, uint32_t *pcObjects, PRTLISTNODE pList) { AssertPtrReturn(pszRootDir, VERR_INVALID_POINTER); @@ -762,5 +815,5 @@ break; } - if (uFlags & CopyFileFlag_Recursive) + if (fFlags & CopyFileFlag_Recursive) { char *pszNewSub = NULL; @@ -774,5 +827,5 @@ rc = ctrlCopyDirectoryRead(pszRootDir, pszNewSub, pszFilter, pszDest, - uFlags, pcObjects, pList); + fFlags, pcObjects, pList); RTStrFree(pszNewSub); } @@ -783,6 +836,6 @@ case RTDIRENTRYTYPE_SYMLINK: - if ( (uFlags & CopyFileFlag_Recursive) - && (uFlags & CopyFileFlag_FollowLinks)) + if ( (fFlags & CopyFileFlag_Recursive) + && (fFlags & CopyFileFlag_FollowLinks)) { /* Fall through to next case is intentional. */ @@ -852,9 +905,9 @@ * @param pszSource Source path on host to use. * @param pszDest Destination path on guest to use. - * @param uFlags Copy flags. + * @param fFlags Copy flags. * @param pcObjects Where to store the count of objects to be copied. * @param pList Where to store the object list. */ -static int ctrlCopyInit(const char *pszSource, const char *pszDest, uint32_t uFlags, +static int ctrlCopyInit(const char *pszSource, const char *pszDest, uint32_t fFlags, uint32_t *pcObjects, PRTLISTNODE pList) { @@ -958,5 +1011,5 @@ rc = ctrlCopyDirectoryRead(pszSourceAbsRoot, NULL /* Sub directory */, pszFilter, pszDestAbs, - uFlags, pcObjects, pList); + fFlags, pcObjects, pList); if (RT_SUCCESS(rc) && *pcObjects == 0) rc = VERR_NOT_FOUND; @@ -988,9 +1041,9 @@ * @param pszUserName User name on guest to use for the copy operation. * @param pszPassword Password of user account. - * @param uFlags Copy flags. + * @param fFlags Copy flags. */ static int ctrlCopyFileToGuest(IGuest *pGuest, bool fVerbose, const char *pszSource, const char *pszDest, const char *pszUserName, const char *pszPassword, - uint32_t uFlags) + uint32_t fFlags) { AssertPtrReturn(pszSource, VERR_INVALID_PARAMETER); @@ -1003,5 +1056,5 @@ HRESULT rc = pGuest->CopyToGuest(Bstr(pszSource).raw(), Bstr(pszDest).raw(), Bstr(pszUserName).raw(), Bstr(pszPassword).raw(), - uFlags, progress.asOutParam()); + fFlags, progress.asOutParam()); if (FAILED(rc)) vrc = ctrlPrintError(pGuest, COM_IIDOF(IGuest)); @@ -1043,5 +1096,5 @@ Utf8Str Utf8UserName; Utf8Str Utf8Password; - uint32_t uFlags = CopyFileFlag_None; + uint32_t fFlags = CopyFileFlag_None; bool fVerbose = false; bool fCopyRecursive = false; @@ -1062,5 +1115,5 @@ case 'F': /* Follow symlinks */ - uFlags |= CopyFileFlag_FollowLinks; + fFlags |= CopyFileFlag_FollowLinks; break; @@ -1070,5 +1123,5 @@ case 'R': /* Recursive processing */ - uFlags |= CopyFileFlag_Recursive; + fFlags |= CopyFileFlag_Recursive; break; @@ -1139,5 +1192,5 @@ RTLISTNODE listToCopy; uint32_t cObjects = 0; - vrc = ctrlCopyInit(Utf8Source.c_str(), Utf8Dest.c_str(), uFlags, + vrc = ctrlCopyInit(Utf8Source.c_str(), Utf8Dest.c_str(), fFlags, &cObjects, &listToCopy); if (RT_FAILURE(vrc)) @@ -1184,5 +1237,5 @@ if (!fDryRun) vrc = ctrlCopyFileToGuest(guest, fVerbose, pNode->pszSourcePath, pNode->pszDestPath, - Utf8UserName.c_str(), Utf8Password.c_str(), uFlags); + Utf8UserName.c_str(), Utf8Password.c_str(), fFlags); } if (RT_FAILURE(vrc)) @@ -1228,5 +1281,5 @@ Utf8Str Utf8UserName; Utf8Str Utf8Password; - uint32_t uFlags = CreateDirectoryFlag_None; + uint32_t fFlags = CreateDirectoryFlag_None; uint32_t uMode = 0; bool fVerbose = false; @@ -1249,5 +1302,5 @@ case 'P': /* Create parents */ - uFlags |= CreateDirectoryFlag_Parents; + fFlags |= CreateDirectoryFlag_Parents; break; @@ -1314,5 +1367,5 @@ rc = guest->CreateDirectory(Bstr(pNode->pszDestPath).raw(), Bstr(Utf8UserName).raw(), Bstr(Utf8Password).raw(), - uMode, uFlags, progress.asOutParam()); + uMode, fFlags, progress.asOutParam()); if (FAILED(rc)) { @@ -1445,4 +1498,15 @@ int handleGuestControl(HandlerArg *a) { + /** @todo This command does not follow the syntax where the + * comes between the command and subcommand. The commands controlvm, + * snapshot and debugvm puts it between. The commands guestproperty, + * guestcontrol and sharedfolder puts it after (the latter is + * questionable). + * + * I would propose changing guestcontrol and guestproperty to the controlvm + * syntax in 4.1. Whether sharedfolder should be changed as well is a bit + * more open. + */ + HandlerArg arg = *a; arg.argc = a->argc - 1; @@ -1453,26 +1517,23 @@ /* switch (cmd) */ + /** @todo r=bird: Subcommands are not case insensitive for other commands + * like controlvm. Drop it. */ if ( !RTStrICmp(a->argv[0], "exec") || !RTStrICmp(a->argv[0], "execute")) - { return handleCtrlExecProgram(&arg); - } - else if ( !RTStrICmp(a->argv[0], "copyto") - || !RTStrICmp(a->argv[0], "cp")) - { + + if ( !RTStrICmp(a->argv[0], "copyto") + || !RTStrICmp(a->argv[0], "cp")) return handleCtrlCopyTo(&arg); - } - else if ( !RTStrICmp(a->argv[0], "createdirectory") - || !RTStrICmp(a->argv[0], "createdir") - || !RTStrICmp(a->argv[0], "mkdir") - || !RTStrICmp(a->argv[0], "md")) - { + + if ( !RTStrICmp(a->argv[0], "createdirectory") + || !RTStrICmp(a->argv[0], "createdir") + || !RTStrICmp(a->argv[0], "mkdir") + || !RTStrICmp(a->argv[0], "md")) return handleCtrlCreateDirectory(&arg); - } - else if ( !RTStrICmp(a->argv[0], "updateadditions") - || !RTStrICmp(a->argv[0], "updateadds")) - { + + if ( !RTStrICmp(a->argv[0], "updateadditions") + || !RTStrICmp(a->argv[0], "updateadds")) return handleCtrlUpdateAdditions(&arg); - } /* default: */