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

Re: [libvirt] [PATCHv2 3/9] network: utility functions for updating network config



On Tue, Sep 18, 2012 at 03:38:59AM -0400, Laine Stump wrote:
> These new functions are highly inspired by those in domain_conf.c (but
> not identical), and are intended to make it simpler to update the
> various combinations of live/persistent network configs.
> 
> The network driver wasn't previously as careful about the separation
> between the live "status" in network->def and the persistent "config"
> in network->newDef (or sometimes in network->def). This series
> attempts to remedy some of that, but probably doesn't go all the way
> (enough to get these functions working and enable continued work on
> virNetworkUpdate though).
> 
> bridge_driver.c and test_driver.c were updated in a few places to take
> advantage of the new functions and/or account for changes in argument
> lists.
> ---
>  src/conf/network_conf.c     | 234 ++++++++++++++++++++++++++++++++++++++++++--
>  src/conf/network_conf.h     |  16 ++-
>  src/libvirt_private.syms    |  10 +-
>  src/network/bridge_driver.c |  14 ++-
>  src/test/test_driver.c      |   9 +-
>  5 files changed, 262 insertions(+), 21 deletions(-)
> 
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 88e1492..a48eb9e 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -228,20 +228,74 @@ void virNetworkObjListFree(virNetworkObjListPtr nets)
>      nets->count = 0;
>  }
>  
> -virNetworkObjPtr virNetworkAssignDef(virNetworkObjListPtr nets,
> -                                     const virNetworkDefPtr def)
> +/*
> + * virNetworkObjAssignDef:
> + * @network: the network object to update
> + * @def: the new NetworkDef (will be consumed by this function iff successful)
> + * @live: is this new def the "live" version, or the "persistent" version
> + *
> + * Replace the appropriate copy of the given network's NetworkDef
> + * with def. Use "live" and current state of the network to determine
> + * which to replace.
> + *
> + * Returns 0 on success, -1 on failure.
> + */
> +int
> +virNetworkObjAssignDef(virNetworkObjPtr network,
> +                       const virNetworkDefPtr def,
> +                       bool live)
>  {
> -    virNetworkObjPtr network;
> -
> -    if ((network = virNetworkFindByName(nets, def->name))) {
> -        if (!virNetworkObjIsActive(network)) {
> +    if (virNetworkObjIsActive(network)) {
> +        if (live) {
>              virNetworkDefFree(network->def);
>              network->def = def;
> -        } else {
> +        } else if (network->persistent) {
> +            /* save current configuration to be restored on network shutdown */
>              virNetworkDefFree(network->newDef);
>              network->newDef = def;
> +        } else {
> +            virReportError(VIR_ERR_OPERATION_INVALID,
> +                           _("cannot save persistent config of transient "
> +                             "network '%s'"), network->def->name);
> +            return -1;
>          }
> +    } else if (!live) {
> +        virNetworkDefFree(network->newDef); /* should be unnecessary */
> +        virNetworkDefFree(network->def);
> +        network->def = def;
> +    } else {
> +        virReportError(VIR_ERR_OPERATION_INVALID,
> +                       _("cannot save live config of inactive "
> +                         "network '%s'"), network->def->name);
> +        return -1;
> +    }
> +    return 0;
> +}
> +
> +/*
> + * virNetworkAssignDef:
> + * @nets: list of all networks
> + * @def: the new NetworkDef (will be consumed by this function iff successful)
> + * @live: is this new def the "live" version, or the "persistent" version
> + *
> + * Either replace the appropriate copy of the NetworkDef with name
> + * matching def->name or, if not found, create a new NetworkObj with
> + * def. For an existing network, use "live" and current state of the
> + * network to determine which to replace.
> + *
> + * Returns -1 on failure, 0 on success.
> + */
> +virNetworkObjPtr
> +virNetworkAssignDef(virNetworkObjListPtr nets,
> +                    const virNetworkDefPtr def,
> +                    bool live)
> +{
> +    virNetworkObjPtr network;
>  
> +    if ((network = virNetworkFindByName(nets, def->name))) {
> +        if (virNetworkObjAssignDef(network, def, live) < 0) {
> +            return NULL;
> +        }
>          return network;
>      }
>  
> @@ -270,6 +324,150 @@ virNetworkObjPtr virNetworkAssignDef(virNetworkObjListPtr nets,
>  
>  }
>  
> +/*
> + * virNetworkObjSetDefTransient:
> + * @network: network object pointer
> + * @live: if true, run this operation even for an inactive network.
> + *   this allows freely updated network->def with runtime defaults
> + *   before starting the network, which will be discarded on network
> + *   shutdown. Any cleanup paths need to be sure to handle newDef if
> + *   the network is never started.
> + *
> + * Mark the active network config as transient. Ensures live-only update
> + * operations do not persist past network destroy.
> + *
> + * Returns 0 on success, -1 on failure
> + */
> +int
> +virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live)
> +{
> +    if (!virNetworkObjIsActive(network) && !live)
> +        return 0;
> +
> +    if (!network->persistent || network->newDef)
> +        return 0;
> +
> +    network->newDef = virNetworkDefCopy(network->def, VIR_NETWORK_XML_INACTIVE);
> +    return network->newDef ? 0 : -1;
> +}
> +
> +/*
> + * virNetworkObjGetPersistentDef:
> + * @network: network object pointer
> + *
> + * Return the persistent network configuration. If network is transient,
> + * return the running config.
> + *
> + * Returns NULL on error, virNetworkDefPtr on success.
> + */
> +virNetworkDefPtr
> +virNetworkObjGetPersistentDef(virNetworkObjPtr network)
> +{
> +    if (network->newDef)
> +        return network->newDef;
> +    else
> +        return network->def;
> +}
> +
> +/*
> + * virNetworkObjReplacePersistentDef:
> + * @network: network object pointer
> + * @def: new virNetworkDef to replace current persistent config
> + *
> + * Replace the "persistent" network configuration with the given new
> + * virNetworkDef. This pays attention to whether or not the network
> + * is active.
> + *
> + * Returns -1 on error, 0 on success
> + */
> +int
> +virNetworkObjReplacePersistentDef(virNetworkObjPtr network,
> +                                  virNetworkDefPtr def)
> +{
> +    if (virNetworkObjIsActive(network)) {
> +        virNetworkDefFree(network->newDef);
> +        network->newDef = def;
> +    } else {
> +        virNetworkDefFree(network->def);
> +        network->def = def;
> +    }
> +    return 0;
> +}
> +
> +/*
> + * virNetworkDefCopy:
> + * @def: NetworkDef to copy
> + * @flags: VIR_NETWORK_XML_INACTIVE if appropriate
> + *
> + * make a deep copy of the given NetworkDef
> + *
> + * Returns a new NetworkDef on success, or NULL on failure.
> + */
> +virNetworkDefPtr
> +virNetworkDefCopy(virNetworkDefPtr def, unsigned int flags)
> +{
> +    char *xml = NULL;
> +    virNetworkDefPtr newDef = NULL;
> +
> +    if (!def) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       "%s", _("NULL NetworkDef"));
> +        return NULL;
> +    }
> +
> +    /* deep copy with a format/parse cycle */
> +    if (!(xml = virNetworkDefFormat(def, flags)))
> +        goto cleanup;
> +    newDef = virNetworkDefParseString(xml);
> +cleanup:
> +    VIR_FREE(xml);
> +    return newDef;
> +}
> +
> +/*
> + * virNetworkConfigChangeSetup:
> + *
> + * 1) checks whether network state is consistent with the requested
> + *    type of modification.
> + *
> + * 3) make sure there are separate "def" and "newDef" copies of
> + *    networkDef if appropriate.
> + *
> + * Returns 0 on success, -1 on error.
> + */
> +int
> +virNetworkConfigChangeSetup(virNetworkObjPtr network, unsigned int flags)
> +{
> +    bool isActive;
> +    int ret = -1;
> +
> +    isActive = virNetworkObjIsActive(network);
> +
> +    if (!isActive && (flags & VIR_NETWORK_UPDATE_AFFECT_LIVE)) {
> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                       _("network is not running"));
> +        goto cleanup;
> +    }
> +
> +    if (flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG) {
> +        if (!network->persistent) {
> +            virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                           _("cannot change persistent config of a "
> +                             "transient network"));
> +            goto cleanup;
> +        }
> +        /* this should already have been done by the driver, but do it
> +         * anyway just in case.
> +         */
> +        if (isActive && (virNetworkObjSetDefTransient(network, false) < 0))
> +            goto cleanup;
> +    }
> +
> +    ret = 0;
> +cleanup:
> +    return ret;
> +}
> +
>  void virNetworkRemoveInactive(virNetworkObjListPtr nets,
>                                const virNetworkObjPtr net)
>  {
> @@ -1791,7 +1989,7 @@ int virNetworkSaveConfig(const char *configDir,
>      int ret = -1;
>      char *xml;
>  
> -    if (!(xml = virNetworkDefFormat(def, 0)))
> +    if (!(xml = virNetworkDefFormat(def, VIR_NETWORK_XML_INACTIVE)))
>          goto cleanup;
>  
>      if (virNetworkSaveXML(configDir, def, xml))
> @@ -1804,6 +2002,24 @@ cleanup:
>  }
>  
>  
> +int virNetworkSaveStatus(const char *statusDir,
> +                         virNetworkObjPtr network)
> +{
> +    int ret = -1;
> +    char *xml;
> +
> +    if (!(xml = virNetworkDefFormat(network->def, 0)))
> +        goto cleanup;
> +
> +    if (virNetworkSaveXML(statusDir, network->def, xml))
> +        goto cleanup;
> +
> +    ret = 0;
> +cleanup:
> +    VIR_FREE(xml);
> +    return ret;
> +}
> +
>  virNetworkObjPtr virNetworkLoadConfig(virNetworkObjListPtr nets,
>                                        const char *configDir,
>                                        const char *autostartDir,
> @@ -1844,7 +2060,7 @@ virNetworkObjPtr virNetworkLoadConfig(virNetworkObjListPtr nets,
>              goto error;
>      }
>  
> -    if (!(net = virNetworkAssignDef(nets, def)))
> +    if (!(net = virNetworkAssignDef(nets, def, false)))
>          goto error;
>  
>      net->autostart = autostart;
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index 5dbf50d..0d37a8b 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -247,7 +247,18 @@ void virNetworkObjFree(virNetworkObjPtr net);
>  void virNetworkObjListFree(virNetworkObjListPtr vms);
>  
>  virNetworkObjPtr virNetworkAssignDef(virNetworkObjListPtr nets,
> -                                     const virNetworkDefPtr def);
> +                                     const virNetworkDefPtr def,
> +                                     bool live);
> +int virNetworkObjAssignDef(virNetworkObjPtr network,
> +                           const virNetworkDefPtr def,
> +                           bool live);
> +int virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live);
> +virNetworkDefPtr virNetworkObjGetPersistentDef(virNetworkObjPtr network);
> +int virNetworkObjReplacePersistentDef(virNetworkObjPtr network,
> +                                      virNetworkDefPtr def);
> +virNetworkDefPtr virNetworkDefCopy(virNetworkDefPtr def, unsigned int flags);
> +int virNetworkConfigChangeSetup(virNetworkObjPtr dom, unsigned int flags);
> +
>  void virNetworkRemoveInactive(virNetworkObjListPtr nets,
>                                const virNetworkObjPtr net);
>  
> @@ -283,6 +294,9 @@ int virNetworkSaveXML(const char *configDir,
>  int virNetworkSaveConfig(const char *configDir,
>                           virNetworkDefPtr def);
>  
> +int virNetworkSaveStatus(const char *statusDir,
> +                         virNetworkObjPtr net) ATTRIBUTE_RETURN_CHECK;
> +
>  virNetworkObjPtr virNetworkLoadConfig(virNetworkObjListPtr nets,
>                                        const char *configDir,
>                                        const char *autostartDir,
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index e8f3fa5..39e06e4 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -839,6 +839,8 @@ virNetDevVPortTypeToString;
>  # network_conf.h
>  virNetworkAssignDef;
>  virNetworkConfigFile;
> +virNetworkConfigChangeSetup;
> +virNetworkDefCopy;
>  virNetworkDefFormat;
>  virNetworkDefFree;
>  virNetworkDefGetIpByIndex;
> @@ -852,15 +854,21 @@ virNetworkIpDefNetmask;
>  virNetworkIpDefPrefix;
>  virNetworkList;
>  virNetworkLoadAllConfigs;
> +virNetworkObjAssignDef;
> +virNetworkObjFree;
> +virNetworkObjGetPersistentDef;
>  virNetworkObjIsDuplicate;
>  virNetworkObjListFree;
>  virNetworkObjLock;
> +virNetworkObjReplacePersistentDef;
> +virNetworkObjSetDefTransient;
>  virNetworkObjUnlock;
> -virNetworkObjFree;
>  virNetworkRemoveInactive;
>  virNetworkSaveConfig;
> +virNetworkSaveStatus;
>  virNetworkSetBridgeMacAddr;
>  virNetworkSetBridgeName;
> +virNetworkSetDefTransient;
>  virPortGroupFindByName;
>  
>  
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 4faad5d..8dbb050 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -2023,6 +2023,9 @@ networkStartNetwork(struct network_driver *driver,
>          return -1;
>      }
>  
> +    if (virNetworkObjSetDefTransient(network, true) < 0)
> +        return -1;
> +
>      switch (network->def->forwardType) {
>  
>      case VIR_NETWORK_FORWARD_NONE:
> @@ -2046,7 +2049,7 @@ networkStartNetwork(struct network_driver *driver,
>      /* Persist the live configuration now that anything autogenerated
>       * is setup.
>       */
> -    if ((ret = virNetworkSaveConfig(NETWORK_STATE_DIR, network->def)) < 0) {
> +    if ((ret = virNetworkSaveStatus(NETWORK_STATE_DIR, network)) < 0) {
>          goto error;
>      }
>  
> @@ -2389,8 +2392,10 @@ static virNetworkPtr networkCreate(virConnectPtr conn, const char *xml) {
>      if (networkValidate(def) < 0)
>         goto cleanup;
>  
> -    if (!(network = virNetworkAssignDef(&driver->networks,
> -                                        def)))
> +    /* NB: "live" is false because this transient network hasn't yet
> +     * been started
> +     */
> +    if (!(network = virNetworkAssignDef(&driver->networks, def, false)))
>          goto cleanup;
>      def = NULL;
>  
> @@ -2465,8 +2470,7 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) {
>      if (networkValidate(def) < 0)
>         goto cleanup;
>  
> -    if (!(network = virNetworkAssignDef(&driver->networks,
> -                                        def)))
> +    if (!(network = virNetworkAssignDef(&driver->networks, def, false)))
>          goto cleanup;
>      freeDef = false;
>  
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index fbd8ed0..1bd0d61 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -579,7 +579,7 @@ static int testOpenDefault(virConnectPtr conn) {
>  
>      if (!(netdef = virNetworkDefParseString(defaultNetworkXML)))
>          goto error;
> -    if (!(netobj = virNetworkAssignDef(&privconn->networks, netdef))) {
> +    if (!(netobj = virNetworkAssignDef(&privconn->networks, netdef, false))) {
>          virNetworkDefFree(netdef);
>          goto error;
>      }
> @@ -948,8 +948,7 @@ static int testOpenFromFile(virConnectPtr conn,
>              if ((def = virNetworkDefParseNode(xml, networks[i])) == NULL)
>                  goto error;
>          }
> -        if (!(net = virNetworkAssignDef(&privconn->networks,
> -                                        def))) {
> +        if (!(net = virNetworkAssignDef(&privconn->networks, def, false))) {
>              virNetworkDefFree(def);
>              goto error;
>          }
> @@ -3112,7 +3111,7 @@ static virNetworkPtr testNetworkCreate(virConnectPtr conn, const char *xml) {
>      if ((def = virNetworkDefParseString(xml)) == NULL)
>          goto cleanup;
>  
> -    if ((net = virNetworkAssignDef(&privconn->networks, def)) == NULL)
> +    if (!(net = virNetworkAssignDef(&privconn->networks, def, false)))
>          goto cleanup;
>      def = NULL;
>      net->active = 1;
> @@ -3137,7 +3136,7 @@ static virNetworkPtr testNetworkDefine(virConnectPtr conn, const char *xml) {
>      if ((def = virNetworkDefParseString(xml)) == NULL)
>          goto cleanup;
>  
> -    if ((net = virNetworkAssignDef(&privconn->networks, def)) == NULL)
> +    if (!(net = virNetworkAssignDef(&privconn->networks, def, false)))
>          goto cleanup;
>      def = NULL;
>      net->persistent = 1;

  Okay, this incorporate Eric's feedabck as far as I can tell, ACK

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


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