On Tue, Sep 18, 2012 at 03:38:57AM -0400, Laine Stump wrote:

This patch adds a new public API virNetworkUpdate that will permit updating an existing network configuration without requiring that the network be destroyed/restarted for the changes to take effect. --- include/libvirt/libvirt.h.in | 66 ++++++++++++++++++++++++++++++++++++++++++++ src/driver.h | 8 ++++++ src/libvirt.c | 62 +++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 1 + 4 files changed, 137 insertions(+)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index d9bfb6e..84ac2d0 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2347,6 +2347,72 @@ virNetworkPtr virNetworkDefineXML (virConnectPtr conn, */ int virNetworkUndefine (virNetworkPtr network);

+/** + * virNetworkUpdateCommand: + * + * describes which type of update to perform on a <network> + * definition. + * + */ +typedef enum { + VIR_NETWORK_UPDATE_COMMAND_NONE = 0, /* (invalid) */ + VIR_NETWORK_UPDATE_COMMAND_MODIFY = 2, /* modify an existing element */ + VIR_NETWORK_UPDATE_COMMAND_DELETE = 3, /* delete an existing element */ + VIR_NETWORK_UPDATE_COMMAND_ADD_LAST = 4, /* add an element at end of list */ + VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST = 5, /* add an element at start of list */ +#ifdef VIR_ENUM_SENTINELS + VIR_NETWORK_UPDATE_COMMAND_LAST +#endif +} virNetworkUpdateCommand; + +/** + * virNetworkUpdateSection: + * + * describes which section of a <network> definition the provided + * xml should be applied to. + * + */ +typedef enum { + VIR_NETWORK_SECTION_NONE = 0, /* (invalid) */ + VIR_NETWORK_SECTION_BRIDGE = 1, /* <bridge> */ + VIR_NETWORK_SECTION_DOMAIN = 2, /* <domain> */ + VIR_NETWORK_SECTION_IP = 3, /* <ip> */ + VIR_NETWORK_SECTION_IP_DHCP_HOST = 4, /* <ip>/<dhcp>/<host> */ + VIR_NETWORK_SECTION_IP_DHCP_RANGE = 5, /* <ip>/<dhcp>/<range> */ + VIR_NETWORK_SECTION_FORWARD = 6, /* <forward> */ + VIR_NETWORK_SECTION_FORWARD_INTERFACE = 7, /* <forward>/<interface> */ + VIR_NETWORK_SECTION_FORWARD_PF = 8, /* <forward>/<pf> */ + VIR_NETWORK_SECTION_PORTGROUP = 9, /* <portgroup> */ + VIR_NETWORK_SECTION_DNS_HOST = 10, /* <dns>/<host> */ + VIR_NETWORK_SECTION_DNS_TXT = 11, /* <dns>/<txt> */ + VIR_NETWORK_SECTION_DNS_SRV = 12, /* <dns>/<srv> */ +#ifdef VIR_ENUM_SENTINELS + VIR_NETWORK_SECTION_LAST +#endif +} virNetworkUpdateSection; + +/** + * virNetworkUpdateFlags: + * + * Flags to control options for virNetworkUpdate() + */ +typedef enum { + VIR_NETWORK_UPDATE_AFFECT_CURRENT = 0, /* affect live if network is active, + config if it's not active */ + VIR_NETWORK_UPDATE_AFFECT_LIVE = 1 << 0, /* affect live state of network only */ + VIR_NETWORK_UPDATE_AFFECT_CONFIG = 1 << 1, /* affect persistent config only */ + } virNetworkUpdateFlags; + +/* + * Update an existing network definition + */ +int virNetworkUpdate(virNetworkPtr network, + unsigned int command, /* virNetworkUpdateCommand */ + unsigned int section, /* virNetworkUpdateSection */ + int parentIndex, + const char *xml, + unsigned int flags); + /* * Activate persistent network */ diff --git a/src/driver.h b/src/driver.h index 063bbbf..bdcaa01 100644 --- a/src/driver.h +++ b/src/driver.h @@ -1120,6 +1120,13 @@ typedef virNetworkPtr typedef int (*virDrvNetworkUndefine) (virNetworkPtr network); typedef int + (*virDrvNetworkUpdate) (virNetworkPtr network, + unsigned int command, /* virNetworkUpdateCommand */ + unsigned int section, /* virNetworkUpdateSection */ + int parentIndex, + const char *xml, + unsigned int flags); +typedef int (*virDrvNetworkCreate) (virNetworkPtr network); typedef int (*virDrvNetworkDestroy) (virNetworkPtr network); @@ -1169,6 +1176,7 @@ struct _virNetworkDriver { virDrvNetworkCreateXML networkCreateXML; virDrvNetworkDefineXML networkDefineXML; virDrvNetworkUndefine networkUndefine; + virDrvNetworkUpdate networkUpdate; virDrvNetworkCreate networkCreate; virDrvNetworkDestroy networkDestroy; virDrvNetworkGetXMLDesc networkGetXMLDesc; diff --git a/src/libvirt.c b/src/libvirt.c index 5438fe4..24d5729 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -10407,6 +10407,68 @@ error: }

/** + * virNetworkUpdate: + * @network: pointer to a defined network + * @section: which section of the network to update + * (see virNetworkUpdateSection for descriptions) + * @command: what action to perform (add/delete/modify) + * (see virNetworkUpdateCommand for descriptions) + * @parentIndex: which parent element, if there are multiple parents + * of the same type (e.g. which <ip> element when modifying + * a <dhcp>/<host> element), or "-1" for "don't care" or + * "automatically find appropriate one". + * @xml: the XML description for the network, preferably in UTF-8 + * @flags: bitwise OR of virNetworkUpdateFlags. + * + * Update the definition of an existing network, either its live + * running state, its persistent configuration, or both. + * + * Returns 0 in case of success, -1 in case of error + */ +int +virNetworkUpdate(virNetworkPtr network, + unsigned int command, /* virNetworkUpdateCommand */ + unsigned int section, /* virNetworkUpdateSection */ + int parentIndex, + const char *xml, + unsigned int flags) +{ + virConnectPtr conn; + VIR_DEBUG("network=%p, section=%d, parentIndex=%d, xml=%s, flags=0x%x", + network, section, parentIndex, xml, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECTED_NETWORK(network)) { + virLibNetworkError(VIR_ERR_INVALID_NETWORK, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + conn = network->conn; + if (conn->flags & VIR_CONNECT_RO) { + virLibNetworkError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + + virCheckNonNullArgGoto(xml, error); + + if (conn->networkDriver && conn->networkDriver->networkUpdate) { + int ret; + ret = conn->networkDriver->networkUpdate(network, section, command, + parentIndex, xml, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(network->conn); + return -1; +} + +/** * virNetworkCreate: * @network: pointer to a defined network * diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index d965c7f..2c924d5 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -563,6 +563,7 @@ LIBVIRT_0.10.2 { virConnectListAllSecrets; virConnectListAllStoragePools; virDomainBlockCommit; + virNetworkUpdate; virNodeGetMemoryParameters; virNodeSetMemoryParameters; virStoragePoolListAllVolumes;

Okay, ACK, the separation of the parameters makes sense, this won't solve Dan's problem with making a simple binding but we don't have a solution for that, 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/

On Tue, Sep 18, 2012 at 03:38:58AM -0400, Laine Stump wrote:

This is very short, because almost everything is autogenerated. All that's needed are:

* src/remote/remote_driver.c: add pointer to autogenerated remoteNetworkUpdate to the function table for the remote network driver.

* src/remote/remote_protocol.x: add the "args" struct and add one more item to the remote_procedure enum for this function.

* src/remote_protocol-struct: update to match remote_protocol.x --- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 12 +++++++++++- src/remote_protocol-structs | 9 +++++++++ 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index b6edf38..2f04a32 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -6076,6 +6076,7 @@ static virNetworkDriver network_driver = { .networkCreateXML = remoteNetworkCreateXML, /* 0.3.0 */ .networkDefineXML = remoteNetworkDefineXML, /* 0.3.0 */ .networkUndefine = remoteNetworkUndefine, /* 0.3.0 */ + .networkUpdate = remoteNetworkUpdate, /* 0.10.2 */ .networkCreate = remoteNetworkCreate, /* 0.3.0 */ .networkDestroy = remoteNetworkDestroy, /* 0.3.0 */ .networkGetXMLDesc = remoteNetworkGetXMLDesc, /* 0.3.0 */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 9481f15..4205875 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1336,6 +1336,15 @@ struct remote_network_undefine_args { remote_nonnull_network net; };

+struct remote_network_update_args { + remote_nonnull_network net; + unsigned int command; + unsigned int section; + int parentIndex; + remote_nonnull_string xml; + unsigned int flags; +}; + struct remote_network_create_args { remote_nonnull_network net; }; @@ -2997,8 +3006,9 @@ enum remote_procedure { REMOTE_PROC_CONNECT_LIST_ALL_SECRETS = 287, /* skipgen skipgen priority:high */ REMOTE_PROC_NODE_SET_MEMORY_PARAMETERS = 288, /* autogen autogen */ REMOTE_PROC_NODE_GET_MEMORY_PARAMETERS = 289, /* skipgen skipgen */ - REMOTE_PROC_DOMAIN_BLOCK_COMMIT = 290 /* autogen autogen */ + REMOTE_PROC_DOMAIN_BLOCK_COMMIT = 290, /* autogen autogen */

+ REMOTE_PROC_NETWORK_UPDATE = 291 /* autogen autogen priority:high */

/* * Notice how the entries are grouped in sets of 10 ? diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 8b0ae1f..0a9beff 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -977,6 +977,14 @@ struct remote_network_define_xml_ret { struct remote_network_undefine_args { remote_nonnull_network net; }; +struct remote_network_update_args { + remote_nonnull_network net; + u_int command; + u_int section; + int index; + remote_nonnull_string xml; + u_int flags; +}; struct remote_network_create_args { remote_nonnull_network net; }; @@ -2406,4 +2414,5 @@ enum remote_procedure { REMOTE_PROC_NODE_SET_MEMORY_PARAMETERS = 288, REMOTE_PROC_NODE_GET_MEMORY_PARAMETERS = 289, REMOTE_PROC_DOMAIN_BLOCK_COMMIT = 290, + REMOTE_PROC_NETWORK_UPDATE = 291, };

ACK, direct from 1/9 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/

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/

On Tue, Sep 18, 2012 at 03:39:00AM -0400, Laine Stump wrote:

virNetworkObjUpdate takes care of all virNetworkUpdate-related changes to the data stored in the in-memory virNetworkObj list. It should be called by network drivers that use this in-memory list.

virNetworkObjUpdate *does not* take care of updating any disk-based copies of the config, nor does it perform any other operations necessary to have the new config data take effect (e.g. it won't re-write dnsmasq host files, nor will it send a SIGHUP to dnsmasq) - those things should all be taken care of in the network driver function that calls virNetworkObjUpdate (assuming that it returns success). --- src/conf/network_conf.c | 308 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/network_conf.h | 8 ++ src/libvirt_private.syms | 1 + 3 files changed, 317 insertions(+)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index a48eb9e..2a65f1d 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2251,6 +2251,314 @@ void virNetworkSetBridgeMacAddr(virNetworkDefPtr def) } }

+/* NetworkObj backend of the virNetworkUpdate API */ + +static void +virNetworkDefUpdateNoSupport(virNetworkDefPtr def, const char *section) +{ + virReportError(VIR_ERR_NO_SUPPORT, + _("can't update '%s' section of network '%s'"), + section, def->name); +} + +#if 0 +static int +virNetworkDefUpdateCheckElementName(virNetworkDefPtr def, + xmlNodePtr node, + const char *section) +{ + if (!xmlStrEqual(node->name, BAD_CAST section)) { + virReportError(VIR_ERR_XML_ERROR, + _("unexpected element <%s>, expecting <%s>, " + "while updating network '%s'"), + node->name, section, def->name); + return -1; + } + return 0; +} +#endif + +static int +virNetworkDefUpdateBridge(virNetworkDefPtr def, + unsigned int command ATTRIBUTE_UNUSED, + int parentIndex ATTRIBUTE_UNUSED, + xmlXPathContextPtr ctxt ATTRIBUTE_UNUSED, + /* virNetworkUpdateFlags */ + unsigned int fflags ATTRIBUTE_UNUSED) +{ + virNetworkDefUpdateNoSupport(def, "bridge"); + return -1; +} + +static int +virNetworkDefUpdateDomain(virNetworkDefPtr def, + unsigned int command ATTRIBUTE_UNUSED, + int parentIndex ATTRIBUTE_UNUSED, + xmlXPathContextPtr ctxt ATTRIBUTE_UNUSED, + /* virNetworkUpdateFlags */ + unsigned int fflags ATTRIBUTE_UNUSED) +{ + virNetworkDefUpdateNoSupport(def, "domain"); + return -1; +} + +static int +virNetworkDefUpdateIP(virNetworkDefPtr def, + unsigned int command ATTRIBUTE_UNUSED, + int parentIndex ATTRIBUTE_UNUSED, + xmlXPathContextPtr ctxt ATTRIBUTE_UNUSED, + /* virNetworkUpdateFlags */ + unsigned int fflags ATTRIBUTE_UNUSED) +{ + virNetworkDefUpdateNoSupport(def, "ip"); + return -1; +} + +static int +virNetworkDefUpdateIPDHCPHost(virNetworkDefPtr def, + unsigned int command ATTRIBUTE_UNUSED, + int parentIndex ATTRIBUTE_UNUSED, + xmlXPathContextPtr ctxt ATTRIBUTE_UNUSED, + /* virNetworkUpdateFlags */ + unsigned int fflags ATTRIBUTE_UNUSED) +{ + virNetworkDefUpdateNoSupport(def, "ip dhcp host"); + return -1; +} + +static int +virNetworkDefUpdateIPDHCPRange(virNetworkDefPtr def, + unsigned int command ATTRIBUTE_UNUSED, + int parentIndex ATTRIBUTE_UNUSED, + xmlXPathContextPtr ctxt ATTRIBUTE_UNUSED, + /* virNetworkUpdateFlags */ + unsigned int fflags ATTRIBUTE_UNUSED) +{ + virNetworkDefUpdateNoSupport(def, "ip dhcp range"); + return -1; +} + +static int +virNetworkDefUpdateForward(virNetworkDefPtr def, + unsigned int command ATTRIBUTE_UNUSED, + int parentIndex ATTRIBUTE_UNUSED, + xmlXPathContextPtr ctxt ATTRIBUTE_UNUSED, + /* virNetworkUpdateFlags */ + unsigned int fflags ATTRIBUTE_UNUSED) +{ + virNetworkDefUpdateNoSupport(def, "forward"); + return -1; +} + +static int +virNetworkDefUpdateForwardInterface(virNetworkDefPtr def, + unsigned int command ATTRIBUTE_UNUSED, + int parentIndex ATTRIBUTE_UNUSED, + xmlXPathContextPtr ctxt ATTRIBUTE_UNUSED, + /* virNetworkUpdateFlags */ + unsigned int fflags ATTRIBUTE_UNUSED) +{ + virNetworkDefUpdateNoSupport(def, "forward interface"); + return -1; +} + +static int +virNetworkDefUpdateForwardPF(virNetworkDefPtr def, + unsigned int command ATTRIBUTE_UNUSED, + int parentIndex ATTRIBUTE_UNUSED, + xmlXPathContextPtr ctxt ATTRIBUTE_UNUSED, + /* virNetworkUpdateFlags */ + unsigned int fflags ATTRIBUTE_UNUSED) +{ + virNetworkDefUpdateNoSupport(def, "forward pf"); + return -1; +} + +static int +virNetworkDefUpdatePortgroup(virNetworkDefPtr def, + unsigned int command ATTRIBUTE_UNUSED, + int parentIndex ATTRIBUTE_UNUSED, + xmlXPathContextPtr ctxt ATTRIBUTE_UNUSED, + /* virNetworkUpdateFlags */ + unsigned int fflags ATTRIBUTE_UNUSED) +{ + virNetworkDefUpdateNoSupport(def, "portgroup"); + return -1; +} + +static int +virNetworkDefUpdateDNSHost(virNetworkDefPtr def, + unsigned int command ATTRIBUTE_UNUSED, + int parentIndex ATTRIBUTE_UNUSED, + xmlXPathContextPtr ctxt ATTRIBUTE_UNUSED, + /* virNetworkUpdateFlags */ + unsigned int fflags ATTRIBUTE_UNUSED) +{ + virNetworkDefUpdateNoSupport(def, "dns host"); + return -1; +} + +static int +virNetworkDefUpdateDNSTxt(virNetworkDefPtr def, + unsigned int command ATTRIBUTE_UNUSED, + int parentIndex ATTRIBUTE_UNUSED, + xmlXPathContextPtr ctxt ATTRIBUTE_UNUSED, + /* virNetworkUpdateFlags */ + unsigned int fflags ATTRIBUTE_UNUSED) +{ + virNetworkDefUpdateNoSupport(def, "dns txt"); + return -1; +} + +static int +virNetworkDefUpdateDNSSrv(virNetworkDefPtr def, + unsigned int command ATTRIBUTE_UNUSED, + int parentIndex ATTRIBUTE_UNUSED, + xmlXPathContextPtr ctxt ATTRIBUTE_UNUSED, + /* virNetworkUpdateFlags */ + unsigned int fflags ATTRIBUTE_UNUSED) +{ + virNetworkDefUpdateNoSupport(def, "dns txt"); + return -1; +} + +static int +virNetworkDefUpdateSection(virNetworkDefPtr def, + unsigned int command, /* virNetworkUpdateCommand */ + unsigned int section, /* virNetworkUpdateSection */ + int parentIndex, + const char *xml, + unsigned int flags) /* virNetworkUpdateFlags */ +{ + int ret = -1; + xmlDocPtr doc; + xmlXPathContextPtr ctxt = NULL; + + if (!(doc = virXMLParseStringCtxt(xml, _("network_update_xml"), &ctxt))) + goto cleanup; + + switch (section) { + case VIR_NETWORK_SECTION_BRIDGE: + ret = virNetworkDefUpdateBridge(def, command, parentIndex, ctxt, flags); + break; + + case VIR_NETWORK_SECTION_DOMAIN: + ret = virNetworkDefUpdateDomain(def, command, parentIndex, ctxt, flags); + break; + case VIR_NETWORK_SECTION_IP: + ret = virNetworkDefUpdateIP(def, command, parentIndex, ctxt, flags); + break; + case VIR_NETWORK_SECTION_IP_DHCP_HOST: + ret = virNetworkDefUpdateIPDHCPHost(def, command, + parentIndex, ctxt, flags); + break; + case VIR_NETWORK_SECTION_IP_DHCP_RANGE: + ret = virNetworkDefUpdateIPDHCPRange(def, command, + parentIndex, ctxt, flags); + break; + case VIR_NETWORK_SECTION_FORWARD: + ret = virNetworkDefUpdateForward(def, command, + parentIndex, ctxt, flags); + break; + case VIR_NETWORK_SECTION_FORWARD_INTERFACE: + ret = virNetworkDefUpdateForwardInterface(def, command, + parentIndex, ctxt, flags); + break; + case VIR_NETWORK_SECTION_FORWARD_PF: + ret = virNetworkDefUpdateForwardPF(def, command, + parentIndex, ctxt, flags); + break; + case VIR_NETWORK_SECTION_PORTGROUP: + ret = virNetworkDefUpdatePortgroup(def, command, + parentIndex, ctxt, flags); + break; + case VIR_NETWORK_SECTION_DNS_HOST: + ret = virNetworkDefUpdateDNSHost(def, command, + parentIndex, ctxt, flags); + break; + case VIR_NETWORK_SECTION_DNS_TXT: + ret = virNetworkDefUpdateDNSTxt(def, command, parentIndex, ctxt, flags); + break; + case VIR_NETWORK_SECTION_DNS_SRV: + ret = virNetworkDefUpdateDNSSrv(def, command, parentIndex, ctxt, flags); + break; + default: + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("can't update unrecognized section of network")); + break; + } + +cleanup: + xmlFreeDoc(doc); + xmlXPathFreeContext(ctxt); + return ret; +} + +/* + * virNetworkObjUpdate: + * + * Apply the supplied update to the given virNetworkObj. Except for + * @network pointing to an actual network object rather than the + * opaque virNetworkPtr, parameters are identical to the public API + * virNetworkUpdate. + * + * The original virNetworkDefs are copied, and all modifications made + * to these copies. The originals are replaced with the copies only + * after success has been guaranteed. + * + * Returns: -1 on error, 0 on success. + */ +int +virNetworkObjUpdate(virNetworkObjPtr network, + unsigned int command, /* virNetworkUpdateCommand */ + unsigned int section, /* virNetworkUpdateSection */ + int parentIndex, + const char *xml, + unsigned int flags) /* virNetworkUpdateFlags */ +{ + int ret = -1; + virNetworkDefPtr def = NULL; + + /* normalize config data, and check for common invalid requests. */ + if (virNetworkConfigChangeSetup(network, flags) < 0) + goto cleanup; + + if (flags & VIR_NETWORK_UPDATE_AFFECT_LIVE) { + /* work on a copy of the def */ + if (!(def = virNetworkDefCopy(network->def, 0))) + goto cleanup; + if (virNetworkDefUpdateSection(def, command, section, + parentIndex, xml, flags) < 0) { + goto cleanup; + } + /* successfully modified copy, now replace original */ + virNetworkDefFree(network->def); + network->def = def; + def = NULL; + } + + if (flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG) { + /* work on a copy of the def */ + if (!(def = virNetworkDefCopy(virNetworkObjGetPersistentDef(network), + VIR_NETWORK_XML_INACTIVE))) { + goto cleanup; + } + if (virNetworkDefUpdateSection(def, command, section, + parentIndex, xml, flags) < 0) { + goto cleanup; + } + /* successfully modified copy, now replace original */ + if (virNetworkObjReplacePersistentDef(network, def) < 0) + goto cleanup; + def = NULL; + } + + ret = 0; +cleanup: + virNetworkDefFree(def); + return ret; +} + /* * virNetworkObjIsDuplicate: * @doms : virNetworkObjListPtr to search diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 0d37a8b..c8ed2ea 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -326,6 +326,14 @@ int virNetworkSetBridgeName(const virNetworkObjListPtr nets,

void virNetworkSetBridgeMacAddr(virNetworkDefPtr def);

+int +virNetworkObjUpdate(virNetworkObjPtr obj, + unsigned int command, /* virNetworkUpdateCommand */ + unsigned int section, /* virNetworkUpdateSection */ + int parentIndex, + const char *xml, + unsigned int flags); /* virNetworkUpdateFlags */ + int virNetworkObjIsDuplicate(virNetworkObjListPtr doms, virNetworkDefPtr def, unsigned int check_active); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 39e06e4..ec2e544 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -863,6 +863,7 @@ virNetworkObjLock; virNetworkObjReplacePersistentDef; virNetworkObjSetDefTransient; virNetworkObjUnlock; +virNetworkObjUpdate; virNetworkRemoveInactive; virNetworkSaveConfig; virNetworkSaveStatus;

ACK, not functional but put the framework in place, 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/

On Tue, Sep 18, 2012 at 03:39:01AM -0400, Laine Stump wrote:

This patch splits the starting of dnsmasq and radvd into multiple files, and adds new networkRefreshXX() and networkRestartXX() functions for each. These new functions are currently commented out because they won't be used until the next commit, and the compile options require all static functions to be used.

networkRefreshXX() - rewrites any file-based config for dnsmasq/radvd, and sends SIGHUP to the process to make it reread its config. If the program isn't already running, it's just started.

networkRestartXX() - kills the given program, waits for it to exit (see the comments in the function networkKillDaemon()), then calls networkStartXX().

This commit is here mostly as a checkpoint to verify no change in functional behavior after refactoring networkStartXX() functions to fit in with these new functions. --- src/network/bridge_driver.c | 326 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 282 insertions(+), 44 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 8dbb050..5f7f31e 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -468,6 +468,68 @@ networkShutdown(void) { }

+#if 0 +/* currently unused, so it causes a build error unless we #if it out */ +/* networkKillDaemon: + * + * kill the specified pid/name, and wait a bit to make sure it's dead. + */ +static int +networkKillDaemon(pid_t pid, const char *daemonName, const char *networkName) +{ + int ii, ret = -1; + const char *signame = "TERM"; + + /* send SIGTERM, then wait up to 3 seconds for the process to + * disappear, send SIGKILL, then wait for up to another 2 + * seconds. If that fails, log a warning and continue, hoping + * for the best. + */ + for (ii = 0; ii < 25; ii++) { + int signum = 0; + if (ii == 0) + signum = SIGTERM; + else if (ii == 15) { + signum = SIGKILL; + signame = "KILL"; + } + if (kill(pid, signum) < 0) { + if (errno == ESRCH) { + ret = 0; + } else { + char ebuf[1024]; + VIR_WARN("Failed to terminate %s process %d " + "for network '%s' with SIG%s: %s", + daemonName, pid, networkName, signame, + virStrerror(errno, ebuf, sizeof(ebuf))); + } + goto cleanup; + } + /* NB: since networks have no reference count like + * domains, there is no safe way to unlock the network + * object temporarily, and so we can't follow the + * procedure used by the qemu driver of 1) unlock driver + * 2) sleep, 3) add ref to object 4) unlock object, 5) + * re-lock driver, 6) re-lock object. We may need to add + * that functionality eventually, but for now this + * function is rarely used and, at worst, leaving the + * network driver locked during this loop of sleeps will + * have the effect of holding up any other thread trying + * to make modifications to a network for up to 5 seconds; + * since modifications to networks are much less common + * than modifications to domains, this seems a reasonable + * tradeoff in exchange for less code disruption. + */ + usleep(20 * 1000); + } + VIR_WARN("Timed out waiting after SIG%s to %s process %d " + "(network '%s')", + signame, daemonName, pid, networkName); +cleanup: + return ret; +} +#endif /* #if 0 */ + static int networkBuildDnsmasqHostsfile(dnsmasqContext *dctx, virNetworkIpDefPtr ipdef, @@ -777,6 +839,12 @@ networkStartDhcpDaemon(virNetworkObjPtr network) int ret = -1; dnsmasqContext *dctx = NULL;

+ if (!virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, 0)) { + /* no IPv6 addresses, so we don't need to run radvd */ + ret = 0; + goto cleanup; + } + if (virFileMakePath(NETWORK_PID_DIR) < 0) { virReportSystemError(errno, _("cannot create directory %s"), @@ -840,50 +908,87 @@ cleanup: return ret; }

+#if 0 +/* currently unused, so they cause a build error unless we #if them out */ +/* networkRefreshDhcpDaemon: + * Update dnsmasq config files, then send a SIGHUP so that it rereads + * them. + * + * Returns 0 on success, -1 on failure. + */ static int -networkStartRadvd(virNetworkObjPtr network) +networkRefreshDhcpDaemon(virNetworkObjPtr network) { - char *pidfile = NULL; - char *radvdpidbase = NULL; - virBuffer configbuf = VIR_BUFFER_INITIALIZER;; - char *configstr = NULL; - char *configfile = NULL; - virCommandPtr cmd = NULL; int ret = -1, ii; virNetworkIpDefPtr ipdef; + dnsmasqContext *dctx = NULL;

- network->radvdPid = -1; + /* if there's no running dnsmasq, just start it */ + if (network->dnsmasqPid <= 0 || (kill(network->dnsmasqPid, 0) < 0)) + return networkStartDhcpDaemon(network);

- if (!virFileIsExecutable(RADVD)) { - virReportSystemError(errno, - _("Cannot find %s - " - "Possibly the package isn't installed"), - RADVD); - goto cleanup; + /* Look for first IPv4 address that has dhcp defined. */ + /* We support dhcp config on 1 IPv4 interface only. */ + for (ii = 0; + (ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET, ii)); + ii++) { + if (ipdef->nranges || ipdef->nhosts) + break; } + /* If no IPv4 addresses had dhcp info, pick the first (if there were any). */ + if (!ipdef) + ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET, 0);

- if (virFileMakePath(NETWORK_PID_DIR) < 0) { - virReportSystemError(errno, - _("cannot create directory %s"), - NETWORK_PID_DIR); - goto cleanup; - } - if (virFileMakePath(RADVD_STATE_DIR) < 0) { - virReportSystemError(errno, - _("cannot create directory %s"), - RADVD_STATE_DIR); - goto cleanup; + if (!ipdef) { + /* no <ip> elements, so nothing to do */ + return 0; }

- /* construct pidfile name */ - if (!(radvdpidbase = networkRadvdPidfileBasename(network->def->name))) { - virReportOOMError(); + if (!(dctx = dnsmasqContextNew(network->def->name, DNSMASQ_STATE_DIR))) goto cleanup; - } - if (!(pidfile = virPidFileBuildPath(NETWORK_PID_DIR, radvdpidbase))) { - virReportOOMError(); + + if (networkBuildDnsmasqHostsfile(dctx, ipdef, network->def->dns) < 0) + goto cleanup; + + if ((ret = dnsmasqSave(dctx)) < 0) goto cleanup; + + ret = kill(network->dnsmasqPid, SIGHUP); +cleanup: + dnsmasqContextFree(dctx); + return ret; +} + +/* networkRestartDhcpDaemon: + * + * kill and restart dnsmasq, in order to update any config that is on + * the dnsmasq commandline (and any placed in separate config files). + * + * Returns 0 on success, -1 on failure. + */ +static int +networkRestartDhcpDaemon(virNetworkObjPtr network) +{ + /* if there is a running dnsmasq, kill it */ + if (network->dnsmasqPid > 0) { + networkKillDaemon(network->dnsmasqPid, "dnsmasq", + network->def->name); + network->dnsmasqPid = -1; } + /* now start dnsmasq if it should be started */ + return networkStartDhcpDaemon(network); +} +#endif /* #if 0 */ + +static int +networkRadvdConfContents(virNetworkObjPtr network, char **configstr) +{ + virBuffer configbuf = VIR_BUFFER_INITIALIZER;; + int ret = -1, ii; + virNetworkIpDefPtr ipdef; + bool v6present = false; + + *configstr = NULL;

/* create radvd config file appropriate for this network */ virBufferAsprintf(&configbuf, "interface %s\n" @@ -893,12 +998,15 @@ networkStartRadvd(virNetworkObjPtr network) " AdvOtherConfigFlag off;\n" "\n", network->def->bridge); + + /* add a section for each IPv6 address in the config */ for (ii = 0; (ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET6, ii)); ii++) { int prefix; char *netaddr;

+ v6present = true; prefix = virNetworkIpDefPrefix(ipdef); if (prefix < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -919,30 +1027,118 @@ networkStartRadvd(virNetworkObjPtr network) VIR_FREE(netaddr); }

- virBufferAddLit(&configbuf, "};\n"); + /* only create the string if we found at least one IPv6 address */ + if (v6present) { + virBufferAddLit(&configbuf, "};\n");

- if (virBufferError(&configbuf)) { - virReportOOMError(); - goto cleanup; + if (virBufferError(&configbuf)) { + virReportOOMError(); + goto cleanup; + } + if (!(*configstr = virBufferContentAndReset(&configbuf))) { + virReportOOMError(); + goto cleanup; + } } - if (!(configstr = virBufferContentAndReset(&configbuf))) { - virReportOOMError(); + + ret = 0; +cleanup: + virBufferFreeAndReset(&configbuf); + return ret; +} + +/* write file and return it's name (which must be freed by caller) */ +static int +networkRadvdConfWrite(virNetworkObjPtr network, char **configFile) +{ + int ret = -1; + char *configStr = NULL; + char *myConfigFile = NULL; + + if (!configFile) + configFile = &myConfigFile; + + *configFile = NULL; + + if (networkRadvdConfContents(network, &configStr) < 0) + goto cleanup; + + if (!configStr) { + ret = 0; goto cleanup; }

/* construct the filename */ - if (!(configfile = networkRadvdConfigFileName(network->def->name))) { + if (!(*configFile = networkRadvdConfigFileName(network->def->name))) { virReportOOMError(); goto cleanup; } /* write the file */ - if (virFileWriteStr(configfile, configstr, 0600) < 0) { + if (virFileWriteStr(*configFile, configStr, 0600) < 0) { virReportSystemError(errno, _("couldn't write radvd config file '%s'"), - configfile); + *configFile); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FREE(configStr); + VIR_FREE(myConfigFile); + return ret; +} + +static int +networkStartRadvd(virNetworkObjPtr network) +{ + char *pidfile = NULL; + char *radvdpidbase = NULL; + char *configfile = NULL; + virCommandPtr cmd = NULL; + int ret = -1; + + network->radvdPid = -1; + + if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0)) { + /* no IPv6 addresses, so we don't need to run radvd */ + ret = 0; + goto cleanup; + } + + if (!virFileIsExecutable(RADVD)) { + virReportSystemError(errno, + _("Cannot find %s - " + "Possibly the package isn't installed"), + RADVD); goto cleanup; }

+ if (virFileMakePath(NETWORK_PID_DIR) < 0) { + virReportSystemError(errno, + _("cannot create directory %s"), + NETWORK_PID_DIR); + goto cleanup; + } + if (virFileMakePath(RADVD_STATE_DIR) < 0) { + virReportSystemError(errno, + _("cannot create directory %s"), + RADVD_STATE_DIR); + goto cleanup; + } + + /* construct pidfile name */ + if (!(radvdpidbase = networkRadvdPidfileBasename(network->def->name))) { + virReportOOMError(); + goto cleanup; + } + if (!(pidfile = virPidFileBuildPath(NETWORK_PID_DIR, radvdpidbase))) { + virReportOOMError(); + goto cleanup; + } + + if (networkRadvdConfWrite(network, &configfile) < 0) + goto cleanup; + /* prevent radvd from daemonizing itself with "--debug 1", and use * a dummy pidfile name - virCommand will create the pidfile we * want to use (this is necessary because radvd's internal @@ -963,21 +1159,63 @@ networkStartRadvd(virNetworkObjPtr network) if (virCommandRun(cmd, NULL) < 0) goto cleanup;

- if (virPidFileRead(NETWORK_PID_DIR, radvdpidbase, - &network->radvdPid) < 0) + if (virPidFileRead(NETWORK_PID_DIR, radvdpidbase, &network->radvdPid) < 0) goto cleanup;

ret = 0; cleanup: virCommandFree(cmd); VIR_FREE(configfile); - VIR_FREE(configstr); - virBufferFreeAndReset(&configbuf); VIR_FREE(radvdpidbase); VIR_FREE(pidfile); return ret; }

+#if 0 +/* currently unused, so they cause a build error unless we #if them out */ +static int +networkRefreshRadvd(virNetworkObjPtr network) +{ + /* if there's no running radvd, just start it */ + if (network->radvdPid <= 0 || (kill(network->radvdPid, 0) < 0)) + return networkStartRadvd(network); + + if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0)) { + /* no IPv6 addresses, so we don't need to run radvd */ + return 0; + } + + if (networkRadvdConfWrite(network, NULL) < 0) + return -1; + + return kill(network->radvdPid, SIGHUP); +} + +static int +networkRestartRadvd(virNetworkObjPtr network) +{ + char *radvdpidbase; + + /* if there is a running radvd, kill it */ + if (network->radvdPid > 0) { + /* essentially ignore errors from the following two functions, + * since there's really no better recovery to be done than to + * just push ahead (and that may be exactly what's needed). + */ + if ((networkKillDaemon(network->dnsmasqPid, "radvd", + network->def->name) >= 0) && + ((radvdpidbase = networkRadvdPidfileBasename(network->def->name)) + != NULL)) { + virPidFileDelete(NETWORK_PID_DIR, radvdpidbase); + VIR_FREE(radvdpidbase); + } + network->radvdPid = -1; + } + /* now start radvd if it should be started */ + return networkStartRadvd(network); +} +#endif /* #if 0 */ + static int networkAddMasqueradingIptablesRules(struct network_driver *driver, virNetworkObjPtr network,

A bit annoying to add so much #if 0'ed code as we don't check that code in practice, but okay, 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/

On Tue, Sep 18, 2012 at 03:39:02AM -0400, Laine Stump wrote:

Call the network_conf function that modifies the live/persistent/both config, then refresh/restart dnsmasq/radvd if necessary, and finally save the config in the proper place(s).

This patch also needed to uncomment a few utility functions that were added inside #if 0 in the previous commit (to avoid compiler errors due to unreferenced static functions). --- src/network/bridge_driver.c | 120 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 112 insertions(+), 8 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 5f7f31e..252c964 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -468,8 +468,6 @@ networkShutdown(void) { }

-#if 0 -/* currently unused, so it causes a build error unless we #if it out */ /* networkKillDaemon: * * kill the specified pid/name, and wait a bit to make sure it's dead. @@ -528,7 +526,6 @@ networkKillDaemon(pid_t pid, const char *daemonName, const char *networkName) cleanup: return ret; } -#endif /* #if 0 */

static int networkBuildDnsmasqHostsfile(dnsmasqContext *dctx, @@ -908,8 +905,6 @@ cleanup: return ret; }

-#if 0 -/* currently unused, so they cause a build error unless we #if them out */ /* networkRefreshDhcpDaemon: * Update dnsmasq config files, then send a SIGHUP so that it rereads * them. @@ -978,7 +973,6 @@ networkRestartDhcpDaemon(virNetworkObjPtr network) /* now start dnsmasq if it should be started */ return networkStartDhcpDaemon(network); } -#endif /* #if 0 */

static int networkRadvdConfContents(virNetworkObjPtr network, char **configstr) @@ -1171,8 +1165,6 @@ cleanup: return ret; }

-#if 0 -/* currently unused, so they cause a build error unless we #if them out */ static int networkRefreshRadvd(virNetworkObjPtr network) { @@ -1191,6 +1183,8 @@ networkRefreshRadvd(virNetworkObjPtr network) return kill(network->radvdPid, SIGHUP); }

+#if 0 +/* currently unused, so it causes a build error unless we #if it out */ static int networkRestartRadvd(virNetworkObjPtr network) { @@ -2830,6 +2824,115 @@ cleanup: return ret; }

+static int +networkUpdate(virNetworkPtr net, + unsigned int command, + unsigned int section, + int parentIndex, + const char *xml, + unsigned int flags) +{ + struct network_driver *driver = net->conn->networkPrivateData; + virNetworkObjPtr network = NULL; + int isActive, ret = -1; + + virCheckFlags(VIR_NETWORK_UPDATE_AFFECT_LIVE | + VIR_NETWORK_UPDATE_AFFECT_CONFIG, + -1); + + networkDriverLock(driver); + + network = virNetworkFindByUUID(&driver->networks, net->uuid); + if (!network) { + virReportError(VIR_ERR_NO_NETWORK, + "%s", _("no network with matching uuid")); + goto cleanup; + } + + /* VIR_NETWORK_UPDATE_AFFECT_CURRENT means "change LIVE if network + * is active, else change CONFIG + */ + isActive = virNetworkObjIsActive(network); + if ((flags & (VIR_NETWORK_UPDATE_AFFECT_LIVE + | VIR_NETWORK_UPDATE_AFFECT_CONFIG)) == + VIR_NETWORK_UPDATE_AFFECT_CURRENT) { + if (isActive) + flags |= VIR_NETWORK_UPDATE_AFFECT_LIVE; + else + flags |= VIR_NETWORK_UPDATE_AFFECT_CONFIG; + } + + /* update the network config in memory/on disk */ + if (virNetworkObjUpdate(network, command, section, parentIndex, xml, flags) < 0) + goto cleanup; + + if (flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG) { + /* save updated persistent config to disk */ + if (virNetworkSaveConfig(driver->networkConfigDir, + virNetworkObjGetPersistentDef(network)) < 0) { + goto cleanup; + } + } + + if (isActive && (flags & VIR_NETWORK_UPDATE_AFFECT_LIVE)) { + /* rewrite dnsmasq host files, restart dnsmasq, update iptables + * rules, etc, according to which section was modified. Note that + * some sections require multiple actions, so a single switch + * statement is inadequate. + */ + if (section == VIR_NETWORK_SECTION_BRIDGE || + section == VIR_NETWORK_SECTION_DOMAIN || + section == VIR_NETWORK_SECTION_IP || + section == VIR_NETWORK_SECTION_IP_DHCP_RANGE) { + /* these sections all change things on the dnsmasq commandline, + * so we need to kill and restart dnsmasq. + */ + if (networkRestartDhcpDaemon(network) < 0) + goto cleanup; + + } else if (section == VIR_NETWORK_SECTION_IP_DHCP_HOST || + section == VIR_NETWORK_SECTION_DNS_HOST || + section == VIR_NETWORK_SECTION_DNS_TXT || + section == VIR_NETWORK_SECTION_DNS_SRV) { + /* these sections only change things in config files, so we + * can just update the config files and send SIGHUP to + * dnsmasq. + */ + if (networkRefreshDhcpDaemon(network) < 0) + goto cleanup; + + } + + if (section == VIR_NETWORK_SECTION_IP) { + /* only a change in IP addresses will affect radvd, and all of radvd's + * config is stored in the conf file which will be re-read with a SIGHUP. + */ + if (networkRefreshRadvd(network) < 0) + goto cleanup; + } + + if (section == VIR_NETWORK_SECTION_IP || + section == VIR_NETWORK_SECTION_FORWARD || + section == VIR_NETWORK_SECTION_FORWARD_INTERFACE) { + /* these could affect the iptables rules */ + networkRemoveIptablesRules(driver, network); + if (networkAddIptablesRules(driver, network) < 0) + goto cleanup; + + } + + /* save current network state to disk */ + if ((ret = virNetworkSaveStatus(NETWORK_STATE_DIR, network)) < 0) + goto cleanup; + } + ret = 0; +cleanup: + if (network) + virNetworkObjUnlock(network); + networkDriverUnlock(driver); + return ret; +} + static int networkStart(virNetworkPtr net) { struct network_driver *driver = net->conn->networkPrivateData; virNetworkObjPtr network; @@ -3057,6 +3160,7 @@ static virNetworkDriver networkDriver = { .networkCreateXML = networkCreate, /* 0.2.0 */ .networkDefineXML = networkDefine, /* 0.2.0 */ .networkUndefine = networkUndefine, /* 0.2.0 */ + .networkUpdate = networkUpdate, /* 0.10.2 */ .networkCreate = networkStart, /* 0.2.0 */ .networkDestroy = networkDestroy, /* 0.2.0 */ .networkGetXMLDesc = networkGetXMLDesc, /* 0.2.0 */ --

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/

On Tue, Sep 18, 2012 at 03:39:03AM -0400, Laine Stump wrote:

The test driver does nothing outside of keeping track of each network's config/state in the in-memory database maintained by network_conf functions, so all we have to do is call the function that updates the network's entry in the in-memory database. --- src/test/test_driver.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 1bd0d61..0036563 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3126,7 +3126,9 @@ cleanup: return ret; }

-static virNetworkPtr testNetworkDefine(virConnectPtr conn, const char *xml) { +static +virNetworkPtr testNetworkDefine(virConnectPtr conn, const char *xml) +{ testConnPtr privconn = conn->privateData; virNetworkDefPtr def; virNetworkObjPtr net = NULL; @@ -3183,6 +3185,54 @@ cleanup: return ret; }

+static int +testNetworkUpdate(virNetworkPtr net, + unsigned int command, + unsigned int section, + int parentIndex, + const char *xml, + unsigned int flags) +{ + testConnPtr privconn = net->conn->privateData; + virNetworkObjPtr network = NULL; + int isActive, ret = -1; + + virCheckFlags(VIR_NETWORK_UPDATE_AFFECT_LIVE | + VIR_NETWORK_UPDATE_AFFECT_CONFIG, + -1); + + testDriverLock(privconn); + + network = virNetworkFindByUUID(&privconn->networks, net->uuid); + if (!network) { + virReportError(VIR_ERR_NO_NETWORK, + "%s", _("no network with matching uuid")); + goto cleanup; + } + + /* VIR_NETWORK_UPDATE_AFFECT_CURRENT means "change LIVE if network + * is active, else change CONFIG + */ + isActive = virNetworkObjIsActive(network); + if ((flags & (VIR_NETWORK_UPDATE_AFFECT_LIVE + | VIR_NETWORK_UPDATE_AFFECT_CONFIG)) == + VIR_NETWORK_UPDATE_AFFECT_CURRENT) { + if (isActive) + flags |= VIR_NETWORK_UPDATE_AFFECT_LIVE; + else + flags |= VIR_NETWORK_UPDATE_AFFECT_CONFIG; + } + + /* update the network config in memory/on disk */ + if (virNetworkObjUpdate(network, command, section, parentIndex, xml, flags) < 0) + goto cleanup; + + ret = 0; +cleanup: + testDriverUnlock(privconn); + return ret; +} + static int testNetworkStart(virNetworkPtr network) { testConnPtr privconn = network->conn->privateData; virNetworkObjPtr privnet; @@ -5722,6 +5772,7 @@ static virNetworkDriver testNetworkDriver = { .networkCreateXML = testNetworkCreate, /* 0.3.2 */ .networkDefineXML = testNetworkDefine, /* 0.3.2 */ .networkUndefine = testNetworkUndefine, /* 0.3.2 */ + .networkUpdate = testNetworkUpdate, /* 0.10.2 */ .networkCreate = testNetworkStart, /* 0.3.2 */ .networkDestroy = testNetworkDestroy, /* 0.3.2 */ .networkGetXMLDesc = testNetworkGetXMLDesc, /* 0.3.2 */

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/

On Tue, Sep 18, 2012 at 03:39:04AM -0400, Laine Stump wrote:

A user on IRC had accidentally killed all of his libvirt-started dnsmasq instances (due to a buggy dnsmasq service script in Fedora 16), and had hoped that libvirtd would notice this on restart and reload all the dnsmasq daemons (as it does with iptables rules). Unfortunately this was not the case - as long as the network object had a pid registered for dnsmasq and/or radvd, it assumed that the processes were running.

This patch takes advantage of the new utility functions in bridge_driver.c to do a "refresh" of all radvd and dnsmasq processes started by libvirt each time libvirtd is restarted - this function attempts to do a SIGHUP of each existing process, and if that fails, it restarts the process, rebuilding all the associated config files and commandline parameters in the process. This normally has no effect, but will be useful in solving the occasional "odd situation" without needing to take the drastic step of destroying/re-starting the network. --- src/network/bridge_driver.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 252c964..0e38016 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -117,6 +117,7 @@ static int networkShutdownNetworkExternal(struct network_driver *driver, virNetworkObjPtr network);

static void networkReloadIptablesRules(struct network_driver *driver); +static void networkRefreshDaemons(struct network_driver *driver);

static struct network_driver *driverState = NULL;

@@ -344,6 +345,7 @@ networkStartup(int privileged) {

networkFindActiveConfigs(driverState); networkReloadIptablesRules(driverState); + networkRefreshDaemons(driverState); networkAutostartConfigs(driverState);

networkDriverUnlock(driverState); @@ -404,6 +406,7 @@ networkReload(void) { driverState->networkConfigDir, driverState->networkAutostartDir); networkReloadIptablesRules(driverState); + networkRefreshDaemons(driverState); networkAutostartConfigs(driverState); networkDriverUnlock(driverState); return 0; @@ -1210,6 +1213,37 @@ networkRestartRadvd(virNetworkObjPtr network) } #endif /* #if 0 */

+/* SIGHUP/restart any dnsmasq or radvd daemons. + * This should be called when libvirtd is restarted. + */ +static void +networkRefreshDaemons(struct network_driver *driver) +{ + unsigned int i; + + VIR_INFO("Refreshing network daemons"); + + for (i = 0 ; i < driver->networks.count ; i++) { + virNetworkObjPtr network = driver->networks.objs[i]; + + virNetworkObjLock(network); + if (virNetworkObjIsActive(network) && + ((network->def->forwardType == VIR_NETWORK_FORWARD_NONE) || + (network->def->forwardType == VIR_NETWORK_FORWARD_NAT) || + (network->def->forwardType == VIR_NETWORK_FORWARD_ROUTE))) { + /* Only the three L3 network types that are configured by + * libvirt will have a dnsmasq or radvd daemon associated + * with them. Here we send a SIGHUP to an existing + * dnsmasq and/or radvd, or restart them if they've + * disappeared. + */ + networkRefreshDhcpDaemon(network); + networkRefreshRadvd(network); + } + virNetworkObjUnlock(network); + } +} + static int networkAddMasqueradingIptablesRules(struct network_driver *driver, virNetworkObjPtr network,

ACK, sounds simple and useful, hopefully there is no side effect to the HUP'ed daemons. 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/

On Tue, Sep 18, 2012 at 03:39:05AM -0400, Laine Stump wrote:

This patch fills in the first implementation for one of the virNetworkUpdate sections. With this code, you can now add/delete/edit <host> entries in a network's <ip> address <dhcp> element (by specifying a section of VIR_NETWORK_SECTION_IP_DHCP_HOST).

If you pass in a parentIndex of -1, the code will automatically find the one ip element that has a <dhcp> section and make the updates there. Otherwise, you can specify an index >= 0, and libvirt will look for that particular instance of <ip> in the network, and modify its <dhcp> element. (This currently isn't very useful, because libvirt only supports having dhcp information on a single IP address, but that could change in the future).

When adding a new host entry (VIR_NETWORK_UPDATE_COMMAND_ADD_(FIRST|LAST)), the existing entries will be compared to the new entry, and if any non-empty attribute matches, the add will fail. When updating an existing entry (VIR_NETWORK_UPDATE_COMMAND_MODIFY), the mac address or name will be used to find the existing entry, and other fields will only be updated (note there is some potential for ambiguity here if you specify the mac address from one entry and the name from another). When deleting an existing entry (VIR_NETWORK_UPDATE_COMMAND_DELETE), all non-empty attributes in the supplied xml arg will be compared - all of them must match before libvirt will delete the host.

The xml should be a fully formed <host> element as it would appear in a network definition, e.g. "<host mac=00:11:22:33:44:55 ip=10.1.23.22 name='testbox'/>" (when adding/updating, ip and one of mac|name is required; when deleting, you can specify any one, two, or all attributes, but they all must match the target element).

As with the update of any other section, you can choose to affect the live config (with flag VIR_NETWORK_UPDATE_AFFECT_LIVE), the persistent config (VIR_NETWORK_UPDATE_AFFECT_CONFIG), or both. If you've chosen to affect the live config, those changes will take effect immediately, with no need to destroy/restart the network.

An example of adding a host entry:

virNetworkUpdate(net, VIR_NETWORK_UPDATE_COMMAND_ADD_LAST, VIR_NETWORK_SECTION_IP_DHCP_HOST, -1, "<host mac='00:11:22:33:44:55' ip='192.168.122.5'/>", VIR_NETWORK_UPDATE_AFFECT_LIVE | VIR_NETWORK_UPDATE_AFFECT_CONFIG);

To delete that same entry:

virNetworkUpdate(net, VIR_NETWORK_UPDATE_COMMAND_DELETE, VIR_NETWORK_SECTION_IP_DHCP_HOST, -1, "<host mac='00:11:22:33:44:55'/>", VIR_NETWORK_UPDATE_AFFECT_LIVE | VIR_NETWORK_UPDATE_AFFECT_CONFIG);

(you could also delete it by replacing "mac='00:11:22:33:44:55'" with "ip='192.168.122.5'".)

We need to move that documentation outside of the git commit log, I mean keep it here but this need to go on the web site !

src/conf/network_conf.c | 181 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 174 insertions(+), 7 deletions(-)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 2a65f1d..4f40c10 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2261,7 +2261,6 @@ virNetworkDefUpdateNoSupport(virNetworkDefPtr def, const char *section) section, def->name); }

-#if 0 static int virNetworkDefUpdateCheckElementName(virNetworkDefPtr def, xmlNodePtr node, @@ -2276,7 +2275,6 @@ virNetworkDefUpdateCheckElementName(virNetworkDefPtr def, } return 0; } -#endif

static int virNetworkDefUpdateBridge(virNetworkDefPtr def, @@ -2314,16 +2312,185 @@ virNetworkDefUpdateIP(virNetworkDefPtr def, return -1; }

+static virNetworkIpDefPtr +virNetworkIpDefByIndex(virNetworkDefPtr def, int parentIndex) +{ + virNetworkIpDefPtr ipdef = NULL; + + /* first find which ip element's dhcp host list to work on */ + if (parentIndex >= 0) { + ipdef = virNetworkDefGetIpByIndex(def, AF_UNSPEC, parentIndex); + if (!(ipdef && + VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET))) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("couldn't update dhcp host entry - " + "no <ip family='ipv4'> " + "element found at index %d in network '%s'"), + parentIndex, def->name); + } + return ipdef; + } + + /* -1 means "find the most appropriate", which in this case + * means the one and only <ip> that has <dhcp> element + */ + int ii;

<grin/> can you move that up to be block start ?

+ + for (ii = 0; + (ipdef = virNetworkDefGetIpByIndex(def, AF_UNSPEC, ii)); + ii++) { + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET) && + (ipdef->nranges || ipdef->nhosts)) { + break; + } + } + if (!ipdef) + ipdef = virNetworkDefGetIpByIndex(def, AF_INET, 0); + if (!ipdef) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("couldn't update dhcp host entry - " + "no <ip family='ipv4'> " + "element found in network '%s'"), def->name); + } + return ipdef; +} + static int virNetworkDefUpdateIPDHCPHost(virNetworkDefPtr def, - unsigned int command ATTRIBUTE_UNUSED, - int parentIndex ATTRIBUTE_UNUSED, - xmlXPathContextPtr ctxt ATTRIBUTE_UNUSED, + unsigned int command, + int parentIndex, + xmlXPathContextPtr ctxt, /* virNetworkUpdateFlags */ unsigned int fflags ATTRIBUTE_UNUSED) { - virNetworkDefUpdateNoSupport(def, "ip dhcp host"); - return -1; + int ii, ret = -1; + virNetworkIpDefPtr ipdef = virNetworkIpDefByIndex(def, parentIndex); + virNetworkDHCPHostDef host; + + memset(&host, 0, sizeof(host)); + + if (virNetworkDefUpdateCheckElementName(def, ctxt->node, "host") < 0) + goto cleanup; + + /* ipdef is the ip element that needs its host array updated */ + if (!ipdef) + goto cleanup; + + /* parse the xml into a virNetworkDHCPHostDef */ + if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) { + + if (virNetworkDHCPHostDefParse(def->name, ctxt->node, &host, false) < 0) + goto cleanup; + + /* search for the entry with this (mac|name), + * and update the IP+(mac|name) */ + for (ii = 0; ii < ipdef->nhosts; ii++) { + if ((host.mac && + !virMacAddrCompare(host.mac, ipdef->hosts[ii].mac)) || + (host.name && + STREQ_NULLABLE(host.name, ipdef->hosts[ii].name))) { + break; + } + } + + if (ii == ipdef->nhosts) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("couldn't locate an existing dhcp host entry with " + "\"mac='%s'\" in network '%s'"), + host.mac, def->name); + goto cleanup; + } + + /* eliminate the old */ + virNetworkDHCPHostDefClear(&ipdef->hosts[ii]); + /* replace with new */ + ipdef->hosts[ii] = host; + /* eliminate the extra copy of the new */ + memset(&host, 0, sizeof(host)); + /* Success! */

Hum, those comments are fine in development mode, but maybe a bit of cleanup or more synthetic comment would be better :-)

+ } else if ((command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) || + (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST)) { + + if (virNetworkDHCPHostDefParse(def->name, ctxt->node, &host, true) < 0) + goto cleanup; + + /* log error if an entry with same name/address/ip already exists */ + for (ii = 0; ii < ipdef->nhosts; ii++) { + if ((host.mac && + !virMacAddrCompare(host.mac, ipdef->hosts[ii].mac)) || + (host.name && + STREQ_NULLABLE(host.name, ipdef->hosts[ii].name)) || + (VIR_SOCKET_ADDR_VALID(&host.ip) && + virSocketAddrEqual(&host.ip, &ipdef->hosts[ii].ip))) { + char *ip = virSocketAddrFormat(&host.ip); + + virReportError(VIR_ERR_OPERATION_INVALID, + _("there is an existing dhcp host entry in " + "network '%s' that matches " + "\"<host mac='%s' name='%s' ip='%s'/>\""), + def->name, host.mac, host.name, + ip ? ip : "unknown"); + VIR_FREE(ip); + goto cleanup; + } + } + /* add to beginning/end of list */ + if (VIR_REALLOC_N(ipdef->hosts, ipdef->nhosts +1) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST) { + + ipdef->hosts[ipdef->nhosts] = host; + ipdef->nhosts++; + memset(&host, 0, sizeof(host)); + + } else { /* implied (command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) */ + + memmove(ipdef->hosts + 1, ipdef->hosts, + sizeof(ipdef->hosts) * ipdef->nhosts); + ipdef->hosts[0] = host; + ipdef->nhosts++; + memset(&host, 0, sizeof(host)); + } + + } else if (command == VIR_NETWORK_UPDATE_COMMAND_DELETE) { + + if (virNetworkDHCPHostDefParse(def->name, ctxt->node, &host, false) < 0) + goto cleanup; + + /* find matching entry - all specified attributes must match */ + for (ii = 0; ii < ipdef->nhosts; ii++) { + if ((!host.mac || + !virMacAddrCompare(host.mac, ipdef->hosts[ii].mac)) && + (!host.name || + STREQ_NULLABLE(host.name, ipdef->hosts[ii].name)) && + (!VIR_SOCKET_ADDR_VALID(&host.ip) || + virSocketAddrEqual(&host.ip, &ipdef->hosts[ii].ip))) { + break; + } + } + if (ii == ipdef->nhosts) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("couldn't locate a matching dhcp host entry " + "in network '%s'"), def->name); + goto cleanup; + } + + /* remove it */ + virNetworkDHCPHostDefClear(&ipdef->hosts[ii]); + memmove(ipdef->hosts + ii, ipdef->hosts + ii + 1, + sizeof(ipdef->hosts) * ipdef->nhosts - ii - 1); + ipdef->nhosts--; + ignore_value(VIR_REALLOC_N(ipdef->hosts, ipdef->nhosts)); + } + + ret = 0; +cleanup: + virNetworkDHCPHostDefClear(&host); + return ret; }

static int

Okay, i think we need to add that patch to be able to actually test the new API, ACK but if possible clean the 2 things above 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/