VirtualBox

Changeset 57694 in vbox for trunk


Ignore:
Timestamp:
Sep 10, 2015 4:35:59 PM (9 years ago)
Author:
vboxsync
Message:

Main/NATEngine+NetworkAdapter: NetworkAdapter::i_copyFrom didn't update the NATEngine settings, which lacked the corresponding logic entirely and had totally incorrect commit/rollback handling for the settings. The NATEngine commit method was called from the wrong place and rollback was never called. The NATEngine code for updating the port forwarding rules was done ignoring the commit/rollback handling for no good reason, which was covered up by a number of hacks. Eliminated the error prone separate m_fModified flags.

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

Legend:

Unmodified
Added
Removed

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

    r49874 r57694  
    77
    88/*
    9  * Copyright (C) 2006-2013 Oracle Corporation
     9 * Copyright (C) 2006-2015 Oracle Corporation
    1010 *
    1111 * This file is part of VirtualBox Open Source Edition (OSE), as
    … …  
    3434    public NATEngineWrap
    3535{
    36     public:
    37     typedef std::map<Utf8Str, settings::NATRule> NATRuleMap;
     36public:
    3837
    3938    DECLARE_EMPTY_CTOR_DTOR(NATEngine)
    … …  
    4847
    4948    bool i_isModified();
    50     bool i_rollback();
     49    void i_rollback();
    5150    void i_commit();
     51    void i_copyFrom(NATEngine *aThat);
    5252    HRESULT i_loadSettings(const settings::NAT &data);
    5353    HRESULT i_saveSettings(settings::NAT &data);
    … …  
    103103    struct Data;
    104104    Data *mData;
    105     bool m_fModified;
    106105    const ComObjPtr<NATEngine> mPeer;
    107106    Machine * const mParent;
    108     NATRuleMap mNATRules;
    109107    INetworkAdapter * const mAdapter;
    110108};

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

    r49951 r57694  
    77
    88/*
    9  * Copyright (C) 2006-2013 Oracle Corporation
     9 * Copyright (C) 2006-2015 Oracle Corporation
    1010 *
    1111 * This file is part of VirtualBox Open Source Edition (OSE), as
    … …  
    155155    const ComObjPtr<NATEngine> mNATEngine;
    156156
    157     bool                m_fModified;
    158157    Backupable<Data>    mData;
    159158};

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

    r51498 r57694  
    55
    66/*
    7  * Copyright (C) 2010-2014 Oracle Corporation
     7 * Copyright (C) 2010-2015 Oracle Corporation
    88 *
    99 * This file is part of VirtualBox Open Source Edition (OSE), as
    … …  
    2828#include <VBox/settings.h>
    2929#include <VBox/com/array.h>
     30
     31typedef std::map<Utf8Str, settings::NATRule> NATRuleMap;
    3032
    3133struct NATEngineData
    … …  
    5961    /* Alias service */
    6062    ULONG mAliasMode;
     63    /* Port forwarding rules */
     64    NATRuleMap mNATRules;
    6165};
    6266
    … …  
    9094    AssertReturn(autoInitSpan.isOk(), E_FAIL);
    9195    autoInitSpan.setSucceeded();
    92     m_fModified = false;
    9396    mData = new Data();
    9497    mData->m.allocate();
    … …  
    113116    mData = new Data();
    114117    mData->m.share(aThat->mData->m);
    115     NATRuleMap::iterator it;
    116     mNATRules.clear();
    117     for (it = aThat->mNATRules.begin(); it != aThat->mNATRules.end(); ++it)
    118     {
    119         mNATRules.insert(std::make_pair(it->first, it->second));
    120     }
    121118    unconst(mParent) = aParent;
    122119    unconst(mAdapter) = aAdapter;
    … …  
    140137    mData = new Data();
    141138    mData->m.attachCopy(aThat->mData->m);
    142     NATRuleMap::iterator it;
    143     mNATRules.clear();
    144     for (it = aThat->mNATRules.begin(); it != aThat->mNATRules.end(); ++it)
    145     {
    146         mNATRules.insert(std::make_pair(it->first, it->second));
    147     }
    148139    unconst(mAdapter) = aAdapter;
    149140    unconst(mParent) = aParent;
    … …  
    159150        return;
    160151
    161     mNATRules.clear();
    162152    mData->m.free();
    163153    delete mData;
    … …  
    169159bool NATEngine::i_isModified()
    170160{
    171     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
    172     bool fModified = m_fModified;
     161    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
     162    bool fModified = mData->m.isBackedUp();
    173163    return fModified;
    174164}
    175165
    176 bool NATEngine::i_rollback()
     166void NATEngine::i_rollback()
    177167{
    178168    AutoCaller autoCaller(this);
    179     AssertComRCReturn(autoCaller.rc(), false);
    180 
    181     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
    182     bool fChanged = m_fModified;
    183 
    184     if (m_fModified)
    185     {
    186         /* we need to check all data to see whether anything will be changed
    187          * after rollback */
    188         mData->m.rollback();
    189     }
    190     m_fModified = false;
    191     return fChanged;
     169    AssertComRCReturnVoid(autoCaller.rc());
     170
     171    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
     172
     173    mData->m.rollback();
    192174}
    193175
    … …  
    204186     * first) */
    205187    AutoMultiWriteLock2 alock(mPeer, this COMMA_LOCKVAL_SRC_POS);
    206     if (m_fModified)
     188    if (mData->m.isBackedUp())
    207189    {
    208190        mData->m.commit();
    209191        if (mPeer)
    210         {
    211192            mPeer->mData->m.attach(mData->m);
    212             mPeer->mNATRules.clear();
    213             NATRuleMap::iterator it;
    214             for (it = mNATRules.begin(); it != mNATRules.end(); ++it)
    215             {
    216                 mPeer->mNATRules.insert(std::make_pair(it->first, it->second));
    217             }
    218         }
    219     }
    220     m_fModified = false;
     193    }
     194}
     195
     196void NATEngine::i_copyFrom(NATEngine *aThat)
     197{
     198    AssertReturnVoid(aThat != NULL);
     199
     200    /* sanity */
     201    AutoCaller autoCaller(this);
     202    AssertComRCReturnVoid(autoCaller.rc());
     203
     204    /* sanity too */
     205    AutoCaller thatCaller(aThat);
     206    AssertComRCReturnVoid(thatCaller.rc());
     207
     208    /* peer is not modified, lock it for reading (aThat is "master" so locked
     209     * first) */
     210    AutoReadLock rl(aThat COMMA_LOCKVAL_SRC_POS);
     211    AutoWriteLock wl(this COMMA_LOCKVAL_SRC_POS);
     212
     213    /* this will back up current data */
     214    mData->m.assignCopy(aThat->mData->m);
    221215}
    222216
    … …  
    245239    {
    246240        mData->m.backup();
    247         m_fModified = true;
     241        mParent->i_setModified(Machine::IsModified_NetworkAdapters);
    248242    }
    249243    if (aMtu)
    … …  
    258252        mData->m->mTcpRcv = aTcpWndRcv;
    259253
    260     if (m_fModified)
    261         mParent->i_setModified(Machine::IsModified_NetworkAdapters);
    262254    return S_OK;
    263255}
    … …  
    268260    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
    269261
    270     aRedirects.resize(mNATRules.size());
     262    aRedirects.resize(mData->m->mNATRules.size());
    271263    size_t i = 0;
    272264    NATRuleMap::const_iterator it;
    273     for (it = mNATRules.begin(); it != mNATRules.end(); ++it, ++i)
     265    for (it = mData->m->mNATRules.begin(); it != mData->m->mNATRules.end(); ++it, ++i)
    274266    {
    275267        settings::NATRule r = it->second;
    … …  
    307299
    308300    NATRuleMap::iterator it;
    309     for (it = mNATRules.begin(); it != mNATRules.end(); ++it)
     301    for (it = mData->m->mNATRules.begin(); it != mData->m->mNATRules.end(); ++it)
    310302    {
    311303        r = it->second;
    … …  
    320312    }
    321313
     314    mData->m.backup();
    322315    r.strName = name.c_str();
    323316    r.proto = aProto;
    … …  
    326319    r.strGuestIP = aGuestIP;
    327320    r.u16GuestPort = aGuestPort;
    328     mNATRules.insert(std::make_pair(name, r));
     321    mData->m->mNATRules.insert(std::make_pair(name, r));
    329322    mParent->i_setModified(Machine::IsModified_NetworkAdapters);
    330     m_fModified = true;
    331323
    332324    ULONG ulSlot;
    … …  
    342334{
    343335    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
    344     NATRuleMap::iterator it = mNATRules.find(aName);
    345     if (it == mNATRules.end())
     336    NATRuleMap::iterator it = mData->m->mNATRules.find(aName);
     337    if (it == mData->m->mNATRules.end())
    346338        return E_INVALIDARG;
    347339    mData->m.backup();
    … …  
    355347    mAdapter->COMGETTER(Slot)(&ulSlot);
    356348
    357     mNATRules.erase(it);
     349    mData->m->mNATRules.erase(it);
    358350    mParent->i_setModified(Machine::IsModified_NetworkAdapters);
    359     m_fModified = true;
    360     mData->m.commit();
    361351    alock.release();
    362352    mParent->i_onNATRedirectRuleChange(ulSlot, TRUE, Bstr(aName).raw(), proto, Bstr(strHostIP).raw(),
    … …  
    391381    mData->m->mAliasMode |= (data.fAliasProxyOnly    ? NATAliasMode_AliasProxyOnly    : 0);
    392382    /* port forwarding */
    393     mNATRules.clear();
     383    mData->m->mNATRules.clear();
    394384    for (settings::NATRuleList::const_iterator it = data.llRules.begin();
    395385        it != data.llRules.end(); ++it)
    396386    {
    397         mNATRules.insert(std::make_pair(it->strName, *it));
    398     }
    399     m_fModified = false;
     387        mData->m->mNATRules.insert(std::make_pair(it->strName, *it));
     388    }
    400389    return rc;
    401390}
    … …  
    429418    data.fAliasUseSamePorts = !!(mData->m->mAliasMode & NATAliasMode_AliasUseSamePorts);
    430419
    431     for (NATRuleMap::iterator it = mNATRules.begin();
    432         it != mNATRules.end(); ++it)
     420    for (NATRuleMap::iterator it = mData->m->mNATRules.begin();
     421        it != mData->m->mNATRules.end(); ++it)
    433422        data.llRules.push_back(it->second);
    434     m_fModified = false;
    435423    return rc;
    436424}
    … …  
    444432        mData->m->mNetwork = aNetwork;
    445433        mParent->i_setModified(Machine::IsModified_NetworkAdapters);
    446         m_fModified = true;
    447434    }
    448435    return S_OK;
    … …  
    469456        mData->m->mBindIP = aHostIP;
    470457        mParent->i_setModified(Machine::IsModified_NetworkAdapters);
    471         m_fModified = true;
    472458    }
    473459    return S_OK;
    … …  
    491477        mData->m->mTFTPPrefix = aTFTPPrefix;
    492478        mParent->i_setModified(Machine::IsModified_NetworkAdapters);
    493         m_fModified = true;
    494479    }
    495480    return S_OK;
    … …  
    517502        mData->m->mTFTPBootFile = aTFTPBootFile;
    518503        mParent->i_setModified(Machine::IsModified_NetworkAdapters);
    519         m_fModified = true;
    520504    }
    521505    return S_OK;
    … …  
    543527        mData->m->mTFTPNextServer = aTFTPNextServer;
    544528        mParent->i_setModified(Machine::IsModified_NetworkAdapters);
    545         m_fModified = true;
    546529    }
    547530    return S_OK;
    … …  
    569552        mData->m->mDNSPassDomain = aDNSPassDomain;
    570553        mParent->i_setModified(Machine::IsModified_NetworkAdapters);
    571         m_fModified = true;
    572554    }
    573555    return S_OK;
    … …  
    591573        mData->m->mDNSProxy = aDNSProxy;
    592574        mParent->i_setModified(Machine::IsModified_NetworkAdapters);
    593         m_fModified = true;
    594575    }
    595576    return S_OK;
    … …  
    619600        mData->m->mDNSUseHostResolver = aDNSUseHostResolver;
    620601        mParent->i_setModified(Machine::IsModified_NetworkAdapters);
    621         m_fModified = true;
    622602    }
    623603    return S_OK;
    … …  
    633613        mData->m->mAliasMode = aAliasMode;
    634614        mParent->i_setModified(Machine::IsModified_NetworkAdapters);
    635         m_fModified = true;
    636615    }
    637616    return S_OK;

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

    r56867 r57694  
    8181    mNATEngine->init(aParent, this);
    8282    /* mPeer is left null */
    83 
    84     m_fModified = false;
    8583
    8684    mData.allocate();
    … …  
    129127
    130128    unconst(mParent) = aParent;
     129    /* mPeer is left null */
     130
    131131    unconst(mNATEngine).createObject();
    132132    mNATEngine->init(aParent, this, aThat->mNATEngine);
    … …  
    180180    mNATEngine->initCopy(aParent, this, aThat->mNATEngine);
    181181
     182    /* sanity */
    182183    AutoCaller thatCaller(aThat);
    183184    AssertComRCReturnRC(thatCaller.rc());
    … …  
    256257        mData->mAdapterType = aAdapterType;
    257258
    258         m_fModified = true;
    259259        // leave the lock before informing callbacks
    260260        alock.release();
    … …  
    304304        mData->mEnabled = aEnabled;
    305305
    306         m_fModified = true;
    307306        // leave the lock before informing callbacks
    308307        alock.release();
    … …  
    394393    if (SUCCEEDED(rc))
    395394    {
    396         m_fModified = true;
    397395        // leave the lock before informing callbacks
    398396        alock.release();
    399 
    400397
    401398        AutoWriteLock mlock(mParent COMMA_LOCKVAL_SRC_POS);       // mParent is const, no need to lock
    … …  
    449446        mData->mAttachmentType = aAttachmentType;
    450447
    451         m_fModified = true;
    452448        // leave the lock before informing callbacks
    453449        alock.release();
    … …  
    501497        mData->mBridgedInterface = aBridgedInterface;
    502498
    503         m_fModified = true;
    504499        // leave the lock before informing callbacks
    505500        alock.release();
    … …  
    548543        mData.backup();
    549544        mData->mHostOnlyInterface = aHostOnlyInterface;
    550 
    551         m_fModified = true;
    552545
    553546        // leave the lock before informing callbacks
    … …  
    597590        mData->mInternalNetwork = aInternalNetwork;
    598591
    599         m_fModified = true;
    600592        // leave the lock before informing callbacks
    601593        alock.release();
    … …  
    646638        mData->mNATNetwork = aNATNetwork;
    647639
    648         m_fModified = true;
    649640        // leave the lock before informing callbacks
    650641        alock.release();
    … …  
    720711        mData->mCableConnected = aConnected;
    721712
    722         m_fModified = true;
    723713        // leave the lock before informing callbacks
    724714        alock.release();
    … …  
    758748        mData->mLineSpeed = aSpeed;
    759749
    760         m_fModified = true;
    761750        // leave the lock before informing callbacks
    762751        alock.release();
    … …  
    808797            mData.backup();
    809798            mData->mPromiscModePolicy = aPromiscModePolicy;
    810             m_fModified = true;
    811799
    812800            alock.release();
    … …  
    843831        mData->mTraceEnabled = aEnabled;
    844832
    845         m_fModified = true;
    846833        // leave the lock before informing callbacks
    847834        alock.release();
    … …  
    881868        mData->mTraceFile = aTraceFile;
    882869
    883         m_fModified = true;
    884870        // leave the lock before informing callbacks
    885871        alock.release();
    … …  
    927913        mData->mBootPriority = aBootPriority;
    928914
    929         m_fModified = true;
    930915        // leave the lock before informing callbacks
    931916        alock.release();
    … …  
    10931078    if (FAILED(rc)) return rc;
    10941079
    1095     // after loading settings, we are no longer different from the XML on disk
    1096     m_fModified = false;
    1097 
    10981080    return S_OK;
    10991081}
    … …  
    11341116    data.mode = mData->mAttachmentType;
    11351117
    1136     mNATEngine->i_commit();
    11371118    mNATEngine->i_saveSettings(data.nat);
    11381119
    … …  
    11471128
    11481129    data.strNATNetworkName = mData->mNATNetwork;
    1149 
    1150     // after saving settings, we are no longer different from the XML on disk
    1151     m_fModified = false;
    11521130
    11531131    return S_OK;
    … …  
    11621140    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
    11631141
    1164     bool fChanged = m_fModified;
     1142    bool fChanged = mData.isBackedUp();
    11651143    fChanged |= (mData->mAdapterType == NetworkAttachmentType_NAT? mNATEngine->i_isModified() : false);
    11661144    return fChanged;
    … …  
    11771155
    11781156    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
     1157
     1158    mNATEngine->i_rollback();
    11791159
    11801160    mData.rollback();
    … …  
    11981178     * first) */
    11991179    AutoMultiWriteLock2 alock(mPeer, this COMMA_LOCKVAL_SRC_POS);
     1180
     1181    mNATEngine->i_commit();
    12001182
    12011183    if (mData.isBackedUp())
    … …  
    12251207    AutoCaller thatCaller(aThat);
    12261208    AssertComRCReturnVoid(thatCaller.rc());
     1209
     1210    mNATEngine->i_copyFrom(aThat->mNATEngine);
    12271211
    12281212    /* peer is not modified, lock it for reading (aThat is "master" so locked
    … …  
    13451329        i_updateBandwidthGroup(pBwGroup);
    13461330
    1347         m_fModified = true;
    13481331        // leave the lock before informing callbacks
    13491332        alock.release();
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