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

[Osier Yang]

On 2012年12月28日 10:50, Osier Yang wrote:
> 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.

Checked LXC and QEMU driver, qemu's defineXML is a bit better as
it already tries to restore the previous def, but it still misses
to restore vm->persistent. LXC driver is just like network driver,
no restoring. I guess same problems are existing for other drivers
which supports persistent object, so we will need a bunch of patches
for them.

> [4] https://www.redhat.com/archives/libvir-list/2012-December/msg01355.html
>
> Osier
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list