Re: [PATCHv2 0.5/4] network: fix virNetworkObjAssignDef and persistence

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 04/23/2014 09:49 AM, Laine Stump wrote:
> Experimentation showed that if virNetworkCreateXML() was called for a
> network that was already defined, and then the network was
> subsequently shutdown, the network would continue to be persistent
> after the shutdown (expected/desired), but the original config would
> be lost in favor of the transient config sent in with
> virNetworkCreateXML() (which would then be the new persistent config)
> (obviously unexpected/not desired).
> 
> To fix this, virNetworkObjAssignDef() has been changed to
> 
> 1) properly save/free network->def and network->newDef for all the
> various combinations of live/active/persistent, including some
> combinations that were previously considered to be an error but didn't
> need to be (e.g. setting a "live" config for a network that isn't yet
> active but soon will be - that was previously considered an error,
> even though in practice it can be very useful).
> 
> 2) automatically set the persistent flag whenever a new non-live
> config is assigned to the network (and clear it when the non-live
> config is set to NULL). the libvirt network driver no longer directly
> manipulates network->persistent, but instead relies entirely on
> virNetworkObjAssignDef() to do the right thing automatically.
> 
> After this patch, the following sequence will behave as expected:
> 
> virNetworkDefineXML(X)
> virNetworkCreateXML(X') (same name but some config different)
> virNetworkDestroy(X)
> 
> At the end of these calls, the network config will remain as it was
> after the initial virNetworkDefine(), whereas previously it would take
> on the changes given during virNetworkCreateXML().
> 
> Another effect of this tighter coupling between a) setting a !live def
> and b) setting/clearing the "persistent" flag, is that future patches
> which change the details of network lifecycle management
> (e.g. upcoming patches to fix detection of "active" networks when
> libvirtd is restarted) will find it much more difficult to break
> persistence functionality.
> ---
>  src/conf/network_conf.c           | 78 +++++++++++++++++++++++----------------
>  src/conf/network_conf.h           |  6 +--
>  src/network/bridge_driver.c       | 34 +++++++----------
>  src/parallels/parallels_network.c |  4 +-
>  src/test/test_driver.c            |  5 +--
>  5 files changed, 64 insertions(+), 63 deletions(-)
> 
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 56c4a09..66945ce 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -302,46 +302,63 @@ void virNetworkObjListFree(virNetworkObjListPtr nets)
>  /*
>   * virNetworkObjAssignDef:
>   * @network: the network object to update
> - * @def: the new NetworkDef (will be consumed by this function iff successful)
> + * @def: the new NetworkDef (will be consumed by this function)
>   * @live: is this new def the "live" version, or the "persistent" version
>   *
> - * Replace the appropriate copy of the given network's NetworkDef
> + * Replace the appropriate copy of the given network's def or newDef
>   * with def. Use "live" and current state of the network to determine
> - * which to replace.
> + * which to replace and what to do with the old defs. When a non-live
> + * def is set, indicate that the network is now persistent.
> + *
> + * NB: a persistent network can be made transient by calling with:
> + * virNetworkObjAssignDef(network, NULL, false) (i.e. set the
> + * persistent def to NULL)
>   *
>   * Returns 0 on success, -1 on failure.
>   */
> -int
> +void

Well this doesn't jive with the comment above about what it returns...

