VirtualBox

Changeset 36035 in vbox for trunk/src/VBox/Frontends


Ignore:
Timestamp:
Feb 21, 2011 2:44:15 PM (14 years ago)
Author:
vboxsync
svn:sync-xref-src-repo-rev:
70144
Message:

VBoxManageGuestCtrl.cpp: Review Todos.

File:
1 edited

Legend:

Unmodified
Added
Removed

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

    r35953 r36035  
    55
    66/*
    7  * Copyright (C) 2011 Oracle Corporation
     7 * Copyright (C) 2010-2011 Oracle Corporation
    88 *
    99 * This file is part of VirtualBox Open Source Edition (OSE), as
    … …  
    7676} DIRECTORYENTRY, *PDIRECTORYENTRY;
    7777
    78 /*
     78/**
    7979 * Special exit codes for returning errors/information of a
    8080 * started guest process to the command line VBoxManage was started from.
    8181 * Useful for e.g. scripting.
     82 * @todo r=bird: RTEXITCODE is using the RT prefix, that is reserved for IPRT
     83 *       use only.
    8284 */
    8385enum RTEXITCODE_EXEC
    … …  
    9193};
    9294
    93 /*
     95/**
    9496 * RTGetOpt-IDs for the guest execution control command line.
     97 * @todo r=bird: RTGETOPTDEF_EXEC is using the RT prefix, that is reserved for
     98 *       IPRT use only.
    9599 */
    96100enum RTGETOPTDEF_EXEC
    … …  
    210214    switch (uStatus)
    211215    {
     216/** @todo r=bird: Why do you use guestControl status codes here? Is the Main
     217 *        API not exposing these constants?  If so, it's something that
     218 *        needs fixing. */
    212219        case guestControl::PROC_STS_STARTED:
    213220            rc = RTEXITCODE_EXEC_SUCCESS;
    214221            break;
    215222        case guestControl::PROC_STS_TEN:
     223            /** @todo check the exit code, 0 is success, !0 should be indicated
     224             *        by a specail VBoxManage exit code. */
    216225            rc = RTEXITCODE_EXEC_SUCCESS;
    217226            break;
    … …  
    231240            /* Service/OS is stopping, process was killed, so
    232241             * not exactly an error of the started process ... */
    233             rc = RTEXITCODE_EXEC_SUCCESS;
     242            rc = RTEXITCODE_EXEC_SUCCESS; /** @todo r=bird: return failure if you don't know what happend. No false positives, please. */
    234243            break;
    235244        case guestControl::PROC_STS_ERROR:
    … …  
    370379        //{ "--output-format",                RTGETOPTDEF_EXEC_OUTPUTFORMAT,              RTGETOPT_REQ_STRING  },
    371380        { "--output-type",                  RTGETOPTDEF_EXEC_OUTPUTTYPE,                RTGETOPT_REQ_STRING  },
     381        /** @todo r=bird: output-type and output-format are the same thing.
     382         *        Please just call it --dos2unix and --unix2dos and
     383         *        implement it them.   (If you insist on flexibility, it would
     384         *        be --input-type <x> and --output-type <y>, where x and y
     385         *        could indicate line ending styles (CR/LF/CRLF), encodings
     386         *        (UTF-8, UTF-16, ++), code pages and plugin provided
     387         *        conversions.  See the conv option of dd and iconv for
     388         *        inspiration.)
     389         *
     390         *        The default must be no conversion at all, i.e.
     391         *        --output-type=binary. */
    372392        { "--password",                     'p',                                        RTGETOPT_REQ_STRING  },
    373393        { "--timeout",                      't',                                        RTGETOPT_REQ_UINT32  },
    … …  
    377397        { "--wait-stdout",                  RTGETOPTDEF_EXEC_WAITFORSTDOUT,             RTGETOPT_REQ_NOTHING },
    378398        { "--wait-stderr",                  RTGETOPTDEF_EXEC_WAITFORSTDERR,             RTGETOPT_REQ_NOTHING },
     399        /* @todo r=bird: '--' is interpreted by RTGetOpt() to indicate the
     400         * end of arguments, you don't need to handle this specially.
     401         * RTGetOpt() will return VINF_GETOPT_NOT_OPTION for anything following
     402         * a '--' argument.  So, let me give you some other examples for illustrating
     403         * what I mean should work now (all the same, userid/pw missing):
     404         *  VBoxManage guestcontrol myvm exec --image /bin/busybox cp /foo /bar
     405         *  VBoxManage guestcontrol myvm exec --image /bin/busybox -- cp /foo /bar
     406         *  VBoxManage guestcontrol myvm exec --image /bin/busybox cp -- /foo /bar
     407         *  VBoxManage guestcontrol myvm exec --image /bin/busybox cp /foo -- /bar
     408         *
     409         * Same, but with standard cp (image defaults to /bin/cp):
     410         *  VBoxManage guestcontrol myvm exec /bin/cp /foo /bar
     411         *  VBoxManage guestcontrol myvm exec -- /bin/cp /foo /bar
     412         *  VBoxManage guestcontrol myvm exec /bin/cp -- /foo /bar
     413         *  VBoxManage guestcontrol myvm exec /bin/cp /foo -- /bar
     414         *
     415         * The old example:
     416         *  VBoxManage guestcontrol myvm exec --image /bin/busybox -- ln -s /foo /bar
     417         *
     418         * As an example of where '--' is used by standard linux utils, try delete a
     419         * file named '-f' without specifying the directory:
     420         *      > -f        # creates the file
     421         *      rm -v -f    # does nothing because '-f' is a rm option.
     422         *      rm -v -- -f # does the trick because '--' tells rm to not read '-f' as an option.
     423         */
    379424        { "--",                             RTGETOPTDEF_EXEC_ARGS,                      RTGETOPT_REQ_STRING }
    380425    };
    … …  
    391436    Utf8Str                 Utf8UserName;
    392437    Utf8Str                 Utf8Password;
    393     uint32_t                u32TimeoutMS    = 0;
     438    uint32_t                cMsTimeout      = 0;
    394439    bool                    fOutputBinary   = false;
    395440    bool                    fWaitForExit    = false;
    … …  
    413458                if (RT_FAILURE(vrc))
    414459                    return errorSyntax(USAGE_GUESTCONTROL, "Failed to parse environment value, rc=%Rrc", vrc);
    415                 else
    416                 {
    417                     for (int j = 0; j < cArgs; j++)
    418                         env.push_back(Bstr(papszArg[j]).raw());
    419 
    420                     RTGetOptArgvFree(papszArg);
    421                 }
     460                for (int j = 0; j < cArgs; j++)
     461                    env.push_back(Bstr(papszArg[j]).raw());
     462
     463                RTGetOptArgvFree(papszArg);
    422464                break;
    423465            }
    … …  
    431473                break;
    432474
    433             /*case RTGETOPTDEF_EXEC_OUTPUTFORMAT:
    434                 /** todo Add DOS2UNIX and vice versa handling!
     475            /** @todo Add DOS2UNIX and vice versa handling!
     476              case RTGETOPTDEF_EXEC_OUTPUTFORMAT:
    435477                break;*/
    436478
    … …  
    455497
    456498            case 't': /* Timeout */
    457                 u32TimeoutMS = ValueUnion.u32;
     499                cMsTimeout = ValueUnion.u32;
    458500                break;
    459501
    … …  
    523565    }
    524566
    525     if (RT_FAILURE(vrc))
     567    if (RT_FAILURE(vrc)) /** @todo r=bird: You don't need to check vrc here or in any of the two while conditions above because you are now returning directly in those cases. Drop the checks, this will keep things simpler. */
    526568    {
    527569        RTMsgError("Failed to parse argument '%c', rc=%Rrc", ch, vrc);
    … …  
    535577        return errorSyntax(USAGE_GUESTCONTROL, "No user name specified!");
    536578
     579    /*
     580     * <missing comment indicating that we're done parsing args and started doing something else>
     581     */
    537582    HRESULT rc = S_OK;
    538583    if (fVerbose)
    539584    {
    540         if (u32TimeoutMS == 0)
     585        if (cMsTimeout == 0)
    541586            RTPrintf("Waiting for guest to start process ...\n");
    542587        else
    543             RTPrintf("Waiting for guest to start process (within %ums)\n", u32TimeoutMS);
     588            RTPrintf("Waiting for guest to start process (within %ums)\n", cMsTimeout);
    544589    }
    545590
    … …  
    548593
    549594    /* Execute the process. */
    550     int rcProc = RTEXITCODE_EXEC_SUCCESS;
     595    int rcProc = RTEXITCODE_EXEC_SUCCESS;   /** @todo r=bird: Don't initialize this, please, set it explicitly in the various branches.  It's easier to see what it's going to be that way. */
    551596    ComPtr<IProgress> progress;
    552597    ULONG uPID = 0;
    … …  
    557602                               Bstr(Utf8UserName).raw(),
    558603                               Bstr(Utf8Password).raw(),
    559                                u32TimeoutMS,
     604                               cMsTimeout,
    560605                               &uPID,
    561606                               progress.asOutParam());
    562607    if (FAILED(rc))
    563         vrc = ctrlPrintError(guest, COM_IIDOF(IGuest));
     608        vrc = ctrlPrintError(guest, COM_IIDOF(IGuest)); /** @todo return straight away and drop state (e.g. rcProc) confusion. */
    564609    else
    565610    {
    … …  
    568613        if (fWaitForExit)
    569614        {
    570             if (u32TimeoutMS) /* Wait with a certain timeout. */
     615            if (fVerbose)
    571616            {
    572                 /* Calculate timeout value left after process has been started.  */
    573                 uint64_t u64Elapsed = RTTimeMilliTS() - u64StartMS;
    574                 /* Is timeout still bigger than current difference? */
    575                 if (u32TimeoutMS > u64Elapsed)
    576                 {
    577                     if (fVerbose)
    578                         RTPrintf("Waiting for process to exit (%ums left) ...\n", u32TimeoutMS - u64Elapsed);
    579                 }
    580                 else
    581                 {
    582                     if (fVerbose)
    583                         RTPrintf("No time left to wait for process!\n");
    584                 }
     617                if (cMsTimeout) /* Wait with a certain timeout. */
     618                {
     619                    /* Calculate timeout value left after process has been started.  */
     620                    uint64_t u64Elapsed = RTTimeMilliTS() - u64StartMS;
     621                    /* Is timeout still bigger than current difference? */
     622                    if (cMsTimeout > u64Elapsed)
     623                        RTPrintf("Waiting for process to exit (%ums left) ...\n", cMsTimeout - u64Elapsed);
     624                    else
     625                        RTPrintf("No time left to wait for process!\n"); /** @todo a bit misleading ... */
     626                }
     627                else /* Wait forever. */
     628                    RTPrintf("Waiting for process to exit ...\n");
    585629            }
    586             else if (fVerbose) /* Wait forever. */
    587                 RTPrintf("Waiting for process to exit ...\n");
    588630
    589631            /* Setup signal handling if cancelable. */
    … …  
    612654                    || fWaitForStdErr)
    613655                {
     656                    /** @todo r=bird: The timeout argument is bogus in several
     657                     * ways:
     658                     *  1. RT_MAX will evaluate the arguments twice, which may
     659                     *     result in different values because RTTimeMilliTS()
     660                     *     returns a higher value the 2nd time. Worst case:
     661                     *     Imagine when RT_MAX calculates the remaining time
     662                     *     out (first expansion) there is say 60 ms left.  Then
     663                     *     we're preempted and rescheduled after, say, 120 ms.
     664                     *     We call RTTimeMilliTS() again and ends up with a
     665                     *     value -60 ms, which translate to a UINT32_MAX - 59
     666                     *     ms timeout.
     667                     *
     668                     *  2. When the period expires, we will wait forever since
     669                     *     both 0 and -1 mean indefinite timeout with this API,
     670                     *     at least that's one way of reading the main code.
     671                     *
     672                     *  3. There is a signed/unsigned ambiguity in the
     673                     *     RT_MAX expression.  The left hand side is signed
     674                     *     integer (0), the right side is unsigned 64-bit. From
     675                     *     what I can tell, the compiler will treat this as
     676                     *     unsigned 64-bit and never return 0.
     677                     */
     678                    /** @todo r=bird: We must separate stderr and stdout
     679                     *        output, seems bunched together here which
     680                     *        won't do the trick for unix BOFHs. */
    614681                    rc = guest->GetProcessOutput(uPID, 0 /* aFlags */,
    615                                                  RT_MAX(0, u32TimeoutMS - (RTTimeMilliTS() - u64StartMS)) /* Timeout in ms */,
     682                                                 RT_MAX(0, cMsTimeout - (RTTimeMilliTS() - u64StartMS)) /* Timeout in ms */,
    616683                                                 _64K, ComSafeArrayAsOutParam(aOutputData));
    617684                    if (FAILED(rc))
    … …  
    685752
    686753                /* Did we run out of time? */
    687                 if (   u32TimeoutMS
    688                     && RTTimeMilliTS() - u64StartMS > u32TimeoutMS)
     754                if (   cMsTimeout
     755                    && RTTimeMilliTS() - u64StartMS > cMsTimeout)
    689756                {
    690757                    progress->Cancel();
    … …  
    705772            }
    706773            else if (   fCompleted
    707                      && SUCCEEDED(rc))
     774                     && SUCCEEDED(rc)) /* The GetProcessOutput rc. */
    708775            {
    709776                LONG iRc;
    … …  
    717784                    if (SUCCEEDED(rc) && fVerbose)
    718785                        RTPrintf("Exit code=%u (Status=%u [%s], Flags=%u)\n", uRetExitCode, uRetStatus, ctrlExecProcessStatusToText(uRetStatus), uRetFlags);
     786                    /** @todo r=bird: This isn't taking uRetExitCode into
     787                     *        account. We MUST give the users a way to indicate
     788                     *        whether the guest program terminated with an 0 or
     789                     *        non-zero exit code. */
    719790                    rcProc = ctrlExecProcessStatusToExitCode(uRetStatus);
    720791                }
    … …  
    724795                if (fVerbose)
    725796                    RTPrintf("Process execution aborted!\n");
     797                /** @todo r=bird: Should set rcProc here, this is not a
     798                 *        success branch. */
    726799            }
    727800        }
    … …  
    11251198
    11261199    /*
    1127      * Check the syntax.  We can deduce the correct syntax from the number of
    1128      * arguments.
     1200     * Check the syntax.
    11291201     */
    11301202    if (pArg->argc < 2) /* At least the source + destination should be present :-). */
    … …  
    11561228
    11571229    int vrc = VINF_SUCCESS;
    1158     uint32_t uNoOptionIdx = 0;
     1230    uint32_t idxNonOption = 0;
    11591231    bool fUsageOK = true;
    11601232    while (   (ch = RTGetOpt(&GetState, &ValueUnion))
    … …  
    11911263            {
    11921264                /* Get the actual source + destination. */
    1193                 switch (uNoOptionIdx)
     1265                switch (idxNonOption)
    11941266                {
    11951267                    case 0:
    … …  
    12021274
    12031275                    default:
     1276                        /** @todo r=bird: {2..UINT32_MAX-1} goes unnoticed
     1277                         *        to /dev/null? That doesn't make much
     1278                         *        sense... Why not just return with a syntax
     1279                         *        error straight away? */
    12041280                        break;
    12051281                }
    1206                 uNoOptionIdx++;
    1207                 if (uNoOptionIdx == UINT32_MAX)
     1282                idxNonOption++;
     1283                if (idxNonOption == UINT32_MAX)
    12081284                {
    12091285                    RTMsgError("Too many files specified! Aborting.\n");
    1210                     vrc = VERR_TOO_MUCH_DATA;
     1286                    vrc = VERR_TOO_MUCH_DATA; /** @todo r=bird: return straight away, drop the RT_SUCCESS(vrc) from the while condition.  Keep things simple. */
    12111287                }
    12121288                break;
    … …  
    12181294    }
    12191295
    1220     if (!fUsageOK)
     1296    if (!fUsageOK) /** @todo r=bird: fUsageOK - never set to false, just drop it.  Keep the state simple. */
    12211297        return errorSyntax(USAGE_GUESTCONTROL, "Incorrect parameters");
    12221298
    … …  
    12321308        return errorSyntax(USAGE_GUESTCONTROL,
    12331309                           "No user name specified!");
     1310
     1311    /*
     1312     * Done parsing arguments, do stuff.
     1313     */
    12341314    HRESULT rc = S_OK;
    12351315    if (fVerbose)
    … …  
    12761356            }
    12771357
    1278             uint32_t uCurObject = 1;
     1358            uint32_t iObject = 1;
    12791359            RTListForEach(&listToCopy, pNode, DIRECTORYENTRY, Node)
    12801360            {
    … …  
    12831363                    if (fVerbose)
    12841364                        RTPrintf("Copying \"%s\" to \"%s\" (%u/%u) ...\n",
    1285                                  pNode->pszSourcePath, pNode->pszDestPath, uCurObject, cObjects);
     1365                                 pNode->pszSourcePath, pNode->pszDestPath, iObject, cObjects);
    12861366                    /* Finally copy the desired file (if no dry run selected). */
    12871367                    if (!fDryRun)
    … …  
    12911371                if (RT_FAILURE(vrc))
    12921372                    break;
    1293                 uCurObject++;
     1373                iObject++;
    12941374            }
    12951375            if (RT_SUCCESS(vrc) && fVerbose)
    … …  
    13361416
    13371417    RTLISTNODE listDirs;
    1338     uint32_t uNumDirs = 0;
     1418    uint32_t cDirs = 0;
    13391419    RTListInit(&listDirs);
    13401420
    … …  
    13741454                if (RT_SUCCESS(vrc))
    13751455                {
    1376                     uNumDirs++;
    1377                     if (uNumDirs == UINT32_MAX)
     1456                    cDirs++;
     1457                    if (cDirs == UINT32_MAX)
    13781458                    {
    13791459                        RTMsgError("Too many directories specified! Aborting.\n");
    … …  
    13921472        return errorSyntax(USAGE_GUESTCONTROL, "Incorrect parameters");
    13931473
    1394     if (!uNumDirs)
     1474    if (!cDirs)
    13951475        return errorSyntax(USAGE_GUESTCONTROL,
    13961476                           "No directory to create specified!");
    … …  
    14011481
    14021482    HRESULT rc = S_OK;
    1403     if (fVerbose && uNumDirs > 1)
    1404         RTPrintf("Creating %u directories ...\n", uNumDirs);
     1483    if (fVerbose && cDirs > 1)
     1484        RTPrintf("Creating %u directories ...\n", cDirs);
    14051485
    14061486    PDIRECTORYENTRY pNode;
    … …  
    14161496        if (FAILED(rc))
    14171497        {
    1418             /* rc ignored */ ctrlPrintError(guest, COM_IIDOF(IGuest));
     1498            ctrlPrintError(guest, COM_IIDOF(IGuest)); /* (return code ignored, save original rc) */
    14191499            break;
    14201500        }
    … …  
    14321512     * arguments.
    14331513     */
    1434     if (pArg->argc < 1) /* At least the VM name should be present :-). */
     1514    if (pArg->argc < 1) /* At least the VM name should be present :-). */ /** @todo r=bird: This is no longer correct. Ditto for all other handlers. */
    14351515        return errorSyntax(USAGE_GUESTCONTROL, "Incorrect parameters");
    14361516
    … …  
    15341614 * Access the guest control store.
    15351615 *
    1536  * @returns 0 on success, 1 on failure
     1616 * @returns program exit code.
    15371617 * @note see the command line API description for parameters
    15381618 */
Note: See TracChangeset for help on using the changeset viewer.

© 2025 Oracle Support Privacy / Do Not Sell My Info Terms of Use Trademark Policy Automated Access Etiquette