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

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 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list