>  virNetworkObjAssignDef(virNetworkObjPtr network,
>                         virNetworkDefPtr def,
>                         bool live)
>  {
> -    if (virNetworkObjIsActive(network)) {
> -        if (live) {
> +    if (live) {
> +        /* before setting new live def, save (into newDef) any
> +         * existing persistent (!live) def to be restored when the
> +         * network is destroyed, unless there is one already saved.
> +         */
> +        if (network->persistent && !network->newDef)
> +            network->newDef = network->def;
> +        else
>              virNetworkDefFree(network->def);
> -            network->def = def;
> -        } else if (network->persistent) {
> -            /* save current configuration to be restored on network shutdown */
> -            virNetworkDefFree(network->newDef);
> +        network->def = def;
> +    } else { /* !live */
> +        virNetworkDefFree(network->newDef);
> +        if (virNetworkObjIsActive(network)) {
> +            /* save new configuration to be restored on network
> +             * shutdown, leaving current live def alone
> +             */
>              network->newDef = def;
> -        } else {
> -            virReportError(VIR_ERR_OPERATION_INVALID,
> -                           _("cannot save persistent config of transient "
> -                             "network '%s'"), network->def->name);
> -            return -1;
> +        } else { /* !live and !active */
> +            if (network->def && !network->persistent) {
> +                /* network isn't (yet) marked active or persistent,
> +                 * but already has a "live" def set. This means we are
> +                 * currently setting the persistent def as a part of
> +                 * the process of starting the network, so we need to
> +                 * preserve the "not yet live" def in network->def.
> +                 */
> +                network->newDef = def;
> +            } else {
> +                /* either there is no live def set, or this network
> +                 * was already set as persistent, so the proper thing
> +                 * is to overwrite network->def.
> +                 */
> +                network->newDef = NULL;

If I'm reading correctly - the newDef is virNetworkDefFree()'d above as
we enter the "} else { /* !live */" clause, which is probably where this
statement should be moved.

> +                virNetworkDefFree(network->def);
> +                network->def = def;


NOTE: I think this is what is causing issues in networkUndefine() -
that'll call this function with '!live' - thus you'll do the free the
'def' and set it to NULL... Back in the caller there's all sorts of
touching of the network->def-> structure


> +            }
>          }
> -    } else if (!live) {
> -        virNetworkDefFree(network->newDef);
> -        virNetworkDefFree(network->def);
> -        network->newDef = NULL;
> -        network->def = def;
> -    } else {
> -        virReportError(VIR_ERR_OPERATION_INVALID,
> -                       _("cannot save live config of inactive "
> -                         "network '%s'"), network->def->name);
> -        return -1;
> +        network->persistent = !!def;
>      }
> -    return 0;
>  }
>  
>  /*
> @@ -365,10 +382,7 @@ virNetworkAssignDef(virNetworkObjListPtr nets,
>      virNetworkObjPtr network;
>  
>      if ((network = virNetworkFindByName(nets, def->name))) {
> -        if (virNetworkObjAssignDef(network, def, live) < 0) {
> -            virNetworkObjUnlock(network);
> -            return NULL;
> -        }
> +        virNetworkObjAssignDef(network, def, live);
>          return network;
>      }
>  
> @@ -392,8 +406,9 @@ virNetworkAssignDef(virNetworkObjListPtr nets,
>      ignore_value(virBitmapSetBit(network->class_id, 2));
>  
>      network->def = def;
> -
> +    network->persistent = !live;
>      return network;
> +
>   error:
>      virNetworkObjUnlock(network);
>      virNetworkObjFree(network);
> @@ -3118,7 +3133,6 @@ virNetworkObjPtr virNetworkLoadConfig(virNetworkObjListPtr nets,
>          goto error;
>  
>      net->autostart = autostart;
> -    net->persistent = 1;
>  
>      VIR_FREE(configFile);
>      VIR_FREE(autostartLink);
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index 1a864de..90da16f 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -335,9 +335,9 @@ typedef bool (*virNetworkObjListFilter)(virConnectPtr conn,
>  virNetworkObjPtr virNetworkAssignDef(virNetworkObjListPtr nets,
>                                       virNetworkDefPtr def,
>                                       bool live);
> -int virNetworkObjAssignDef(virNetworkObjPtr network,
> -                           virNetworkDefPtr def,
> -                           bool live);
> +void virNetworkObjAssignDef(virNetworkObjPtr network,
> +                            virNetworkDefPtr def,
> +                            bool live);
>  int virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live);
>  void virNetworkObjUnsetDefTransient(virNetworkObjPtr network);
>  virNetworkDefPtr virNetworkObjGetPersistentDef(virNetworkObjPtr network);
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index eb276cd..62d1c25 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -2667,10 +2667,11 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml)
>      if (networkValidate(driver, def, true) < 0)
>         goto cleanup;
>  
> -    /* NB: "live" is false because this transient network hasn't yet
> -     * been started
> +    /* NB: even though this transient network hasn't yet been started,
> +     * we assign the def with live = true in anticipation that it will
> +     * be started momentarily.
>       */
> -    if (!(network = virNetworkAssignDef(&driver->networks, def, false)))
> +    if (!(network = virNetworkAssignDef(&driver->networks, def, true)))
>          goto cleanup;
>      def = NULL;
>  
> @@ -2719,19 +2720,10 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml)
>      if (networkValidate(driver, def, false) < 0)
>         goto cleanup;
>  
> -    if ((network = virNetworkFindByName(&driver->networks, def->name))) {
> -        network->persistent = 1;
> -        if (virNetworkObjAssignDef(network, def, false) < 0)
> -            goto cleanup;
> -    } else {
> -        if (!(network = virNetworkAssignDef(&driver->networks, def, false)))
> -            goto cleanup;
> -    }
> -
> -    /* define makes the network persistent - always */
> -    network->persistent = 1;
> +    if (!(network = virNetworkAssignDef(&driver->networks, def, false)))
> +       goto cleanup;
>  
> -    /* def was asigned */
> +    /* def was assigned to network object */
>      freeDef = false;
>  
>      if (virNetworkSaveConfig(driver->networkConfigDir, def) < 0) {
> @@ -2740,9 +2732,11 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml)
>              network = NULL;
>              goto cleanup;
>          }
> -        network->persistent = 0;
> -        virNetworkDefFree(network->newDef);
> -        network->newDef = NULL;
> +        /* if network was active already, just undo new persistent
> +         * definition by making it transient.
> +         * XXX - this isn't necessarily the correct thing to do.
> +         */

