Changeset 75663 in vbox

Timestamp:

Nov 22, 2018 2:00:59 PM (6 years ago)

Author:

vboxsync

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

126855

Message:

Main/ExtPackManager+CloudProviderManager+VirtualBox: Handle runtime install and uninstall of extension packs containing cloud providers. This is a safeguard against uninstalling active cloud providers (which would cause crashes), and makes the install more convcenient. At the same time it eliminated the need to have a special "detect all cloud providers" logic when VBoxSVC is started. It achieves perfect singleton property of the per-extpack CloudProviderManager and the related CloudProvider objects. Nothing is ever created twice (which would cause trouble with coordinating e.g. the profile file updates).

Location:

trunk/src/VBox/Main

Files:

• idl/VirtualBox.xidl (modified) (2 diffs)
• include/CloudProviderManagerImpl.h (modified) (3 diffs)
• include/ExtPackManagerImpl.h (modified) (1 diff)
• include/VirtualBoxImpl.h (modified) (3 diffs)
• src-all/ExtPackManagerImpl.cpp (modified) (7 diffs)
• src-server/CloudProviderManagerImpl.cpp (modified) (4 diffs)
• src-server/VirtualBoxImpl.cpp (modified) (2 diffs)

trunk/src/VBox/Main/idl/VirtualBox.xidl

@@ -25520 +25520 @@
   <interface
     name="ICloudProvider" extends="$unknown"
-    uuid="8154fa84-35d7-4a9b-86b4-cb2635b0d9f5"
+    uuid="caaf8bc1-aab0-4ea0-b825-be53a1453bdd"
     wsmap="managed" reservedMethods="4" reservedAttributes="4"
     >
@@ -25604 +25604 @@
       <param name="profileName" type="wstring" dir="in"/>
       <param name="profile" type="ICloudProfile" dir="return"/>
+    </method>
+
+    <method name="prepareUninstall" wrap-hint-server="passcaller">
+      <desc>
+        The caller requests the cloud provider to cease operation. Should
+        return an error if this is currently not possible (due to ongoing
+        cloud activity, possibly by a different API client). However, this
+        must not wait for the completion for a larger amount of time (ideally
+        stays below a second of execution time). If this succeeds it should
+        leave the cloud provider in a state which does not allow starting new
+        operations.
+      </desc>
     </method>
 

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

@@ -21 +21 @@
 
 #include "CloudProviderManagerWrap.h"
-#ifdef VBOX_WITH_EXTPACK
-class ExtPackManager;
-#endif
 
 
@@ -35 +32 @@
     void FinalRelease();
 
-    HRESULT init(VirtualBox *aParent);
+    HRESULT init();
     void uninit();
 
 #ifdef VBOX_WITH_EXTPACK
     // Safe helpers, take care of caller and lock themselves.
-    void i_refreshProviders();
+    bool i_canRemoveExtPack(IExtPack *aExtPack);
+    void i_addExtPack(IExtPack *aExtPack);
 #endif
 
@@ -55 +53 @@
 private:
 #ifdef VBOX_WITH_EXTPACK
-    ComObjPtr<ExtPackManager> mpExtPackMgr;
-    uint64_t mcExtPackMgrUpdate;
-    std::map<com::Utf8Str, ComPtr<ICloudProviderManager> > m_mapCloudProviderManagers;
+    typedef std::map<com::Utf8Str, ComPtr<ICloudProviderManager> > ExtPackNameCloudProviderManagerMap;
+    ExtPackNameCloudProviderManagerMap m_mapCloudProviderManagers;
+    typedef std::vector<com::Utf8Str> ExtPackNameVec;
+    ExtPackNameVec m_astrExtPackNames;
 #endif
 
-    std::vector<ComPtr<ICloudProvider> > m_apCloudProviders;
+    typedef std::vector<ComPtr<ICloudProvider> > CloudProviderVec;
+    CloudProviderVec m_apCloudProviders;
 };
 

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

@@ -131 +131 @@
     bool        i_wantsToBeDefaultVrde(void) const;
     HRESULT     i_refresh(bool *pfCanDelete);
