VirtualBox

Changeset 61137 in vbox


Ignore:
Timestamp:
May 23, 2016 3:55:49 PM (9 years ago)
Author:
vboxsync
Message:

Main/Medium: some more lock order fixing, but a lot of work is left

Location:
trunk/src/VBox/Main
Files:
3 edited

Legend:

Unmodified
Added
Removed

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

    r61042 r61137  
    1458314583    </attribute>
    1458414584
    14585     <attribute name="type" type="MediumType">
     14585    <attribute name="type" type="MediumType" wrap-hint-server="passcaller">
    1458614586      <desc>
    1458714587        Type (role) of this medium.
    … …  
    1464714647    </attribute>
    1464814648
    14649     <attribute name="readOnly" type="boolean" readonly="yes">
     14649    <attribute name="readOnly" type="boolean" readonly="yes" wrap-hint-server="passcaller">
    1465014650      <desc>
    1465114651        Returns @c true if this medium is read-only and @c false otherwise.
    … …  
    1520115201    <!-- diff methods -->
    1520215202
    15203     <method name="createDiffStorage">
     15203    <method name="createDiffStorage" wrap-hint-server="passcaller">
    1520415204      <desc>
    1520515205        Starts creating an empty differencing storage unit based on this
    … …  
    1550915509    </method>
    1551015510
    15511     <method name="reset">
     15511    <method name="reset" wrap-hint-server="passcaller">
    1551215512      <desc>
    1551315513        Starts erasing the contents of this differencing medium.

  • trunk/src/VBox/Main/include/MediumImpl.h

    r60627 r61137  
    237237    HRESULT getFormat(com::Utf8Str &aFormat);
    238238    HRESULT getMediumFormat(ComPtr<IMediumFormat> &aMediumFormat);
    239     HRESULT getType(MediumType_T *aType);
    240     HRESULT setType(MediumType_T aType);
     239    HRESULT getType(AutoCaller &autoCaller, MediumType_T *aType);
     240    HRESULT setType(AutoCaller &autoCaller, MediumType_T aType);
    241241    HRESULT getAllowedTypes(std::vector<MediumType_T> &aAllowedTypes);
    242242    HRESULT getParent(AutoCaller &autoCaller, ComPtr<IMedium> &aParent);
    243243    HRESULT getChildren(AutoCaller &autoCaller, std::vector<ComPtr<IMedium> > &aChildren);
    244244    HRESULT getBase(AutoCaller &autoCaller, ComPtr<IMedium> &aBase);
    245     HRESULT getReadOnly(BOOL *aReadOnly);
     245    HRESULT getReadOnly(AutoCaller &autoCaller, BOOL *aReadOnly);
    246246    HRESULT getLogicalSize(LONG64 *aLogicalSize);
    247247    HRESULT getAutoReset(BOOL *aAutoReset);
    … …  
    276276                              ComPtr<IProgress> &aProgress);
    277277    HRESULT deleteStorage(ComPtr<IProgress> &aProgress);
    278     HRESULT createDiffStorage(const ComPtr<IMedium> &aTarget,
     278    HRESULT createDiffStorage(AutoCaller &autoCaller,
     279                              const ComPtr<IMedium> &aTarget,
    279280                              const std::vector<MediumVariant_T> &aVariant,
    280281                              ComPtr<IProgress> &aProgress);
    … …  
    293294    HRESULT resize(LONG64 aLogicalSize,
    294295                   ComPtr<IProgress> &aProgress);
    295     HRESULT reset(ComPtr<IProgress> &aProgress);
     296    HRESULT reset(AutoCaller &autoCaller, ComPtr<IProgress> &aProgress);
    296297    HRESULT changeEncryption(const com::Utf8Str &aCurrentPassword, const com::Utf8Str &aCipher,
    297298                             const com::Utf8Str &aNewPassword, const com::Utf8Str &aNewPasswordId,

  • trunk/src/VBox/Main/src-server/MediumImpl.cpp

    r61009 r61137  
    15241524    /* It is possible that some previous/concurrent uninit has already cleared
    15251525     * the pVirtualBox reference, and in this case we don't need to continue.
    1526      * Normally this would be handled through the AutoUninitSpan magic,
    1527      * however this cannot be done at this point as the media tree must be
    1528      * locked before reaching the AutoUninitSpan, otherwise deadlocks can
    1529      * happen due to*/
     1526     * Normally this would be handled through the AutoUninitSpan magic, however
     1527     * this cannot be done at this point as the media tree must be locked
     1528     * before reaching the AutoUninitSpan, otherwise deadlocks can happen.
     1529     *
     1530     * NOTE: The tree lock is higher priority than the medium caller and medium
     1531     * object locks, i.e. the medium caller may have to be released and be
     1532     * re-acquired in the right place later. See Medium::getParent() for sample
     1533     * code how to do this safely. */
    15301534    ComObjPtr<VirtualBox> pVirtualBox(m->pVirtualBox);
    15311535    if (!pVirtualBox)
    … …  
    17311735}
    17321736
    1733 HRESULT Medium::getType(MediumType_T *aType)
    1734 {
     1737HRESULT Medium::getType(AutoCaller &autoCaller, MediumType_T *aType)
     1738{
     1739    NOREF(autoCaller);
    17351740    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
    17361741
    … …  
    17401745}
    17411746
    1742 HRESULT Medium::setType(MediumType_T aType)
    1743 {
    1744     // we access mParent and members
    1745     AutoWriteLock treeLock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
     1747HRESULT Medium::setType(AutoCaller &autoCaller, MediumType_T aType)
     1748{
     1749    autoCaller.release();
     1750
     1751    /* It is possible that some previous/concurrent uninit has already cleared
     1752     * the pVirtualBox reference, see #uninit(). */
     1753    ComObjPtr<VirtualBox> pVirtualBox(m->pVirtualBox);
     1754
     1755    // we access m->pParent
     1756    AutoReadLock treeLock(!pVirtualBox.isNull() ? &pVirtualBox->i_getMediaTreeLockHandle() : NULL COMMA_LOCKVAL_SRC_POS);
     1757
     1758    autoCaller.add();
     1759    if (FAILED(autoCaller.rc())) return autoCaller.rc();
     1760
    17461761    AutoWriteLock mlock(this COMMA_LOCKVAL_SRC_POS);
    17471762
    … …  
    18851900    ComObjPtr<VirtualBox> pVirtualBox(m->pVirtualBox);
    18861901
    1887     /* we access mParent */
     1902    /* we access m->pParent */
    18881903    AutoReadLock treeLock(!pVirtualBox.isNull() ? &pVirtualBox->i_getMediaTreeLockHandle() : NULL COMMA_LOCKVAL_SRC_POS);
    18891904
    … …  
    19281943}
    19291944
    1930 HRESULT Medium::getReadOnly(BOOL *aReadOnly)
    1931 {
     1945HRESULT Medium::getReadOnly(AutoCaller &autoCaller, BOOL *aReadOnly)
     1946{
     1947    autoCaller.release();
     1948
    19321949    /* isReadOnly() will do locking */
    19331950    *aReadOnly = i_isReadOnly();
    … …  
    25872604}
    25882605
    2589 HRESULT Medium::createDiffStorage(const ComPtr<IMedium> &aTarget,
     2606HRESULT Medium::createDiffStorage(AutoCaller &autoCaller,
     2607                                  const ComPtr<IMedium> &aTarget,
    25902608                                  const std::vector<MediumVariant_T> &aVariant,
    25912609                                  ComPtr<IProgress> &aProgress)
    25922610{
     2611    /** @todo r=klaus The code below needs to be double checked with regard
     2612     * to lock order violations, it probably causes lock order issues related
     2613     * to the AutoCaller usage. */
    25932614    IMedium *aT = aTarget;
    25942615    ComObjPtr<Medium> diff = static_cast<Medium*>(aT);
    25952616
    2596     // locking: we need the tree lock first because we access parent pointers
    2597     AutoMultiWriteLock3 alock(&m->pVirtualBox->i_getMediaTreeLockHandle(),
    2598                               this->lockHandle(), diff->lockHandle() COMMA_LOCKVAL_SRC_POS);
     2617    autoCaller.release();
     2618
     2619    /* It is possible that some previous/concurrent uninit has already cleared
     2620     * the pVirtualBox reference, see #uninit(). */
     2621    ComObjPtr<VirtualBox> pVirtualBox(m->pVirtualBox);
     2622
     2623    // we access m->pParent
     2624    AutoReadLock treeLock(!pVirtualBox.isNull() ? &pVirtualBox->i_getMediaTreeLockHandle() : NULL COMMA_LOCKVAL_SRC_POS);
     2625
     2626    autoCaller.add();
     2627    if (FAILED(autoCaller.rc())) return autoCaller.rc();
     2628
     2629    AutoMultiWriteLock2 alock(this->lockHandle(), diff->lockHandle() COMMA_LOCKVAL_SRC_POS);
    25992630
    26002631    if (m->type == MediumType_Writethrough)
    … …  
    26142645    MediumLockList *pMediumLockList(new MediumLockList());
    26152646    alock.release();
     2647    treeLock.release();
    26162648    HRESULT rc = diff->i_createMediumLockList(true /* fFailIfInaccessible */,
    26172649                                              diff /* pToLockWrite */,
    … …  
    26192651                                              this,
    26202652                                              *pMediumLockList);
     2653    treeLock.acquire();
    26212654    alock.acquire();
    26222655    if (FAILED(rc))
    … …  
    26272660
    26282661    alock.release();
     2662    treeLock.release();
    26292663    rc = pMediumLockList->Lock();
     2664    treeLock.acquire();
    26302665    alock.acquire();
    26312666    if (FAILED(rc))
    … …  
    26432678        diff->m->llRegistryIDs.push_back(parentMachineRegistry);
    26442679        alock.release();
     2680        treeLock.release();
    26452681        diff->i_markRegistriesModified();
     2682        treeLock.acquire();
    26462683        alock.acquire();
    26472684    }
    26482685
    26492686    alock.release();
     2687    treeLock.release();
    26502688
    26512689    ComObjPtr<Progress> pProgress;
    … …  
    26732711{
    26742712
     2713    /** @todo r=klaus The code below needs to be double checked with regard
     2714     * to lock order violations, it probably causes lock order issues related
     2715     * to the AutoCaller usage. */
    26752716    IMedium *aT = aTarget;
    26762717
    … …  
    27172758                        ComPtr<IProgress> &aProgress)
    27182759{
     2760    /** @todo r=klaus The code below needs to be double checked with regard
     2761     * to lock order violations, it probably causes lock order issues related
     2762     * to the AutoCaller usage. */
    27192763    ComAssertRet(aTarget != this, E_INVALIDARG);
    27202764
    … …  
    30103054}
    30113055
    3012 HRESULT Medium::reset(ComPtr<IProgress> &aProgress)
     3056HRESULT Medium::reset(AutoCaller &autoCaller, ComPtr<IProgress> &aProgress)
    30133057{
    30143058    HRESULT rc = S_OK;
    … …  
    30183062    try
    30193063    {
     3064        autoCaller.release();
     3065
     3066        /* It is possible that some previous/concurrent uninit has already
     3067         * cleared the pVirtualBox reference, see #uninit(). */
     3068        ComObjPtr<VirtualBox> pVirtualBox(m->pVirtualBox);
     3069
    30203070        /* canClose() needs the tree lock */
    3021         AutoMultiWriteLock2 multilock(&m->pVirtualBox->i_getMediaTreeLockHandle(),
     3071        AutoMultiWriteLock2 multilock(!pVirtualBox.isNull() ? &pVirtualBox->i_getMediaTreeLockHandle() : NULL,
    30223072                                      this->lockHandle()
    30233073                                      COMMA_LOCKVAL_SRC_POS);
     3074
     3075        autoCaller.add();
     3076        if (FAILED(autoCaller.rc())) return autoCaller.rc();
    30243077
    30253078        LogFlowThisFunc(("ENTER for medium %s\n", m->strLocationFull.c_str()));
    … …  
    40344087        return pBase;
    40354088
    4036     /* we access mParent */
     4089    /* we access m->pParent */
    40374090    AutoReadLock treeLock(pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
    40384091
    … …  
    40774130        return 1;
    40784131
    4079     /* we access mParent */
     4132    /* we access m->pParent */
    40804133    AutoReadLock treeLock(pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
    40814134
    … …  
    41034156bool Medium::i_isReadOnly()
    41044157{
     4158    /* it is possible that some previous/concurrent uninit has already cleared
     4159     * the pVirtualBox reference, and in this case we don't need to continue */
     4160    ComObjPtr<VirtualBox> pVirtualBox(m->pVirtualBox);
     4161    if (!pVirtualBox)
     4162        return false;
     4163
     4164    /* we access children */
     4165    AutoReadLock treeLock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
     4166
    41054167    AutoCaller autoCaller(this);
    41064168    AssertComRCReturn(autoCaller.rc(), false);
    4107 
    4108     /* we access children */
    4109     AutoReadLock treeLock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
    41104169
    41114170    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
    … …  
    42344293                               const Utf8Str &strHardDiskFolder)
    42354294{
     4295    /* we access m->pParent */
     4296    AutoReadLock treeLock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
     4297
    42364298    AutoCaller autoCaller(this);
    42374299    if (FAILED(autoCaller.rc())) return autoCaller.rc();
    4238 
    4239     /* we access mParent */
    4240     AutoReadLock treeLock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
    42414300
    42424301    i_saveSettingsOne(data, strHardDiskFolder);
    … …  
    42834342                                       MediumLockList &mediumLockList)
    42844343{
    4285     Assert(!m->pVirtualBox->i_getMediaTreeLockHandle().isWriteLockOnCurrentThread());
     4344    /** @todo r=klaus this needs to be reworked, as the code below uses
     4345     * i_getParent without holding the tree lock, and changing this is
     4346     * a significant amount of effort. */
     4347    Assert(m->pVirtualBox->i_getMediaTreeLockHandle().isWriteLockOnCurrentThread());
    42864348    Assert(!isWriteLockOnCurrentThread());
    42874349
    … …  
    46514713                              bool aWait)
    46524714{
     4715    /** @todo r=klaus The code below needs to be double checked with regard
     4716     * to lock order violations, it probably causes lock order issues related
     4717     * to the AutoCaller usage. */
    46534718    AssertReturn(aProgress != NULL || aWait == true, E_FAIL);
    46544719
    … …  
    49334998                                               bool &fMergeForward)
    49344999{
     5000    /** @todo r=klaus The code below needs to be double checked with regard
     5001     * to lock order violations, it probably causes lock order issues related
     5002     * to the AutoCaller usage. Likewise the code using this method seems
     5003     * problematic. */
    49355004    AssertReturn(pOther != NULL, E_FAIL);
    49365005    AssertReturn(pOther != this, E_FAIL);
    … …  
    50585127                                 MediumLockList * &aMediumLockList)
    50595128{
     5129    /** @todo r=klaus The code below needs to be double checked with regard
     5130     * to lock order violations, it probably causes lock order issues related
     5131     * to the AutoCaller usage. Likewise the code using this method seems
     5132     * problematic. */
    50605133    AssertReturn(pTarget != NULL, E_FAIL);
    50615134    AssertReturn(pTarget != this, E_FAIL);
    … …  
    55635636HRESULT Medium::i_fixParentUuidOfChildren(MediumLockList *pChildrenToReparent)
    55645637{
     5638    /** @todo r=klaus The code below needs to be double checked with regard
     5639     * to lock order violations, it probably causes lock order issues related
     5640     * to the AutoCaller usage. Likewise the code using this method seems
     5641     * problematic. */
    55655642    Assert(!isWriteLockOnCurrentThread());
    55665643    Assert(!m->pVirtualBox->i_getMediaTreeLockHandle().isWriteLockOnCurrentThread());
    … …  
    57425819                             const ComObjPtr<Progress> &aProgress)
    57435820{
     5821    /** @todo r=klaus The code below needs to be double checked with regard
     5822     * to lock order violations, it probably causes lock order issues related
     5823     * to the AutoCaller usage. */
    57445824    AssertPtrReturn(aFilename, E_INVALIDARG);
    57455825    AssertReturn(!aFormat.isNull(), E_INVALIDARG);
    … …  
    58385918                            uint32_t idxSrcImageSame, uint32_t idxDstImageSame)
    58395919{
     5920    /** @todo r=klaus The code below needs to be double checked with regard
     5921     * to lock order violations, it probably causes lock order issues related
     5922     * to the AutoCaller usage. */
    58405923    CheckComArgNotNull(aTarget);
    58415924    CheckComArgOutPointerValid(aProgress);
    … …  
    60336116 * @note Caller MUST NOT hold the media tree or medium lock.
    60346117 *
    6035  * @note Locks mParent for reading. Locks this object for writing.
     6118 * @note Locks m->pParent for reading. Locks this object for writing.
    60366119 *
    60376120 * @param fSetImageId Whether to reset the UUID contained in the image file to the UUID in the medium instance data (see SetIDs())
    … …  
    63426425                    /* must drop the caller before taking the tree lock */
    63436426                    autoCaller.release();
    6344                     /* we set mParent & children() */
     6427                    /* we set m->pParent & children() */
    63456428                    treeLock.acquire();
    63466429                    autoCaller.add();
    … …  
    63676450                    /* must drop the caller before taking the tree lock */
    63686451                    autoCaller.release();
    6369                     /* we access mParent */
     6452                    /* we access m->pParent */
    63706453                    treeLock.acquire();
    63716454                    autoCaller.add();
    … …  
    65776660     * VirtualBox::i_unregisterMedium() properly save the registry */
    65786661
    6579     /* we modify mParent and access children */
     6662    /* we modify m->pParent and access children */
    65806663    Assert(m->pVirtualBox->i_getMediaTreeLockHandle().isWriteLockOnCurrentThread());
    65816664
    … …  
    74387521HRESULT Medium::i_taskCreateBaseHandler(Medium::CreateBaseTask &task)
    74397522{
     7523    /** @todo r=klaus The code below needs to be double checked with regard
     7524     * to lock order violations, it probably causes lock order issues related
     7525     * to the AutoCaller usage. */
    74407526    HRESULT rc = S_OK;
    74417527
    … …  
    75817667HRESULT Medium::i_taskCreateDiffHandler(Medium::CreateDiffTask &task)
    75827668{
     7669    /** @todo r=klaus The code below needs to be double checked with regard
     7670     * to lock order violations, it probably causes lock order issues related
     7671     * to the AutoCaller usage. */
    75837672    HRESULT rcTmp = S_OK;
    75847673
    … …  
    78017890HRESULT Medium::i_taskMergeHandler(Medium::MergeTask &task)
    78027891{
     7892    /** @todo r=klaus The code below needs to be double checked with regard
     7893     * to lock order violations, it probably causes lock order issues related
     7894     * to the AutoCaller usage. */
    78037895    HRESULT rcTmp = S_OK;
    78047896
    … …  
    81118203HRESULT Medium::i_taskCloneHandler(Medium::CloneTask &task)
    81128204{
     8205    /** @todo r=klaus The code below needs to be double checked with regard
     8206     * to lock order violations, it probably causes lock order issues related
     8207     * to the AutoCaller usage. */
    81138208    HRESULT rcTmp = S_OK;
    81148209
    … …  
    83208415    if (SUCCEEDED(mrc) && fCreatingTarget)
    83218416    {
    8322         /* we set mParent & children() */
     8417        /* we set m->pParent & children() */
    83238418        AutoWriteLock treeLock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
    83248419
    … …  
    90499144HRESULT Medium::i_taskImportHandler(Medium::ImportTask &task)
    90509145{
     9146    /** @todo r=klaus The code below needs to be double checked with regard
     9147     * to lock order violations, it probably causes lock order issues related
     9148     * to the AutoCaller usage. */
    90519149    HRESULT rcTmp = S_OK;
    90529150
    … …  
    92189316    if (SUCCEEDED(mrc) && fCreatingTarget)
    92199317    {
    9220         /* we set mParent & children() */
     9318        /* we set m->pParent & children() */
    92219319        AutoWriteLock treeLock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
    92229320
Note: See TracChangeset for help on using the changeset viewer.

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