Leaving the XXX comment in here leaves me wondering what is the correct
thing to do...

> +        virNetworkObjAssignDef(network, NULL, false);
>          goto cleanup;
>      }
>  
> @@ -2794,10 +2788,8 @@ networkUndefine(virNetworkPtr net)
>          goto cleanup;
>  
>      /* make the network transient */
> -    network->persistent = 0;
>      network->autostart = 0;
> -    virNetworkDefFree(network->newDef);
> -    network->newDef = NULL;
> +    virNetworkObjAssignDef(network, NULL, false);

This seems to be the cause of the crash I noted in private chat. If I
restore just these lines and not make the ObjAssignDef() call, then
things work again.  That includes issues I was seeing in virt-test for
patches 3 & 4 as well.

Just figured I'd close the loop on this review at least...

John
>  
>      event = virNetworkEventLifecycleNew(network->def->name,
>                                          network->def->uuid,
> diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c
> index cbb44b9..0ebeb7b 100644
> --- a/src/parallels/parallels_network.c
> +++ b/src/parallels/parallels_network.c
> @@ -2,7 +2,7 @@
>   * parallels_network.c: core privconn functions for managing
>   * Parallels Cloud Server hosts
>   *
> - * Copyright (C) 2013 Red Hat, Inc.
> + * Copyright (C) 2013-2014 Red Hat, Inc.
>   * Copyright (C) 2012 Parallels, Inc.
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -231,7 +231,6 @@ parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj)
>          goto cleanup;
>      }
>      net->active = 1;
> -    net->persistent = 1;
>      net->autostart = 1;
>      virNetworkObjUnlock(net);
>      return net;
> @@ -267,7 +266,6 @@ parallelsAddRoutedNetwork(parallelsConnPtr privconn)
>          goto cleanup;
>      }
>      net->active = 1;
> -    net->persistent = 1;
>      net->autostart = 1;
>      virNetworkObjUnlock(net);
>  
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 64f3daa..37756e7 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -784,7 +784,6 @@ testOpenDefault(virConnectPtr conn)
>          goto error;
>      }
>      netobj->active = 1;
> -    netobj->persistent = 1;
>      virNetworkObjUnlock(netobj);
>  
>      if (!(interfacedef = virInterfaceDefParseString(defaultInterfaceXML)))
> @@ -1155,7 +1154,6 @@ testParseNetworks(testConnPtr privconn,
>              goto error;
>          }
>  
> -        obj->persistent = 1;
>          obj->active = 1;
>          virNetworkObjUnlock(obj);
>      }
> @@ -3711,7 +3709,7 @@ static virNetworkPtr testNetworkCreateXML(virConnectPtr conn, const char *xml)
>      if ((def = virNetworkDefParseString(xml)) == NULL)
>          goto cleanup;
>  
> -    if (!(net = virNetworkAssignDef(&privconn->networks, def, false)))
> +    if (!(net = virNetworkAssignDef(&privconn->networks, def, true)))
>          goto cleanup;
>      def = NULL;
>      net->active = 1;
> @@ -3748,7 +3746,6 @@ virNetworkPtr testNetworkDefineXML(virConnectPtr conn, const char *xml)
>      if (!(net = virNetworkAssignDef(&privconn->networks, def, false)))
>          goto cleanup;
>      def = NULL;
> -    net->persistent = 1;
>  
>      event = virNetworkEventLifecycleNew(net->def->name, net->def->uuid,
>                                          VIR_NETWORK_EVENT_DEFINED,
> 


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]