+#ifndef VBOX_COM_INPROC
+    bool        i_areThereCloudProviderUninstallVetos();
+    void        i_notifyCloudProviderManager();
+#endif
     /** @}  */
 

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

@@ -40 +40 @@
 class DHCPServer;
 class PerformanceCollector;
+class CloudProviderManager;
 #ifdef VBOX_WITH_EXTPACK
 class ExtPackManager;
@@ -216 +217 @@
     const Guid &i_getGlobalRegistryId() const;
 
-    const ComObjPtr<Host>& i_host() const;
-    SystemProperties* i_getSystemProperties() const;
+    const ComObjPtr<Host> &i_host() const;
+    SystemProperties *i_getSystemProperties() const;
+    CloudProviderManager *i_getCloudProviderManager() const;
 #ifdef VBOX_WITH_EXTPACK
-    ExtPackManager* i_getExtPackManager() const;
+    ExtPackManager *i_getExtPackManager() const;
 #endif
 #ifdef VBOX_WITH_RESOURCE_USAGE_API
-    const ComObjPtr<PerformanceCollector>& i_performanceCollector() const;
+    const ComObjPtr<PerformanceCollector> &i_performanceCollector() const;
 #endif /* VBOX_WITH_RESOURCE_USAGE_API */
 
@@ -229 +231 @@
 
     /** Returns the VirtualBox home directory */
-    const Utf8Str& i_homeDir() const;
+    const Utf8Str &i_homeDir() const;
     int i_calculateFullPath(const Utf8Str &strPath, Utf8Str &aResult);
     void i_copyPathRelativeToConfig(const Utf8Str &strSource, Utf8Str &strTarget);

trunk/src/VBox/Main/src-all/ExtPackManagerImpl.cpp

@@ -21 +21 @@
 *********************************************************************************************************************************/
 #include "ExtPackManagerImpl.h"
+#include "CloudProviderManagerImpl.h"
 #include "ExtPackUtil.h"
 #include "ThreadTask.h"
@@ -928 +929 @@
             a_pLock->release();
             m->pReg->pfnVirtualBoxReady(m->pReg, a_pVirtualBox);
+            i_notifyCloudProviderManager();
             a_pLock->acquire();
             return true;
@@ -1085 +1087 @@
     return false;
 }
