Changeset 55588 in vbox for trunk/src/VBox/Main

Timestamp:

May 1, 2015 7:37:46 PM (10 years ago)

Author:

vboxsync

Message:

Main: Environment cleanup in the guest control area. The GuestEnvironment class implementation has been replaced by one based on IPRT's RTEnv API and it has been changed into a way of recording environment changes, thus renamed to GuestEnvironmentChanges.

Location:

trunk/src/VBox/Main

Files:

• include/GuestCtrlImplPrivate.h (modified) (4 diffs)
• include/GuestSessionImpl.h (modified) (4 diffs)
• src-client/GuestCtrlPrivate.cpp (modified) (2 diffs)
• src-client/GuestProcessImpl.cpp (modified) (6 diffs)
• src-client/GuestSessionImpl.cpp (modified) (16 diffs)

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

@@ -6 +6 @@
 
 /*
- * Copyright (C) 2011-2013 Oracle Corporation
+ * Copyright (C) 2011-2015 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -21 +21 @@
 
 #include "ConsoleImpl.h"
+#include "Global.h"
 
 #include <iprt/asm.h>
+#include <iprt/env.h>
 #include <iprt/semaphore.h>
 
@@ -60 +62 @@
 
 
-typedef std::vector <Utf8Str> GuestEnvironmentArray;
-class GuestEnvironment
-{
-public:
-
-    int BuildEnvironmentBlock(void **ppvEnv, size_t *pcbEnv, uint32_t *pcEnvVars);
-
-    void Clear(void);
-
-    int CopyFrom(const GuestEnvironmentArray &environment);
-
-    int CopyTo(GuestEnvironmentArray &environment);
-
-    static void FreeEnvironmentBlock(void *pvEnv);
-
-    Utf8Str Get(const Utf8Str &strKey);
-
-    Utf8Str Get(size_t nPos);
-
-    bool Has(const Utf8Str &strKey);
-
-    int Set(const Utf8Str &strKey, const Utf8Str &strValue);
-
-    int Set(const Utf8Str &strPair);
-
-    size_t Size(void);
-
-    int Unset(const Utf8Str &strKey);
-
-public:
-
-    GuestEnvironment& operator=(const GuestEnvironmentArray &that);
-
-    GuestEnvironment& operator=(const GuestEnvironment &that);
-
-protected:
-
-    int appendToEnvBlock(const char *pszEnv, void **ppvList, size_t *pcbList, uint32_t *pcEnvVars);
-
-protected:
-
-    std::map <Utf8Str, Utf8Str> mEnvironment;
+
+/**
+ * Wrapper around the RTEnv API, unusable base class.
+ *
+ * @remarks Feel free to elevate this class to iprt/cpp/env.h as RTCEnv.
+ */
+class GuestEnvironmentBase
+{
+public:
+    /**
+     * Default constructor.
+     *
+     * The user must invoke one of the init methods before using the object.
+     */
+    GuestEnvironmentBase(void)
+        : m_hEnv(NIL_RTENV)
+    { }
+
+    /**
+     * Destructor.
+     */
+    virtual ~GuestEnvironmentBase(void)
+    {
+        int rc = RTEnvDestroy(m_hEnv); AssertRC(rc);
+        m_hEnv = NIL_RTENV;
+    }
+
+    /**
+     * Initialize this as a normal environment block.
+     * @returns IPRT status code.
+     */
+    int initNormal(void)
+    {
+        AssertReturn(m_hEnv == NIL_RTENV, VERR_WRONG_ORDER);
+        return RTEnvCreate(&m_hEnv);
+    }
+
+    /**
+     * Returns the variable count.
+     * @return Number of variables.
+     * @sa      RTEnvCountEx
+     */
+    uint32_t count(void) const
+    {
+        return RTEnvCountEx(m_hEnv);
+    }
+
+    /**
+     * Deletes the environment change record entirely.
+     *
+     * The count() method will return zero after this call.
+     *
+     * @sa      RTEnvReset
+     */
+    void reset(void)
+    {
+        int rc = RTEnvReset(m_hEnv);
+        AssertRC(rc);
+    }
+
+    /**
+     * Exports the environment change block as an array of putenv style strings.
+     *
+     *
+     * @returns VINF_SUCCESS or VERR_NO_MEMORY.
+     * @param   pArray              The output array.
+     */
+    int queryPutEnvArray(std::vector<com::Utf8Str> *pArray) const
+    {
+        uint32_t cVars = RTEnvCountEx(m_hEnv);
+        try
+        {
+            pArray->resize(cVars);
+            for (uint32_t iVar = 0; iVar < cVars; iVar++)
+            {
+                const char *psz = RTEnvGetByIndexRawEx(m_hEnv, iVar);
+                AssertReturn(psz, VERR_INTERNAL_ERROR_3); /* someone is racing us! */
+                (*pArray)[iVar] = psz;
+            }
+            return VINF_SUCCESS;
+        }
+        catch (std::bad_alloc &)
+        {
+            return VERR_NO_MEMORY;
+        }
+    }
+
+    /**
+     * Applies an array of putenv style strings.
+     *
+     * @returns IPRT status code.
+     * @param   rArray              The array with the putenv style strings.
+     * @sa      RTEnvPutEnvEx
+     */
+    int applyPutEnvArray(const std::vector<com::Utf8Str> &rArray)
+    {
+        size_t cArray = rArray.size();
+        for (size_t i = 0; i < cArray; i++)
+        {
+            int rc = RTEnvPutEx(m_hEnv, rArray[i].c_str());
+            if (RT_FAILURE(rc))
+                return rc;
+        }
+        return VINF_SUCCESS;
+    }
+
+    /**
+     * See RTEnvQueryUtf8Block for details.
+     * @returns IPRT status code.
+     * @param   ppszzBlock      Where to return the block pointer.
+     * @param   pcbBlock        Where to optionally return the block size.
+     * @sa      RTEnvQueryUtf8Block
+     */
+    int queryUtf8Block(char **ppszzBlock, size_t *pcbBlock)
+    {
+        return RTEnvQueryUtf8Block(m_hEnv, true /*fSorted*/, ppszzBlock, pcbBlock);
+    }
+
+    /**
+     * Frees what queryUtf8Block returned, NULL ignored.
+     * @sa      RTEnvFreeUtf8Block
+     */
+    static void freeUtf8Block(char *pszzBlock)
+    {
+        return RTEnvFreeUtf8Block(pszzBlock);
+    }
+
+    /**
+     * Get an environment variable.
+     *
+     * @returns IPRT status code.
+     * @param   rName               The variable name.
+     * @param   pValue              Where to return the value.
+     * @sa      RTEnvGetEx
+     */
+    int getVariable(const com::Utf8Str &rName, com::Utf8Str *pValue) const
+    {
+        size_t cchNeeded;
+        int rc = RTEnvGetEx(m_hEnv, rName.c_str(), NULL, 0, &cchNeeded);
+        if (   RT_SUCCESS(rc)
+            || rc == VERR_BUFFER_OVERFLOW)
+        {
+            try
+            {
+                pValue->reserve(cchNeeded + 1);
+                rc = RTEnvGetEx(m_hEnv, rName.c_str(), pValue->mutableRaw(), pValue->capacity(), NULL);
+                pValue->jolt();
+            }
+            catch (std::bad_alloc &)
+            {
+                rc = VERR_NO_STR_MEMORY;
+            }
+        }
+        return rc;
+    }
+
+    /**
+     * Set an environment variable.
+     *
+     * @returns IPRT status code.
+     * @param   rName               The variable name.
+     * @param   rValue              The value of the variable.
+     * @sa      RTEnvSetEx
+     */
+    int setVariable(const com::Utf8Str &rName, const com::Utf8Str &rValue)
+    {
+        return RTEnvSetEx(m_hEnv, rName.c_str(), rValue.c_str());
+    }
+
+    /**
+     * Unset an environment variable.
+     *
+     * @returns IPRT status code.
+     * @param   rName               The variable name.
+     * @sa      RTEnvUnsetEx
+     */
+    int unsetVariable(const com::Utf8Str &rName)
+    {
+        return RTEnvUnsetEx(m_hEnv, rName.c_str());
+    }
+
+#if 0
+private:
+    /* No copy operator. */
+    GuestEnvironmentBase(const GuestEnvironmentBase &) { throw E_FAIL; }
+#else
+    /**
+     * Copy constructor.
+     * @throws HRESULT
+     */
+    GuestEnvironmentBase(const GuestEnvironmentBase &rThat, bool fChangeRecord)
+        : m_hEnv(NIL_RTENV)
+    {
+        int rc = cloneCommon(rThat, fChangeRecord);
+        if (RT_FAILURE(rc))
+            throw (Global::vboxStatusCodeToCOM(rc));
+    }
+#endif
+
+protected:
+    /**
+     * Common clone/copy method with type conversion abilities.
+     *
+     * @returns IPRT status code.
+     * @param   rThat           The object to clone.
+     * @param   fChangeRecord   Whether the this instance is a change record (true)
+     *                          or normal (false) environment.
+     */
+    int cloneCommon(const GuestEnvironmentBase &rThat, bool fChangeRecord)
+    {
+        int   rc = VINF_SUCCESS;
+        RTENV hNewEnv = NIL_RTENV;
+        if (rThat.m_hEnv != NIL_RTENV)
+        {
+            if (RTEnvIsChangeRecord(rThat.m_hEnv) == fChangeRecord)
+                rc = RTEnvClone(&hNewEnv, rThat.m_hEnv);
+            else
+            {
+                /* Need to type convert it. */
+                if (fChangeRecord)
+                    rc = RTEnvCreateChangeRecord(&hNewEnv);
+                else
+                    rc = RTEnvCreate(&hNewEnv);
+                if (RT_SUCCESS(rc))
+                {
+                    rc = RTEnvApplyChanges(hNewEnv, rThat.m_hEnv);
+                    if (RT_FAILURE(rc))
+                        RTEnvDestroy(hNewEnv);
+                }
+            }
+
+        }
+        if (RT_SUCCESS(rc))
+        {
+            RTEnvDestroy(m_hEnv);
+            m_hEnv = hNewEnv;
+        }
+        return rc;
+    }
+
+
+    /** The environment change record. */
+    RTENV       m_hEnv;
+};
+
+
+#if 0 /* Not currently used. */
+/**
+ * Wrapper around the RTEnv API for a normal environment.
+ */
+class GuestEnvironment : public GuestEnvironmentBase
+{
+public:
+    /**
+     * Default constructor.
+     *
+     * The user must invoke one of the init methods before using the object.
+     */
+    GuestEnvironment(void)
+        : GuestEnvironmentBase()
+    { }
+
+    /**
+     * Copy operator.
+     * @param   rThat       The object to copy.
+     * @throws HRESULT
+     */
+    GuestEnvironment(const GuestEnvironment &rThat)
+        : GuestEnvironmentBase(rThat, false /*fChangeRecord*/)
+    { }
+
+    /**
+     * Copy operator.
+     * @param   rThat       The object to copy.
+     * @throws HRESULT
+     */
+    GuestEnvironment(const GuestEnvironmentBase &rThat)
+        : GuestEnvironmentBase(rThat, false /*fChangeRecord*/)
+    { }
+
+    /**
+     * Initialize this as a normal environment block.
+     * @returns IPRT status code.
+     */
+    int initNormal(void)
+    {
+        AssertReturn(m_hEnv == NIL_RTENV, VERR_WRONG_ORDER);
+        return RTEnvCreate(&m_hEnv);
+    }
+
+    /**
+     * Replaces this environemnt with that in @a rThat.
+     *
+     * @returns IPRT status code
+     * @param   rThat       The environment to copy. If it's a different type
+     *                      we'll convert the data to a normal environment block.
+     */
+    int copy(const GuestEnvironmentBase &rThat)
+    {
+        return cloneCommon(rThat, false /*fChangeRecord*/);
+    }
+};
+#endif /* unused */
+
+
+/**
+ * Wrapper around the RTEnv API for a environment change record.
+ *
+ * This class is used as a record of changes to be applied to a different
+ * environment block (in VBoxService before launching a new process).
+ */
+class GuestEnvironmentChanges : public GuestEnvironmentBase
+{
+public:
+    /**
+     * Default constructor.
+     *
+     * The user must invoke one of the init methods before using the object.
+     */
+    GuestEnvironmentChanges(void)
+        : GuestEnvironmentBase()
+    { }
+
+    /**
+     * Copy operator.
+     * @param   rThat       The object to copy.
+     * @throws HRESULT
+     */
+    GuestEnvironmentChanges(const GuestEnvironmentChanges &rThat)
+        : GuestEnvironmentBase(rThat, true /*fChangeRecord*/)
+    { }
+
+    /**
+     * Copy operator.
+     * @param   rThat       The object to copy.
+     * @throws HRESULT
+     */
+    GuestEnvironmentChanges(const GuestEnvironmentBase &rThat)
+        : GuestEnvironmentBase(rThat, true /*fChangeRecord*/)
+    { }
+
+    /**
+     * Initialize this as a environment change record.
+     * @returns IPRT status code.
+     */
+    int initChangeRecord(void)
+    {
+        AssertReturn(m_hEnv == NIL_RTENV, VERR_WRONG_ORDER);
+        return RTEnvCreateChangeRecord(&m_hEnv);
+    }
+
+    /**
+     * Replaces this environemnt with that in @a rThat.
+     *
+     * @returns IPRT status code
+     * @param   rThat       The environment to copy. If it's a different type
+     *                      we'll convert the data to a set of changes.
+     */
+    int copy(const GuestEnvironmentBase &rThat)
+    {
+        return cloneCommon(rThat, true /*fChangeRecord*/);
+    }
 };
 
