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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list