-#endif /* !VBOX_COM_INPROC */
+#endif /* VBOX_COM_INPROC */
 
 /**
@@ -1248 +1250 @@
     return S_OK;
 }
+
+#ifndef VBOX_COM_INPROC
+/**
+ * Checks if there are cloud providers vetoing extension pack uninstall.
+ *
+ * This needs going through the cloud provider singleton in VirtualBox,
+ * the job cannot be done purely by using the code in the extension pack).
+ * It cannot be integrated into i_callUninstallHookAndClose, because it
+ * can only do its job when the extpack lock is not held, whereas the
+ * actual uninstall must be done with the lock held all the time for
+ * consistency reasons.
+ *
+ * This is called when uninstalling or replacing an extension pack.
+ *
+ * @returns true / false
+ */
+bool ExtPack::i_areThereCloudProviderUninstallVetos()
+{
+    Assert(m->pVirtualBox != NULL); /* Only called from VBoxSVC. */
+
+    ComObjPtr<CloudProviderManager> cpm(m->pVirtualBox->i_getCloudProviderManager());
+    AssertReturn(!cpm.isNull(), false);
+
+    return !cpm->i_canRemoveExtPack(static_cast<IExtPack *>(this));
+}
+
+/**
+ * Notifies the Cloud Provider Manager that there is a new extension pack.
+ *
+ * This is called when installing an extension pack.
+ *
+ * @param   a_pExtPack          The extension pack to be added.
+ */
+void ExtPack::i_notifyCloudProviderManager()
+{
+    Assert(m->pVirtualBox != NULL); /* Only called from VBoxSVC. */
+
+    ComObjPtr<CloudProviderManager> cpm(m->pVirtualBox->i_getCloudProviderManager());
+    AssertReturnVoid(!cpm.isNull());
+
+    cpm->i_addExtPack(static_cast<IExtPack *>(this));
+}
+
+#endif /* !VBOX_COM_INPROC */
 
 /**
@@ -2849 +2895 @@
                 autoLock.release();
                 bool fRunningVMs = i_areThereAnyRunningVMs();
+                bool fVetoingCP = pExtPack->i_areThereCloudProviderUninstallVetos();
                 autoLock.acquire();
                 hrc = i_refreshExtPack(pStrName->c_str(), false /*a_fUnusableIsError*/, &pExtPack);
                 if (fRunningVMs)
                 {
-                    LogRel(("Install extension pack '%s' failed because at least one VM is still running.", pStrName->c_str()));
-                    hrc = setError(E_FAIL, tr("Install extension pack '%s' failed because at least one VM is still running"),
+                    LogRel(("Upgrading extension pack '%s' failed because at least one VM is still running.", pStrName->c_str()));
+                    hrc = setError(E_FAIL, tr("Upgrading extension pack '%s' failed because at least one VM is still running"),
+                                   pStrName->c_str());
+                }
+                else if (fVetoingCP)
+                {
+                    LogRel(("Upgrading extension pack '%s' failed because at least one Cloud Provider is still busy.", pStrName->c_str()));
+                    hrc = setError(E_FAIL, tr("Upgrading extension pack '%s' failed because at least one Cloud Provider is still busy"),
                                    pStrName->c_str());
                 }
@@ -2956 +3009 @@
     if (SUCCEEDED(hrc))
     {
-        if (a_fForcedRemoval || !i_areThereAnyRunningVMs())
-        {
-            AutoWriteLock autoLock(this COMMA_LOCKVAL_SRC_POS);
-
-            /*
-             * Refresh the data we have on the extension pack as it may be made
-             * stale by direct meddling or some other user.
-             */
-            ExtPack *pExtPack;
-            hrc = i_refreshExtPack(a_pstrName->c_str(), false /*a_fUnusableIsError*/, &pExtPack);
-            if (SUCCEEDED(hrc))
+        AutoWriteLock autoLock(this COMMA_LOCKVAL_SRC_POS);
+
+        /*
+         * Refresh the data we have on the extension pack as it
+         * may be made stale by direct meddling or some other user.
+         */
+        ExtPack *pExtPack;
+        hrc = i_refreshExtPack(a_pstrName->c_str(), false /*a_fUnusableIsError*/, &pExtPack);
+        if (SUCCEEDED(hrc) && pExtPack)
+        {
+            /* We must leave the lock when calling i_areThereAnyRunningVMs,
+               which means we have to redo the refresh call afterwards. */
+            autoLock.release();
+            bool fRunningVMs = i_areThereAnyRunningVMs();
+            bool fVetoingCP = pExtPack->i_areThereCloudProviderUninstallVetos();
+            autoLock.acquire();
+            if (a_fForcedRemoval || (!fRunningVMs && !fVetoingCP))
             {
-                if (!pExtPack)
+                hrc = i_refreshExtPack(a_pstrName->c_str(), false /*a_fUnusableIsError*/, &pExtPack);
+                if (SUCCEEDED(hrc))
                 {
-                    LogRel(("ExtPackManager: Extension pack '%s' is not installed, so nothing to uninstall.\n", a_pstrName->c_str()));
-                    hrc = S_OK;             /* nothing to uninstall */
-                }
-                else
-                {
-                    /*
-                     * Call the uninstall hook and unload the main dll.
-                     */
-                    hrc = pExtPack->i_callUninstallHookAndClose(m->pVirtualBox, a_fForcedRemoval);
-                    if (SUCCEEDED(hrc))
+                    if (!pExtPack)
+                    {
+                        LogRel(("ExtPackManager: Extension pack '%s' is not installed, so nothing to uninstall.\n", a_pstrName->c_str()));
+                        hrc = S_OK;             /* nothing to uninstall */
+                    }
+                    else
                     {
                         /*
-                         * Run the set-uid-to-root binary that performs the
-                         * uninstallation.  Then refresh the object.
-                         *
-                         * This refresh is theorically subject to races, but it's of
-                         * the don't-do-that variety.
+                         * Call the uninstall hook and unload the main dll.
                          */
-                        const char *pszForcedOpt = a_fForcedRemoval ? "--forced" : NULL;
-                        hrc = i_runSetUidToRootHelper(a_pstrDisplayInfo,
-                                                      "uninstall",
-                                                      "--base-dir", m->strBaseDir.c_str(),
-                                                      "--name",     a_pstrName->c_str(),
-                                                      pszForcedOpt, /* Last as it may be NULL. */
-                                                      (const char *)NULL);
+                        hrc = pExtPack->i_callUninstallHookAndClose(m->pVirtualBox, a_fForcedRemoval);
                         if (SUCCEEDED(hrc))
                         {
-                            hrc = i_refreshExtPack(a_pstrName->c_str(), false /*a_fUnusableIsError*/, &pExtPack);
+                            /*
+                             * Run the set-uid-to-root binary that performs the
+                             * uninstallation.  Then refresh the object.
+                             *
+                             * This refresh is theorically subject to races, but it's of
+                             * the don't-do-that variety.
+                             */
+                            const char *pszForcedOpt = a_fForcedRemoval ? "--forced" : NULL;
+                            hrc = i_runSetUidToRootHelper(a_pstrDisplayInfo,
+                                                          "uninstall",
+                                                          "--base-dir", m->strBaseDir.c_str(),
+                                                          "--name",     a_pstrName->c_str(),
+                                                          pszForcedOpt, /* Last as it may be NULL. */
+                                                          (const char *)NULL);
                             if (SUCCEEDED(hrc))
                             {
-                                if (!pExtPack)
-                                    LogRel(("ExtPackManager: Successfully uninstalled extension pack '%s'.\n", a_pstrName->c_str()));
-                                else
-                                    hrc = setError(E_FAIL,
-                                                   tr("Uninstall extension pack '%s' failed under mysterious circumstances"),
-                                                   a_pstrName->c_str());
+                                hrc = i_refreshExtPack(a_pstrName->c_str(), false /*a_fUnusableIsError*/, &pExtPack);
+                                if (SUCCEEDED(hrc))
+                                {
+                                    if (!pExtPack)
+                                        LogRel(("ExtPackManager: Successfully uninstalled extension pack '%s'.\n", a_pstrName->c_str()));
+                                    else
+                                        hrc = setError(E_FAIL,
+                                                       tr("Uninstall extension pack '%s' failed under mysterious circumstances"),
+                                                       a_pstrName->c_str());
+                                }
                             }
-                        }
-                        else
-                        {
-                            ErrorInfoKeeper Eik;
-                            i_refreshExtPack(a_pstrName->c_str(), false /*a_fUnusableIsError*/, NULL);
+                            else
+                            {
+                                ErrorInfoKeeper Eik;
+                                i_refreshExtPack(a_pstrName->c_str(), false /*a_fUnusableIsError*/, NULL);
+                            }
                         }
                     }
                 }
             }
-        }
-        else
-        {
-            LogRel(("Uninstall extension pack '%s' failed because at least one VM is still running.", a_pstrName->c_str()));
-            hrc = setError(E_FAIL, tr("Uninstall extension pack '%s' failed because at least one VM is still running"),
-                           a_pstrName->c_str());
+            else
+            {
+                if (fRunningVMs)
+                {
+                    LogRel(("Uninstall extension pack '%s' failed because at least one VM is still running.", a_pstrName->c_str()));
+                    hrc = setError(E_FAIL, tr("Uninstall extension pack '%s' failed because at least one VM is still running"),
+                                   a_pstrName->c_str());
+                }
+                else if (fVetoingCP)
+                {
+                    LogRel(("Uninstall extension pack '%s' failed because at least one Cloud Provider is still busy.", a_pstrName->c_str()));
+                    hrc = setError(E_FAIL, tr("Uninstall extension pack '%s' failed because at least one Cloud Provider is still busy"),
+                                   a_pstrName->c_str());
+                }
+                else
+                {
+                    LogRel(("Uninstall extension pack '%s' failed for an unknown reason.", a_pstrName->c_str()));
+                    hrc = setError(E_FAIL, tr("Uninstall extension pack '%s' failed for an unknown reason"),
+                                   a_pstrName->c_str());
+
+                }
+            }
         }
 
@@ -3029 +3108 @@
          */
         if (m->enmContext == VBOXEXTPACKCTX_PER_USER_DAEMON)
+        {
+            autoLock.release();
             i_callAllVirtualBoxReadyHooks();
+        }
     }
 

trunk/src/VBox/Main/src-server/CloudProviderManagerImpl.cpp

@@ -25 +25 @@
 #include "Logging.h"
 
-using namespace std;
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -53 +52 @@
 }
 
-HRESULT CloudProviderManager::init(VirtualBox *aParent)
+HRESULT CloudProviderManager::init()
 {
     // Enclose the state transition NotReady->InInit->Ready.
@@ -61 +60 @@
     m_apCloudProviders.clear();
 
-#ifdef VBOX_WITH_EXTPACK
-    // Engage the extension pack manager and get all the implementations of
-    // this class and all implemented cloud providers.
-    ExtPackManager *pExtPackMgr = aParent->i_getExtPackManager();
-    mpExtPackMgr = pExtPackMgr;
-    if (pExtPackMgr)
-    {
-        mcExtPackMgrUpdate = pExtPackMgr->i_getUpdateCounter();
-        // Make sure that the current value doesn't match, forcing a refresh.
-        mcExtPackMgrUpdate--;
-        i_refreshProviders();
-    }
-#else
-    RT_NOREF(aParent);
-#endif
-
     autoInitSpan.setSucceeded();
     return S_OK;
@@ -90 +73 @@
 
 #ifdef VBOX_WITH_EXTPACK
-void CloudProviderManager::i_refreshProviders()
-{
-    uint64_t cExtPackMgrUpdate;
-    {
-        AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-        if (mpExtPackMgr.isNull())
-            return;
-        cExtPackMgrUpdate = mpExtPackMgr->i_getUpdateCounter();
-        if (cExtPackMgrUpdate == mcExtPackMgrUpdate)
-            return;
-    }
+bool CloudProviderManager::i_canRemoveExtPack(IExtPack *aExtPack)
+{
+    AssertReturn(aExtPack, false);
 
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-    // Reread the current value to figure out if some other thead did the work
-    // already before this thread got hold of the write lock.
-    cExtPackMgrUpdate = mpExtPackMgr->i_getUpdateCounter();
-    if (cExtPackMgrUpdate == mcExtPackMgrUpdate)
-        return;
-    mcExtPackMgrUpdate = cExtPackMgrUpdate;
-
-    if (!m_mapCloudProviderManagers.empty())
-    {
-        /// @todo The code below will need to be extended to handle extpack
-        // install and uninstall safely (and the latter needs non-trivial
-        // checks if any extpack related cloud activity is pending.
-        return;
-    }
-
-    std::vector<ComPtr<IUnknown> > apObjects;
+
+    // If any cloud provider in this extension pack fails to prepare the
+    // uninstall it and the cloud provider will be kept, so that the user
+    // can retry safely later. All other cloud providers in this extpack
+    // will be done as usual. No attempt is made to bring back the other
+    // cloud providers into working shape.
+
+    bool fRes = true;
+    Bstr bstrName;
+    aExtPack->GetName(bstrName.asOutParam());
+    Utf8Str strName(bstrName);
+    ExtPackNameCloudProviderManagerMap::iterator it = m_mapCloudProviderManagers.find(strName);
+    if (it != m_mapCloudProviderManagers.end())
+    {
+        ComPtr<ICloudProviderManager> pTmp(it->second);
+
+        Assert(m_astrExtPackNames.size() == m_apCloudProviders.size());
+        for (size_t i = 0; i < m_astrExtPackNames.size(); )
+        {
+            if (m_astrExtPackNames[i] != strName)
+            {
+                i++;
+                continue;
+            }
+
+            // pTmpProvider will point to an object with refcount > 0 until
+            // the ComPtr is removed from m_apCloudProviders.
+            HRESULT hrc = S_OK;
+            ULONG uRefCnt = 1;
+            ICloudProvider *pTmpProvider(m_apCloudProviders[i]);
+            if (pTmpProvider)
+            {
+                hrc = pTmpProvider->PrepareUninstall();
+                // Sanity check the refcount, it should be 1 at this point.
+                pTmpProvider->AddRef();
+                uRefCnt = pTmpProvider->Release();
+                Assert(uRefCnt == 1);
+            }
+            if (SUCCEEDED(hrc) && uRefCnt == 1)
+            {
+                m_astrExtPackNames.erase(m_astrExtPackNames.begin() + i);
+                m_apCloudProviders.erase(m_apCloudProviders.begin() + i);
+            }
+            else
+            {
+                LogRel(("CloudProviderManager: provider '%s' blocks extpack uninstall, result=%Rhrc, refcount=%u\n", strName.c_str(), hrc, uRefCnt));
+                fRes = false;
+                i++;
+            }
+        }
+
+        if (fRes)
+            m_mapCloudProviderManagers.erase(it);
+    }
+
+    return fRes;
+}
+
+void CloudProviderManager::i_addExtPack(IExtPack *aExtPack)
+{
+    AssertReturnVoid(aExtPack);
+
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+    Bstr bstrName;
+    aExtPack->GetName(bstrName.asOutParam());
+    Utf8Str strName(bstrName);
+    ComPtr<IUnknown> pObj;
     std::vector<com::Utf8Str> astrExtPackNames;
     com::Guid idObj(COM_IIDOF(ICloudProviderManager));
-    mpExtPackMgr->i_queryObjects(idObj.toString(), apObjects, &astrExtPackNames);
-    for (unsigned i = 0; i < apObjects.size(); i++)
-    {
-        ComPtr<ICloudProviderManager> pTmp;
-        HRESULT hrc = apObjects[i].queryInterfaceTo(pTmp.asOutParam());
-        if (SUCCEEDED(hrc))
-            m_mapCloudProviderManagers[astrExtPackNames[i]] = pTmp;
-        SafeIfaceArray<ICloudProvider> apProvidersFromCurrExtPack;
-        hrc = pTmp->COMGETTER(Providers)(ComSafeArrayAsOutParam(apProvidersFromCurrExtPack));
-        if (SUCCEEDED(hrc))
-        {
-            for (unsigned j = 0; j < apProvidersFromCurrExtPack.size(); j++)
-                m_apCloudProviders.push_back(apProvidersFromCurrExtPack[i]);
-        }
+    HRESULT hrc = aExtPack->QueryObject(Bstr(idObj.toString()).raw(), pObj.asOutParam());
+    if (FAILED(hrc))
+        return;
+
+    ComPtr<ICloudProviderManager> pTmp(pObj);
+    if (pTmp.isNull())
+        return;
+
+    SafeIfaceArray<ICloudProvider> apProvidersFromCurrExtPack;
+    hrc = pTmp->COMGETTER(Providers)(ComSafeArrayAsOutParam(apProvidersFromCurrExtPack));
+    if (FAILED(hrc))
+        return;
+
+    m_mapCloudProviderManagers[strName] = pTmp;
+    for (unsigned i = 0; i < apProvidersFromCurrExtPack.size(); i++)
+    {
+        Assert(m_astrExtPackNames.size() == m_apCloudProviders.size());
+        m_astrExtPackNames.push_back(strName);
+        m_apCloudProviders.push_back(apProvidersFromCurrExtPack[i]);
     }
 }

trunk/src/VBox/Main/src-server/VirtualBoxImpl.cpp

@@ -555 +555 @@
         rc = unconst(m->pCloudProviderManager).createObject();
         if (SUCCEEDED(rc))
-            rc = m->pCloudProviderManager->init(this);
+            rc = m->pCloudProviderManager->init();
         ComAssertComRCThrowRC(rc);
         if (FAILED(rc)) throw rc;
@@ -3768 +3768 @@
 }
 
+CloudProviderManager *VirtualBox::i_getCloudProviderManager() const
+{
+    return m->pCloudProviderManager;
+}
+
 #ifdef VBOX_WITH_EXTPACK
 /**