@@ -225 +547 @@
     /** Arguments vector (starting with argument \#0). */
     ProcessArguments            mArguments;
-    GuestEnvironment            mEnvironment;
+    /** The process environment change record.  */
+    GuestEnvironmentChanges     mEnvironment;
     /** Process creation flags. */
     uint32_t                    mFlags;

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

@@ -391 +391 @@
 public:
     /** @name Public internal methods.
+     * @todo r=bird: Most of these are public for no real reason...
      * @{ */
     int                     i_closeSession(uint32_t uFlags, uint32_t uTimeoutMS, int *pGuestRc);
@@ -415 +416 @@
     int                     i_fsQueryInfoInternal(const Utf8Str &strPath, GuestFsObjData &objData, int *pGuestRc);
     const GuestCredentials &i_getCredentials(void);
-    const GuestEnvironment &i_getEnvironment(void);
     EventSource            *i_getEventSource(void) { return mEventSource; }
     Utf8Str                 i_getName(void);
@@ -441 +441 @@
     int                     i_startTaskAsync(const Utf8Str &strTaskDesc, GuestSessionTask *pTask,
                                              ComObjPtr<Progress> &pProgress);
-    int                     i_queryInfo(void);
+    int                     i_determineProtocolVersion(void);
     int                     i_waitFor(uint32_t fWaitFlags, ULONG uTimeoutMS, GuestSessionWaitResult_T &waitResult, int *pGuestRc);
     int                     i_waitForStatusChange(GuestWaitEvent *pEvent, uint32_t fWaitFlags, uint32_t uTimeoutMS,
@@ -470 +470 @@
         /** The session's current status. */
         GuestSessionStatus_T        mStatus;
-        /** The session's environment block. Can be
-         *  overwritten/extended by ProcessCreate(Ex). */
-        GuestEnvironment            mEnvironment;
+        /** The set of environment changes for the session for use when
+         *  creating new guest processes. */
+        GuestEnvironmentChanges     mEnvironment;
         /** Directory objects bound to this session. */
         SessionDirectories          mDirectories;

trunk/src/VBox/Main/src-client/GuestCtrlPrivate.cpp

@@ -1 +1 @@
 /* $Id$ */
 /** @file
- *
  * Internal helpers/structures for guest control functionality.
  */
 
 /*
- * Copyright (C) 2011-2014 Oracle Corporation
+ * Copyright (C) 2011-2015 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -37 +36 @@
 #include <VBox/log.h>
 
-/******************************************************************************
- *   Structures and Typedefs                                                  *
- ******************************************************************************/
-
-int GuestEnvironment::BuildEnvironmentBlock(void **ppvEnv, size_t *pcbEnv, uint32_t *pcEnvVars)
-{
-    AssertPtrReturn(ppvEnv, VERR_INVALID_POINTER);
-    /* Rest is optional. */
-
-    size_t cbEnv = 0;
-    uint32_t cEnvVars = 0;
-
-    int rc = VINF_SUCCESS;
-
-    size_t cEnv = mEnvironment.size();
-    if (cEnv)
-    {
-        std::map<Utf8Str, Utf8Str>::const_iterator itEnv = mEnvironment.begin();
-        for (; itEnv != mEnvironment.end() && RT_SUCCESS(rc); itEnv++)
-        {
-            char *pszEnv;
-            if (!RTStrAPrintf(&pszEnv, "%s=%s", itEnv->first.c_str(), itEnv->second.c_str()))
-            {
-                rc = VERR_NO_MEMORY;
-                break;
-            }
-            AssertPtr(pszEnv);
-            rc = appendToEnvBlock(pszEnv, ppvEnv, &cbEnv, &cEnvVars);
-            RTStrFree(pszEnv);
-        }
-        Assert(cEnv == cEnvVars);
-    }
-
-    if (pcbEnv)
-        *pcbEnv = cbEnv;
-    if (pcEnvVars)
-        *pcEnvVars = cEnvVars;
-
-    return rc;
-}
-
-void GuestEnvironment::Clear(void)
-{
-    mEnvironment.clear();
-}
-
-int GuestEnvironment::CopyFrom(const GuestEnvironmentArray &environment)
-{
-    int rc = VINF_SUCCESS;
-
-    for (GuestEnvironmentArray::const_iterator it = environment.begin();
-         it != environment.end() && RT_SUCCESS(rc);
-         ++it)
-    {
-        rc = Set((*it));
-    }
-
-    return rc;
-}
-
-int GuestEnvironment::CopyTo(GuestEnvironmentArray &environment)
-{
-    size_t s = 0;
-    for (std::map<Utf8Str, Utf8Str>::const_iterator it = mEnvironment.begin();
-         it != mEnvironment.end();
-         ++it, ++s)
-    {
-        environment[s] = Bstr(it->first + "=" + it->second).raw();
-    }
-
-    return VINF_SUCCESS;
-}
-
-/* static */
-void GuestEnvironment::FreeEnvironmentBlock(void *pvEnv)
-{
-    if (pvEnv)
-        RTMemFree(pvEnv);
-}
-
-Utf8Str GuestEnvironment::Get(size_t nPos)
-{
-    size_t curPos = 0;
-    std::map<Utf8Str, Utf8Str>::const_iterator it = mEnvironment.begin();
-    for (; it != mEnvironment.end() && curPos < nPos;
-         ++it, ++curPos) { }
-
-    if (it != mEnvironment.end())
-        return Utf8Str(it->first + "=" + it->second);
-
-    return Utf8Str("");
-}
-
-Utf8Str GuestEnvironment::Get(const Utf8Str &strKey)
-{
-    std::map <Utf8Str, Utf8Str>::const_iterator itEnv = mEnvironment.find(strKey);
-    Utf8Str strRet;
-    if (itEnv != mEnvironment.end())
-        strRet = itEnv->second;
-    return strRet;
-}
-
-bool GuestEnvironment::Has(const Utf8Str &strKey)
-{
-    std::map <Utf8Str, Utf8Str>::const_iterator itEnv = mEnvironment.find(strKey);
-    return (itEnv != mEnvironment.end());
-}
-
-int GuestEnvironment::Set(const Utf8Str &strKey, const Utf8Str &strValue)
-{
-    /** @todo Do some validation using regex. */
-    if (strKey.isEmpty())
-        return VERR_INVALID_PARAMETER;
-
-    int rc = VINF_SUCCESS;
-    const char *pszString = strKey.c_str();
-    while (*pszString != '\0' && RT_SUCCESS(rc))
-    {
-         if (   !RT_C_IS_ALNUM(*pszString)
-             && !RT_C_IS_GRAPH(*pszString))
-             rc = VERR_INVALID_PARAMETER;
-         *pszString++;
-    }
-
-    if (RT_SUCCESS(rc))
-        mEnvironment[strKey] = strValue;
-
-    return rc;
-}
-
-int GuestEnvironment::Set(const Utf8Str &strPair)
-{
-    RTCList<RTCString> listPair = strPair.split("=", RTCString::KeepEmptyParts);
-    /* Skip completely empty pairs. Note that we still need pairs with a valid
-     * (set) key and an empty value. */
-    if (listPair.size() <= 1)
-        return VINF_SUCCESS;
-
-    int rc = VINF_SUCCESS;
-    size_t p = 0;
-    while (p < listPair.size() && RT_SUCCESS(rc))
-    {
-        Utf8Str strKey = listPair.at(p++);
-        if (   strKey.isEmpty()
-            || strKey.equals("=")) /* Skip pairs with empty keys (e.g. "=FOO"). */
-        {
-            break;
-        }
-        Utf8Str strValue;
-        if (p < listPair.size()) /* Does the list also contain a value? */
-            strValue = listPair.at(p++);
-
-#ifdef DEBUG
-        LogFlowFunc(("strKey=%s, strValue=%s\n",
-                     strKey.c_str(), strValue.c_str()));
-#endif
-        rc = Set(strKey, strValue);
-    }
-
-    return rc;
-}
-
-size_t GuestEnvironment::Size(void)
-{
-    return mEnvironment.size();
-}
-
-int GuestEnvironment::Unset(const Utf8Str &strKey)
-{
-    std::map <Utf8Str, Utf8Str>::iterator itEnv = mEnvironment.find(strKey);
-    if (itEnv != mEnvironment.end())
-    {
-        mEnvironment.erase(itEnv);
-        return VINF_SUCCESS;
-    }
-
-    return VERR_NOT_FOUND;
-}
-
-GuestEnvironment& GuestEnvironment::operator=(const GuestEnvironmentArray &that)
-{
-    CopyFrom(that);
-    return *this;
-}
-
-GuestEnvironment& GuestEnvironment::operator=(const GuestEnvironment &that)
-{
-    for (std::map<Utf8Str, Utf8Str>::const_iterator it = that.mEnvironment.begin();
-         it != that.mEnvironment.end();
-         ++it)
-    {
-        mEnvironment[it->first] = it->second;
-    }
-
-    return *this;
-}
-
-/**
- * Appends environment variables to the environment block.
- *
- * Each var=value pair is separated by the null character ('\\0').  The whole
- * block will be stored in one blob and disassembled on the guest side later to
- * fit into the HGCM param structure.
- *
- * @returns VBox status code.
- *
- * @param   pszEnvVar       The environment variable=value to append to the
- *                          environment block.
- * @param   ppvList         This is actually a pointer to a char pointer
- *                          variable which keeps track of the environment block
- *                          that we're constructing.
- * @param   pcbList         Pointer to the variable holding the current size of
- *                          the environment block.  (List is a misnomer, go
- *                          ahead a be confused.)
- * @param   pcEnvVars       Pointer to the variable holding count of variables
- *                          stored in the environment block.
- */
-int GuestEnvironment::appendToEnvBlock(const char *pszEnv, void **ppvList, size_t *pcbList, uint32_t *pcEnvVars)
-{
-    int rc = VINF_SUCCESS;
-    size_t cchEnv = strlen(pszEnv); Assert(cchEnv >= 2);
-    if (*ppvList)
-    {
-        size_t cbNewLen = *pcbList + cchEnv + 1; /* Include zero termination. */
-        char *pvTmp = (char *)RTMemRealloc(*ppvList, cbNewLen);
-        if (pvTmp == NULL)
-            rc = VERR_NO_MEMORY;
-        else
-        {
-            memcpy(pvTmp + *pcbList, pszEnv, cchEnv);
-            pvTmp[cbNewLen - 1] = '\0'; /* Add zero termination. */
-            *ppvList = (void **)pvTmp;
-        }
-    }
-    else
-    {
-        char *pszTmp;
-        if (RTStrAPrintf(&pszTmp, "%s", pszEnv) >= 0)
-        {
-            *ppvList = (void **)pszTmp;
-            /* Reset counters. */
-            *pcEnvVars = 0;
-            *pcbList = 0;
-        }
-    }
-    if (RT_SUCCESS(rc))
-    {
-        *pcbList += cchEnv + 1; /* Include zero termination. */
-        *pcEnvVars += 1;        /* Increase env variable count. */
-    }
-    return rc;
-}
+
 
 int GuestFsObjData::FromLs(const GuestProcessStreamBlock &strmBlk)

trunk/src/VBox/Main/src-client/GuestProcessImpl.cpp

@@ -304 +304 @@
 
     AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-    mData.mProcess.mEnvironment.CopyTo(aEnvironment);
+    mData.mProcess.mEnvironment.queryPutEnvArray(&aEnvironment);
     return S_OK;
 #endif /* VBOX_WITH_GUEST_CONTROL */
@@ -1054 +1054 @@
 
     /* Prepare environment. */
-    void *pvEnv = NULL;
-    size_t cbEnv = 0;
+    size_t  cbEnvBlock;
+    char   *pszzEnvBlock;
     if (RT_SUCCESS(vrc))
-        vrc = mData.mProcess.mEnvironment.BuildEnvironmentBlock(&pvEnv, &cbEnv, NULL /* cEnv */);
+        vrc = mData.mProcess.mEnvironment.queryUtf8Block(&pszzEnvBlock, &cbEnvBlock);
 
     if (RT_SUCCESS(vrc))
@@ -1065 +1065 @@
         int i = 0;
         paParms[i++].setUInt32(pEvent->ContextID());
-        paParms[i++].setPointer((void*)mData.mProcess.mExecutable.c_str(),
-                                (ULONG)mData.mProcess.mExecutable.length() + 1);
+        paParms[i++].setCppString(mData.mProcess.mExecutable);
         paParms[i++].setUInt32(mData.mProcess.mFlags);
         paParms[i++].setUInt32((uint32_t)mData.mProcess.mArguments.size());
-        paParms[i++].setPointer((void*)pszArgs, (uint32_t)cbArgs);
-        paParms[i++].setUInt32((uint32_t)mData.mProcess.mEnvironment.Size());
-        paParms[i++].setUInt32((uint32_t)cbEnv);
-        paParms[i++].setPointer((void*)pvEnv, (uint32_t)cbEnv);
+        paParms[i++].setPointer(pszArgs, (uint32_t)cbArgs);
+        paParms[i++].setUInt32(mData.mProcess.mEnvironment.count());
+        paParms[i++].setUInt32((uint32_t)cbEnvBlock);
+        paParms[i++].setPointer(pszzEnvBlock, (uint32_t)cbEnvBlock);
         if (uProtocol < 2)
         {
@@ -1078 +1077 @@
              * call. In newer protocols these credentials are part of the opened guest
              * session, so not needed anymore here. */
-            paParms[i++].setPointer((void*)sessionCreds.mUser.c_str(), (ULONG)sessionCreds.mUser.length() + 1);
-            paParms[i++].setPointer((void*)sessionCreds.mPassword.c_str(), (ULONG)sessionCreds.mPassword.length() + 1);
+            paParms[i++].setCppString(sessionCreds.mUser);
+            paParms[i++].setCppString(sessionCreds.mPassword);
         }
         /*
@@ -1098 +1097 @@
             paParms[i++].setUInt32(1);
             /* The actual CPU affinity blocks. */
-            paParms[i++].setPointer((void*)&mData.mProcess.mAffinity, sizeof(mData.mProcess.mAffinity));
+            paParms[i++].setPointer((void *)&mData.mProcess.mAffinity, sizeof(mData.mProcess.mAffinity));
         }
 
