[libvirt] [PATCHv2 7/6] net: Mark network persistent when assigning persistent definition

[Osier Yang]

On 2012年11月02日 01:30, Laine Stump wrote:
 > On 11/01/2012 11:25 AM, Eric Blake wrote:
 >> On 10/29/2012 03:35 AM, Peter Krempa wrote:
 >>> When assigning the new persistent definition for a transient network
 >>> (thus making it persistent) the network needs to be marked persistent
 >>> before actually atempting to assign the definition.
 >>> ---
 >>>   src/network/bridge_driver.c | 16 +++++++++++-----
 >>>   1 file changed, 11 insertions(+), 5 deletions(-)
 >> You might want to get Laine's opinion as well, but I think this is a
 >> candidate for 1.0.0, and looks right to me.
 >
 > Transient networks have never worked very well (mainly because nobody
 > has ever used them) and the other patches in this series (plus more) are
 > needed to make them work properly, so I think it makes more sense to
 > save them all until after 1.0 is released.
 >
 >>
 >> ACK.
 >>
 >>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
 >>> index 22bd99b..643b00c 100644
 >>> --- a/src/network/bridge_driver.c
 >>> +++ b/src/network/bridge_driver.c
 >>> @@ -2820,7 +2820,7 @@ cleanup:
 >>>
 >>>   static virNetworkPtr networkDefine(virConnectPtr conn, const char 
*xml) {
 >>>       struct network_driver *driver = conn->networkPrivateData;
 >>> -    virNetworkDefPtr def;
 >>> +    virNetworkDefPtr def = NULL;
 >>>       bool freeDef = true;
 >>>       virNetworkObjPtr network = NULL;
 >>>       virNetworkPtr ret = NULL;
 >>> @@ -2833,11 +2833,17 @@ static virNetworkPtr 
networkDefine(virConnectPtr conn, const char *xml) {
 >>>       if (networkValidate(driver, def, false)<  0)
 >>>          goto cleanup;
 >>>
 >>> -    if (!(network = virNetworkAssignDef(&driver->networks, def, 
false)))
 >>> -        goto cleanup;
 >>> -    freeDef = false;
 >>> +    if ((network = virNetworkFindByName(&driver->networks, 
def->name))) {
 >>> +        network->persistent = 1;
 >>> +        if (virNetworkObjAssignDef(network, def, false)<  0)
 >>> +            goto cleanup;

Except the introduced regression [1]. This could cause incorrect result
once virNetworkObjAssignDef fails, though I believe it's unlikely to
fail (Assuming there is a same network, but transient, it will be
changed to persistent).

 >>> +    } else {
 >>> +        if (!(network = virNetworkAssignDef(&driver->networks, 
def, false)))
 >>> +            goto cleanup;
 >>> +    }
 >>>
 >>> -    network->persistent = 1;
 >>> +    /* def was asigned */
 >>> +    freeDef = false;
 >>>
 >>>       if (virNetworkSaveConfig(driver->networkConfigDir, def)<  0) {
 >>>           virNetworkRemoveInactive(&driver->networks, network);

And virNetworkRemoveInactive could be bindly removed a previous inactive
network obj. Also we have to restore the previous network's def if
virNetworkSaveConfig fails.

[4] https://www.redhat.com/archives/libvir-list/2012-December/msg01355.html

Osier