@@ -1109 +1108 @@
             AssertRC(rc2);
         }
-    }
-
-    GuestEnvironment::FreeEnvironmentBlock(pvEnv);
+
+        mData.mProcess.mEnvironment.freeUtf8Block(pszzEnvBlock);
+    }
+
     if (pszArgs)
         RTStrFree(pszArgs);

trunk/src/VBox/Main/src-client/GuestSessionImpl.cpp

@@ -183 +183 @@
     AssertPtrReturn(pGuest, VERR_INVALID_POINTER);
 
+    /*
+     * Initialize our data members from the input.
+     */
     mParent = pGuest;
 
@@ -198 +201 @@
     mData.mCredentials.mDomain = guestCreds.mDomain;
 
+    /* Initialize the remainder of the data. */
     mData.mRC = VINF_SUCCESS;
     mData.mStatus = GuestSessionStatus_Undefined;
     mData.mNumObjects = 0;
-
-    HRESULT hr;
-
-    int rc = i_queryInfo();
+    int rc = mData.mEnvironment.initNormal();
     if (RT_SUCCESS(rc))
     {
-        hr = unconst(mEventSource).createObject();
-        if (FAILED(hr))
-            rc = VERR_NO_MEMORY;
-        else
-        {
+        rc = RTCritSectInit(&mWaitEventCritSect);
+        AssertRC(rc);
+    }
+    if (RT_SUCCESS(rc))
+        rc = i_determineProtocolVersion();
+    if (RT_SUCCESS(rc))
+    {
+        /*
+         * <Replace this if you figure out what the code is doing.>
+         */
+        HRESULT hr = unconst(mEventSource).createObject();
+        if (SUCCEEDED(hr))
             hr = mEventSource->init();
-            if (FAILED(hr))
-                rc = VERR_COM_UNEXPECTED;
-        }
-    }
-
-    if (RT_SUCCESS(rc))
-    {
-        try
-        {
-            GuestSessionListener *pListener = new GuestSessionListener();
-            ComObjPtr<GuestSessionListenerImpl> thisListener;
-            hr = thisListener.createObject();
-            if (SUCCEEDED(hr))
-                hr = thisListener->init(pListener, this);
-
-            if (SUCCEEDED(hr))
+        if (SUCCEEDED(hr))
+        {
+            try
             {
-                com::SafeArray <VBoxEventType_T> eventTypes;
-                eventTypes.push_back(VBoxEventType_OnGuestSessionStateChanged);
-                hr = mEventSource->RegisterListener(thisListener,
-                                                    ComSafeArrayAsInParam(eventTypes),
-                                                    TRUE /* Active listener */);
+                GuestSessionListener *pListener = new GuestSessionListener();
+                ComObjPtr<GuestSessionListenerImpl> thisListener;
+                hr = thisListener.createObject();
+                if (SUCCEEDED(hr))
+                    hr = thisListener->init(pListener, this);
                 if (SUCCEEDED(hr))
                 {
-                    mLocalListener = thisListener;
-
-                    rc = RTCritSectInit(&mWaitEventCritSect);
-                    AssertRC(rc);
+                    com::SafeArray <VBoxEventType_T> eventTypes;
+                    eventTypes.push_back(VBoxEventType_OnGuestSessionStateChanged);
+                    hr = mEventSource->RegisterListener(thisListener,
+                                                        ComSafeArrayAsInParam(eventTypes),
+                                                        TRUE /* Active listener */);
+                    if (SUCCEEDED(hr))
+                    {
+                        mLocalListener = thisListener;
+
+                        /*
+                         * Mark this object as operational and return success.
+                         */
+                        autoInitSpan.setSucceeded();
+                        LogFlowThisFunc(("mName=%s mID=%RU32 mIsInternal=%RTbool rc=VINF_SUCCESS\n",
+                                         mData.mSession.mName.c_str(), mData.mSession.mID, mData.mSession.mIsInternal));
+                        return VINF_SUCCESS;
+                    }
                 }
-                else
-                    rc = VERR_COM_UNEXPECTED;
             }
-            else
-                rc = VERR_COM_UNEXPECTED;
-        }
-        catch(std::bad_alloc &)
-        {
-            rc = VERR_NO_MEMORY;
-        }
-    }
-
-    if (RT_SUCCESS(rc))
-    {
-        /* Confirm a successful initialization when it's the case. */
-        autoInitSpan.setSucceeded();
-    }
-    else
-        autoInitSpan.setFailed();
-
-    LogFlowThisFunc(("mName=%s, mID=%RU32, mIsInternal=%RTbool, rc=%Rrc\n",
-                     mData.mSession.mName.c_str(), mData.mSession.mID, mData.mSession.mIsInternal, rc));
+            catch (std::bad_alloc &)
+            {
+                hr = E_OUTOFMEMORY;
+            }
+        }
+        rc = Global::vboxStatusCodeFromCOM(hr);
+    }
+
+    autoInitSpan.setFailed();
+    LogThisFunc(("Failed! mName=%s mID=%RU32 mIsInternal=%RTbool => rc=%Rrc\n",
+                 mData.mSession.mName.c_str(), mData.mSession.mID, mData.mSession.mIsInternal, rc));
     return rc;
 #endif /* VBOX_WITH_GUEST_CONTROL */
@@ -322 +320 @@
     mData.mProcesses.clear();
 
+    mData.mEnvironment.reset();
+
     AssertMsg(mData.mNumObjects == 0,
               ("mNumObjects=%RU32 when it should be 0\n", mData.mNumObjects));
@@ -471 +471 @@
     AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
 
-    size_t cEnvVars = mData.mEnvironment.Size();
-    aEnvironment.resize(cEnvVars);
-
-    LogFlowThisFunc(("[%s]: cEnvVars=%RU32\n",
-                     mData.mSession.mName.c_str(), cEnvVars));
-
-    for (size_t i = 0; i < cEnvVars; i++)
-        aEnvironment[i] = mData.mEnvironment.Get(i);
-
-    LogFlowThisFuncLeave();
-    return S_OK;
+    int vrc = mData.mEnvironment.queryPutEnvArray(&aEnvironment);
+
+    LogFlowFuncLeaveRC(vrc);
+    return Global::vboxStatusCodeToCOM(vrc);
 #endif /* VBOX_WITH_GUEST_CONTROL */
 }
@@ -494 +487 @@
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
-    int rc = VINF_SUCCESS;
-    for (size_t i = 0; i < aEnvironment.size() && RT_SUCCESS(rc); ++i)
-        if (!aEnvironment[i].isEmpty()) /* Silently skip empty entries. */
-            rc = mData.mEnvironment.Set(aEnvironment[i]);
-
-    HRESULT hr = RT_SUCCESS(rc) ? S_OK : VBOX_E_IPRT_ERROR;
-    LogFlowFuncLeaveRC(hr);
-    return hr;
+    mData.mEnvironment.reset();
+    int vrc = mData.mEnvironment.applyPutEnvArray(aEnvironment);
+
+    LogFlowFuncLeaveRC(vrc);
+    return Global::vboxStatusCodeToCOM(vrc);
 #endif /* VBOX_WITH_GUEST_CONTROL */
 }
@@ -1414 +1404 @@
 {
     return mData.mCredentials;
-}
-
-const GuestEnvironment& GuestSession::i_getEnvironment(void)
-{
-    return mData.mEnvironment;
 }
 
@@ -2118 +2103 @@
 
 /**
- * Queries/collects information prior to establishing a guest session.
- * This is necessary to know which guest control protocol version to use,
- * among other things (later).
+ * Determines the protocol version (sets mData.mProtocolVersion).
+ *
+ * This is called from the init method prior to to establishing a guest
+ * session.
  *
  * @return  IPRT status code.
  */
-int GuestSession::i_queryInfo(void)
+int GuestSession::i_determineProtocolVersion(void)
 {
     /*
-     * Try querying the guest control protocol version running on the guest.
-     * This is done using the Guest Additions version
+     * We currently do this based on the reported guest additions version,
+     * ASSUMING that VBoxService and VBoxDrv are at the same version.
      */
     ComObjPtr<Guest> pGuest = mParent;
-    Assert(!pGuest.isNull());
-
-    uint32_t uVerAdditions = pGuest->i_getAdditionsVersion();
-    uint32_t uVBoxMajor    = VBOX_FULL_VERSION_GET_MAJOR(uVerAdditions);
-    uint32_t uVBoxMinor    = VBOX_FULL_VERSION_GET_MINOR(uVerAdditions);
-
-#ifdef DEBUG_andy
-    /* Hardcode the to-used protocol version; nice for testing side effects. */
-    mData.mProtocolVersion = 2;
-#else
-    mData.mProtocolVersion = (
-                              /* VBox 5.0 and up. */
-                                 uVBoxMajor  >= 5
-                              /* VBox 4.3 and up. */
-                              || (uVBoxMajor == 4 && uVBoxMinor >= 3))
-                           ? 2  /* Guest control 2.0. */
-                           : 1; /* Legacy guest control (VBox < 4.3). */
-    /* Build revision is ignored. */
-#endif
-
-    LogFlowThisFunc(("uVerAdditions=%RU32 (%RU32.%RU32), mProtocolVersion=%RU32\n",
-                     uVerAdditions, uVBoxMajor, uVBoxMinor, mData.mProtocolVersion));
-
-    /* Tell the user but don't bitch too often. */
-    /** @todo Find a bit nicer text. */
+    AssertReturn(!pGuest.isNull(), VERR_NOT_SUPPORTED);
+    uint32_t uGaVersion = pGuest->i_getAdditionsVersion();
+
+    /* Everyone supports version one, if they support anything at all. */
+    mData.mProtocolVersion = 1;
+
+    /* Guest control 2.0 was introduced with 4.3.0. */
+    if (uGaVersion >= VBOX_FULL_VERSION_MAKE(4,3,0))
+        mData.mProtocolVersion = 2; /* Guest control 2.0. */
+
+    LogFlowThisFunc(("uGaVersion=%u.%u.%u => mProtocolVersion=%u\n",
+                     VBOX_FULL_VERSION_GET_MAJOR(uGaVersion), VBOX_FULL_VERSION_GET_MINOR(uGaVersion),
+                     VBOX_FULL_VERSION_GET_BUILD(uGaVersion), mData.mProtocolVersion));
+
+    /*
+     * Inform the user about outdated guest additions (VM release log).
+     */
     if (mData.mProtocolVersion < 2)
-        LogRelMax(3, (tr("Warning: Guest Additions are older (%ld.%ld) than host capabilities for guest control, please upgrade them. Using protocol version %ld now\n"),
-                    uVBoxMajor, uVBoxMinor, mData.mProtocolVersion));
+        LogRelMax(3, (tr("Warning: Guest Additions v%u.%u.%u only supports the older guest control protocol version %u.\n"
+                         "         Please upgrade GAs to the current version to get full guest control capabilities.\n"),
+                      VBOX_FULL_VERSION_GET_MAJOR(uGaVersion), VBOX_FULL_VERSION_GET_MINOR(uGaVersion),
+                      VBOX_FULL_VERSION_GET_BUILD(uGaVersion), mData.mProtocolVersion));
 
     return VINF_SUCCESS;
@@ -2924 +2904 @@
 }
 
+/** @todo remove this it duplicates the 'environment' attribute.   */
 HRESULT GuestSession::environmentClear()
 {
@@ -2933 +2914 @@
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
-    mData.mEnvironment.Clear();
+    mData.mEnvironment.reset();
 
     LogFlowThisFuncLeave();
@@ -2940 +2921 @@
 }
 
+/** @todo Remove this because the interface isn't suitable for returning 'unset'
+ *        or empty values, and it can easily be misunderstood.  Besides there
+ *        is hardly a usecase for it as long as it just works on
+ *        environment changes and there is the 'environment' attribute. */
 HRESULT GuestSession::environmentGet(const com::Utf8Str &aName, com::Utf8Str &aValue)
 {
@@ -2947 +2932 @@
     LogFlowThisFuncEnter();
 
-    if (RT_UNLIKELY(aName.c_str() == NULL) || *(aName.c_str()) == '\0')
-        return setError(E_INVALIDARG, tr("No value name specified"));
-
-    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-
-    aValue =  mData.mEnvironment.Get(aName);
+    HRESULT hrc;
+    if (RT_LIKELY(aName.isNotEmpty()))
+    {
+        if (RT_LIKELY(strchr(aName.c_str(), '=') == NULL))
+        {
+            AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+            mData.mEnvironment.getVariable(aName, &aValue);
+            hrc = S_OK;
+        }
+        else
+            hrc = setError(E_INVALIDARG, tr("The equal char is not allowed in environment variable names"));
+    }
+    else
+        hrc = setError(E_INVALIDARG, tr("No variable name specified"));
 
     LogFlowThisFuncLeave();
-    return S_OK;
+    return hrc;
 #endif /* VBOX_WITH_GUEST_CONTROL */
 }
@@ -2966 +2960 @@
     LogFlowThisFuncEnter();
 
-    if (RT_UNLIKELY((aName.c_str() == NULL) || *(aName.c_str()) == '\0'))
-        return setError(E_INVALIDARG, tr("No value name specified"));
-
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-
-    int rc = mData.mEnvironment.Set(aName, aValue);
-
-    HRESULT hr = RT_SUCCESS(rc) ? S_OK : VBOX_E_IPRT_ERROR;
-    LogFlowFuncLeaveRC(hr);
-    return hr;
+    HRESULT hrc;
+    if (RT_LIKELY(aName.isNotEmpty()))
+    {
+        if (RT_LIKELY(strchr(aName.c_str(), '=') == NULL))
+        {
+            AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+            int vrc = mData.mEnvironment.setVariable(aName, aValue);
+            if (RT_SUCCESS(vrc))
+                hrc = S_OK;
+            else
+                hrc = setErrorVrc(vrc);
+        }
+        else
+            hrc = setError(E_INVALIDARG, tr("The equal char is not allowed in environment variable names"));
+    }
+    else
+        hrc = setError(E_INVALIDARG, tr("No variable name specified"));
+
+    LogFlowThisFuncLeave();
+    return hrc;
 #endif /* VBOX_WITH_GUEST_CONTROL */
 }
@@ -2985 +2989 @@
 #else
     LogFlowThisFuncEnter();
-
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-
-    mData.mEnvironment.Unset(aName);
+    HRESULT hrc;
+    if (RT_LIKELY(aName.isNotEmpty()))
+    {
+        if (RT_LIKELY(strchr(aName.c_str(), '=') == NULL))
+        {
+            AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+            int vrc = mData.mEnvironment.unsetVariable(aName);
+            if (RT_SUCCESS(vrc))
+                hrc = S_OK;
+            else
+                hrc = setErrorVrc(vrc);
+        }
+        else
+            hrc = setError(E_INVALIDARG, tr("The equal char is not allowed in environment variable names"));
+    }
+    else
+        hrc = setError(E_INVALIDARG, tr("No variable name specified"));
 
     LogFlowThisFuncLeave();
-    return S_OK;
+    return hrc;
 #endif /* VBOX_WITH_GUEST_CONTROL */
 }
@@ -3338 +3355 @@
     LogFlowThisFuncEnter();
 
+    /** @todo r=bird: Check input better? aPriority is passed on to the guest
+     * without any validation.  Flags not existing in this vbox version are
+     * ignored, potentially doing something entirely different than what the
+     * caller had in mind. */
+
     /*
      * Must have an executable to execute.  If none is given, we try use the
@@ -3369 +3391 @@
             procInfo.mArguments.push_back(aArguments[i]);
 
-    /*
-     * Create the process environment:
-     * - Apply the session environment in a first step, and
-     * - Apply environment variables specified by this call to
-     *   have the chance of overwriting/deleting session entries.
-     */
-    procInfo.mEnvironment = mData.mEnvironment; /* Apply original session environment. */
-
-    int rc = VINF_SUCCESS;
-    if (aEnvironment.size())
-        for (size_t i = 0; i < aEnvironment.size() && RT_SUCCESS(rc); i++)
-        {
-            /** @todo r=bird: What ARE you trying to do here??? The documentation is crystal
-             *        clear on that each entry contains ONE pair, however,
-             *        GuestEnvironment::Set(const Utf8Str &) here will split up the input
-             *        into any number of pairs, from what I can tell.  Such that for e.g.
-             *        "VBOX_LOG_DEST=file=/tmp/foobared.log" becomes "VBOX_LOG_DEST=file"
-             *        and "/tmp/foobared.log" - which I obviously don't want! */
-            rc = procInfo.mEnvironment.Set(aEnvironment[i]);
-        }
-
-    if (RT_SUCCESS(rc))
+    /* Combine the environment changes associated with the ones passed in by
+       the caller, giving priority to the latter.  The changes are putenv style
+       and will be applied to the standard environment for the guest user. */
+    int vrc = procInfo.mEnvironment.copy(mData.mEnvironment);
+    if (RT_SUCCESS(vrc))
+        vrc = procInfo.mEnvironment.applyPutEnvArray(aEnvironment);
+    if (RT_SUCCESS(vrc))
     {
         /* Convert the flag array into a mask. */
@@ -3411 +3418 @@
          */
         ComObjPtr<GuestProcess> pProcess;
-        rc = i_processCreateExInternal(procInfo, pProcess);
-        if (RT_SUCCESS(rc))
+        vrc = i_processCreateExInternal(procInfo, pProcess);
+        if (RT_SUCCESS(vrc))
         {
             /* Return guest session to the caller. */
-            HRESULT hr2 = pProcess.queryInterfaceTo(aGuestProcess.asOutParam());
-            if (SUCCEEDED(hr2))
+            hr = pProcess.queryInterfaceTo(aGuestProcess.asOutParam());
+            if (SUCCEEDED(hr))
             {
                 /*
                  * Start the process.
                  */
-                rc = pProcess->i_startProcessAsync();
-                if (RT_FAILURE(rc))
+                vrc = pProcess->i_startProcessAsync();
+                if (RT_SUCCESS(vrc))
                 {
-                    /** @todo r=bird: What happens to the interface that *aGuestProcess points to
-                     *        now?  Looks like a leak or an undocument hack of sorts... */
+                    LogFlowFuncLeaveRC(vrc);
+                    return S_OK;
                 }
+
+                hr = setErrorVrc(vrc, tr("Failed to start guest process: %Rrc"), vrc);
+                /** @todo r=bird: What happens to the interface that *aGuestProcess points to
+                 *        now?  Looks like a leak or an undocument hack of sorts... */
             }
-            else
-                rc = VERR_COM_OBJECT_NOT_FOUND;
-        }
-    }
-
-    /** @todo you're better off doing this is 'else if (rc == xxx') statements,
-     *        since there is just one place where you'll get
-     *        VERR_MAX_PROCS_REACHED in the above code. */
-    if (RT_FAILURE(rc))
-    {
-        switch (rc)
-        {
-            case VERR_MAX_PROCS_REACHED:
-                hr = setError(VBOX_E_IPRT_ERROR, tr("Maximum number of concurrent guest processes per session (%ld) reached"),
-                                                    VBOX_GUESTCTRL_MAX_OBJECTS);
-                break;
-
-            /** @todo Add more errors here. */
-
-            default:
-                hr = setError(VBOX_E_IPRT_ERROR, tr("Could not create guest process, rc=%Rrc"), rc);
-                break;
-        }
-    }
-
-    LogFlowFuncLeaveRC(rc);
+        }
+        else if (vrc == VERR_MAX_PROCS_REACHED)
+            hr = setErrorVrc(vrc, tr("Maximum number of concurrent guest processes per session (%u) reached"),
+                             VBOX_GUESTCTRL_MAX_OBJECTS);
+        else
+            hr = setErrorVrc(vrc, tr("Failed to create guest process object: %Rrc"), vrc);
+    }
+    else
+        hr = setErrorVrc(vrc, tr("Failed to set up the environment: %Rrc"), vrc);
+
+    LogFlowFuncLeaveRC(vrc);
     return hr;
 #endif /* VBOX_WITH_GUEST_CONTROL */