[libvirt] [PATCH 0/4] virNetworkUpdate bugfixes
The 4 patches here are in decreasing order of importance. The first: [PATCH 1/4] network: don't refresh iptables rules on networks prevents proper behavior with a valid request when modifying the interface pool list of a macvtap or hostdev network (because it tries to refresh non-existent iptables rules, fails, then aborts the update). The second: [PATCH 2/4] network: make virNetworkObjUpdate error detection/recovery better fixes two problems that would crop up when attempting an invalid update to a network. The third was pointed out by DV in his review of the dhcp-range and portgroup update backends: [PATCH 3/4] network: log error for unknown virNetworkUpdate command would allow a call to the virNetworkUpdate() API with an unknown command number to succeed (it wouldn't do anything, but might provide the illusion that it *had*). The fourth: [PATCH 4/4] network: backend for virNetworkUpdate of interface list is the last of the networkUpdate section backends that I was explicitly/specifically asked to implement.
The bridge driver implementation of virNetworkUpdate() removes and re-adds iptables rules any time a network has an <ip>, <forward>, or <forward>/<interface> elements updated. There are some types of networks that have those elements and yet have no iptables rules associated with them, and unfortunately the functions that remove/add iptables rules don't check the type of network before attempting to remove/add the rules. Under normal circumstances I would refactor the lower level functions to be more robust, but to avoid code churn as much as possible, I've just added extra checks directly to networkUpdate(). --- src/network/bridge_driver.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index fce1739..6e260f7 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2945,9 +2945,12 @@ networkUpdate(virNetworkPtr net, goto cleanup; } - if (section == VIR_NETWORK_SECTION_IP || - section == VIR_NETWORK_SECTION_FORWARD || - section == VIR_NETWORK_SECTION_FORWARD_INTERFACE) { + if ((section == VIR_NETWORK_SECTION_IP || + section == VIR_NETWORK_SECTION_FORWARD || + section == VIR_NETWORK_SECTION_FORWARD_INTERFACE) && + (network->def->forwardType == VIR_NETWORK_FORWARD_NONE || + network->def->forwardType == VIR_NETWORK_FORWARD_NAT || + network->def->forwardType == VIR_NETWORK_FORWARD_ROUTE)) { /* these could affect the iptables rules */ networkRemoveIptablesRules(driver, network); if (networkAddIptablesRules(driver, network) < 0) -- 1.7.11.4
On 09/21/2012 01:46 PM, Laine Stump wrote:
The bridge driver implementation of virNetworkUpdate() removes and re-adds iptables rules any time a network has an <ip>, <forward>, or <forward>/<interface> elements updated. There are some types of networks that have those elements and yet have no iptables rules associated with them, and unfortunately the functions that remove/add iptables rules don't check the type of network before attempting to remove/add the rules.
Under normal circumstances I would refactor the lower level functions to be more robust, but to avoid code churn as much as possible, I've just added extra checks directly to networkUpdate(). --- src/network/bridge_driver.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index fce1739..6e260f7 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2945,9 +2945,12 @@ networkUpdate(virNetworkPtr net, goto cleanup; }
- if (section == VIR_NETWORK_SECTION_IP || - section == VIR_NETWORK_SECTION_FORWARD || - section == VIR_NETWORK_SECTION_FORWARD_INTERFACE) { + if ((section == VIR_NETWORK_SECTION_IP || + section == VIR_NETWORK_SECTION_FORWARD || + section == VIR_NETWORK_SECTION_FORWARD_INTERFACE) && + (network->def->forwardType == VIR_NETWORK_FORWARD_NONE ||
Is network->def->forwardType something that can be changed by networkUpdate()? If so,
+ network->def->forwardType == VIR_NETWORK_FORWARD_NAT || + network->def->forwardType == VIR_NETWORK_FORWARD_ROUTE)) { /* these could affect the iptables rules */ networkRemoveIptablesRules(driver, network); if (networkAddIptablesRules(driver, network) < 0)
then it seems like you'd have to check the old type before networkRemoveIptablesRules, and the new type before networkAddIptablesRules. But if the forward type is always locked down once the network is started (where changing types requires destroying and restarting the network, rather than an on-the-fly update), then this makes sense. Conditional ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/21/2012 04:04 PM, Eric Blake wrote:
On 09/21/2012 01:46 PM, Laine Stump wrote:
The bridge driver implementation of virNetworkUpdate() removes and re-adds iptables rules any time a network has an <ip>, <forward>, or <forward>/<interface> elements updated. There are some types of networks that have those elements and yet have no iptables rules associated with them, and unfortunately the functions that remove/add iptables rules don't check the type of network before attempting to remove/add the rules.
Under normal circumstances I would refactor the lower level functions to be more robust, but to avoid code churn as much as possible, I've just added extra checks directly to networkUpdate(). --- src/network/bridge_driver.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index fce1739..6e260f7 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2945,9 +2945,12 @@ networkUpdate(virNetworkPtr net, goto cleanup; }
- if (section == VIR_NETWORK_SECTION_IP || - section == VIR_NETWORK_SECTION_FORWARD || - section == VIR_NETWORK_SECTION_FORWARD_INTERFACE) { + if ((section == VIR_NETWORK_SECTION_IP || + section == VIR_NETWORK_SECTION_FORWARD || + section == VIR_NETWORK_SECTION_FORWARD_INTERFACE) && + (network->def->forwardType == VIR_NETWORK_FORWARD_NONE || Is network->def->forwardType something that can be changed by networkUpdate()?
Currently it's locked down, because the backend that can do that hasn't been implemented yet.
If so,
+ network->def->forwardType == VIR_NETWORK_FORWARD_NAT || + network->def->forwardType == VIR_NETWORK_FORWARD_ROUTE)) { /* these could affect the iptables rules */ networkRemoveIptablesRules(driver, network); if (networkAddIptablesRules(driver, network) < 0) then it seems like you'd have to check the old type before networkRemoveIptablesRules, and the new type before networkAddIptablesRules.
Beyond that, we would have to keep the original networkdef in place until after networkRemoveiptablesRules was finished, since it uses things from there. And not just when moving from one type of network to another, but also when adding/removing IP addresses.
But if the forward type is always locked down once the network is started (where changing types requires destroying and restarting the network, rather than an on-the-fly update), then this makes sense. Conditional ACK.
Currently it is locked down. As a matter of fact, the section that is triggering this code is the update of <forward>/<interface>, which is almost never useful unless you're in one of the modes that doesn't setup iptable rules anyway. There's just a seldom-used corner case where the dev attribute of <forward> could also be used to limit egress traffic to a single physical interface, and if that was being used, the likelyhood they would want to change it live is much lower than the chance that someone would want to add another <interface> to a pool used for one of the "non-iptables-using" modes. So, the conclusion is that I think it's safe for now to not worry about that, but when I add support for updating <ip> and <forward>, then we'll need to deal with it.
1) virNetworkObjUpdate should be an all or none operation, but in the case that we want to update both the live state and persistent config versions of the network, it was committing the update to the live state before starting to update the persistent config. If update of the persistent config failed, we would leave with things in an inconsistent state - the live state would be updated (even though an error was returned), but persistent config unchanged. This patch changed virNetworkObjUpdate to use a separate pointer for each copy of the virNetworkDef, and not commit either of them in the virNetworkObj until both live and config parts of the update have successfully completed. 2) The parsers for various pieces of the virNetworkDef have all sorts of subtle limitations on them that may not be known by the Update[section] function, making it possible for one of these functions to make a modification directly to the object that may not pass the scrutiny of a subsequent parse. But normally another parse wouldn't be done on the data until the *next* time the object was updated (which could leave the network definition in an unusable state). Rather than fighting the losing battle of trying to duplicate all the checks from the parsers into the update functions as well, the more foolproof solution to this is to simply do an extra virNetworkDefCopy() operation on the updated networkdef - virNetworkDefCopy() does a virNetworkFormat() followed by a virNetworkParseString(), so it will do all the checks we need. If this fails, then we don't commit the changed def. --- src/conf/network_conf.c | 47 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 13 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 34487dd..17b9643 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2840,45 +2840,66 @@ virNetworkObjUpdate(virNetworkObjPtr network, unsigned int flags) /* virNetworkUpdateFlags */ { int ret = -1; - virNetworkDefPtr def = NULL; + virNetworkDefPtr livedef = NULL, configdef = NULL; /* normalize config data, and check for common invalid requests. */ if (virNetworkConfigChangeSetup(network, flags) < 0) goto cleanup; if (flags & VIR_NETWORK_UPDATE_AFFECT_LIVE) { + virNetworkDefPtr checkdef; + /* work on a copy of the def */ - if (!(def = virNetworkDefCopy(network->def, 0))) + if (!(livedef = virNetworkDefCopy(network->def, 0))) goto cleanup; - if (virNetworkDefUpdateSection(def, command, section, + if (virNetworkDefUpdateSection(livedef, command, section, parentIndex, xml, flags) < 0) { goto cleanup; } - /* successfully modified copy, now replace original */ - virNetworkDefFree(network->def); - network->def = def; - def = NULL; + /* run a final format/parse cycle to make sure we didn't + * add anything illegal to the def + */ + if (!(checkdef = virNetworkDefCopy(livedef, 0))) + goto cleanup; + virNetworkDefFree(checkdef); } if (flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG) { + virNetworkDefPtr checkdef; + /* work on a copy of the def */ - if (!(def = virNetworkDefCopy(virNetworkObjGetPersistentDef(network), - VIR_NETWORK_XML_INACTIVE))) { + if (!(configdef = virNetworkDefCopy(virNetworkObjGetPersistentDef(network), + VIR_NETWORK_XML_INACTIVE))) { goto cleanup; } - if (virNetworkDefUpdateSection(def, command, section, + if (virNetworkDefUpdateSection(configdef, command, section, parentIndex, xml, flags) < 0) { goto cleanup; } + if (!(checkdef = virNetworkDefCopy(configdef, + VIR_NETWORK_XML_INACTIVE))) { + goto cleanup; + } + virNetworkDefFree(checkdef); + } + + if (configdef) { /* successfully modified copy, now replace original */ - if (virNetworkObjReplacePersistentDef(network, def) < 0) + if (virNetworkObjReplacePersistentDef(network, configdef) < 0) goto cleanup; - def = NULL; + configdef = NULL; + } + if (livedef) { + /* successfully modified copy, now replace original */ + virNetworkDefFree(network->def); + network->def = livedef; + livedef = NULL; } ret = 0; cleanup: - virNetworkDefFree(def); + virNetworkDefFree(livedef); + virNetworkDefFree(configdef); return ret; } -- 1.7.11.4
On 09/21/2012 01:46 PM, Laine Stump wrote:
1) virNetworkObjUpdate should be an all or none operation, but in the case that we want to update both the live state and persistent config versions of the network, it was committing the update to the live state before starting to update the persistent config. If update of the persistent config failed, we would leave with things in an inconsistent state - the live state would be updated (even though an error was returned), but persistent config unchanged.
This patch changed virNetworkObjUpdate to use a separate pointer for each copy of the virNetworkDef, and not commit either of them in the virNetworkObj until both live and config parts of the update have successfully completed.
2) The parsers for various pieces of the virNetworkDef have all sorts of subtle limitations on them that may not be known by the Update[section] function, making it possible for one of these functions to make a modification directly to the object that may not pass the scrutiny of a subsequent parse. But normally another parse wouldn't be done on the data until the *next* time the object was updated (which could leave the network definition in an unusable state).
Rather than fighting the losing battle of trying to duplicate all the checks from the parsers into the update functions as well, the more foolproof solution to this is to simply do an extra virNetworkDefCopy() operation on the updated networkdef - virNetworkDefCopy() does a virNetworkFormat() followed by a virNetworkParseString(), so it will do all the checks we need. If this fails, then we don't commit the changed def.
Not necessarily the fastest approach, but also not the first time we've used round-tripping (and reminds me that I still want an API someday that the user can call to canonicalize XML via round-tripping through the parser and formatter).
--- src/conf/network_conf.c | 47 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 13 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 34487dd..17b9643 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2840,45 +2840,66 @@ virNetworkObjUpdate(virNetworkObjPtr network, unsigned int flags) /* virNetworkUpdateFlags */ { int ret = -1; - virNetworkDefPtr def = NULL; + virNetworkDefPtr livedef = NULL, configdef = NULL;
/* normalize config data, and check for common invalid requests. */ if (virNetworkConfigChangeSetup(network, flags) < 0) goto cleanup;
if (flags & VIR_NETWORK_UPDATE_AFFECT_LIVE) { + virNetworkDefPtr checkdef;
Is it worth hoisting this declaration,
if (flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG) { + virNetworkDefPtr checkdef;
since you use it twice? But no semantic change, so no problem if you don't. ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/21/2012 04:13 PM, Eric Blake wrote:
1) virNetworkObjUpdate should be an all or none operation, but in the case that we want to update both the live state and persistent config versions of the network, it was committing the update to the live state before starting to update the persistent config. If update of the persistent config failed, we would leave with things in an inconsistent state - the live state would be updated (even though an error was returned), but persistent config unchanged.
This patch changed virNetworkObjUpdate to use a separate pointer for each copy of the virNetworkDef, and not commit either of them in the virNetworkObj until both live and config parts of the update have successfully completed.
2) The parsers for various pieces of the virNetworkDef have all sorts of subtle limitations on them that may not be known by the Update[section] function, making it possible for one of these functions to make a modification directly to the object that may not pass the scrutiny of a subsequent parse. But normally another parse wouldn't be done on the data until the *next* time the object was updated (which could leave the network definition in an unusable state).
Rather than fighting the losing battle of trying to duplicate all the checks from the parsers into the update functions as well, the more foolproof solution to this is to simply do an extra virNetworkDefCopy() operation on the updated networkdef - virNetworkDefCopy() does a virNetworkFormat() followed by a virNetworkParseString(), so it will do all the checks we need. If this fails, then we don't commit the changed def. Not necessarily the fastest approach, but also not the first time we've used round-tripping (and reminds me that I still want an API someday
On 09/21/2012 01:46 PM, Laine Stump wrote: that the user can call to canonicalize XML via round-tripping through the parser and formatter).
--- src/conf/network_conf.c | 47 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 13 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 34487dd..17b9643 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2840,45 +2840,66 @@ virNetworkObjUpdate(virNetworkObjPtr network, unsigned int flags) /* virNetworkUpdateFlags */ { int ret = -1; - virNetworkDefPtr def = NULL; + virNetworkDefPtr livedef = NULL, configdef = NULL;
/* normalize config data, and check for common invalid requests. */ if (virNetworkConfigChangeSetup(network, flags) < 0) goto cleanup;
if (flags & VIR_NETWORK_UPDATE_AFFECT_LIVE) { + virNetworkDefPtr checkdef; Is it worth hoisting this declaration,
if (flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG) { + virNetworkDefPtr checkdef;
since you use it twice? But no semantic change, so no problem if you don't.
I did it that way on purpose, just to make it clear that it would never need cleaning up at the end of the function.
ACK.
Okay, thanks. I'm pushing this then.
Every level of the code for virNetworkUpdate was assuming that some other level was checking for validity of the "command" arg, but none actually were. The result was that an invalid command code would do nothing, but also report success. Since the command code isn't used until the very lowest level backend functions, that's where I put the check. I made a separate one-line function to log the error. The compiler would have combined the identical strings used by multiple calls if I'd just called virReportError directly in each location, but sending them all to the same string in the source guards against inadvertant divergence (which would lead to extra work for translators.) --- src/conf/network_conf.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 17b9643..a2d82d4 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2260,6 +2260,12 @@ virNetworkDefUpdateNoSupport(virNetworkDefPtr def, const char *section) _("can't update '%s' section of network '%s'"), section, def->name); } +static void +virNetworkDefUpdateUnknownCommand(unsigned int command) +{ + virReportError(VIR_ERR_NO_SUPPORT, + _("unrecognized network update command code %d"), command); +} static int virNetworkDefUpdateCheckElementName(virNetworkDefPtr def, @@ -2484,6 +2490,9 @@ virNetworkDefUpdateIPDHCPHost(virNetworkDefPtr def, sizeof(*ipdef->hosts) * (ipdef->nhosts - ii - 1)); ipdef->nhosts--; ignore_value(VIR_REALLOC_N(ipdef->hosts, ipdef->nhosts)); + } else { + virNetworkDefUpdateUnknownCommand(command); + goto cleanup; } ret = 0; @@ -2581,6 +2590,9 @@ virNetworkDefUpdateIPDHCPRange(virNetworkDefPtr def, sizeof(*ipdef->ranges) * (ipdef->nranges - ii - 1)); ipdef->nranges--; ignore_value(VIR_REALLOC_N(ipdef->ranges, ipdef->nranges)); + } else { + virNetworkDefUpdateUnknownCommand(command); + goto cleanup; } ret = 0; @@ -2701,6 +2713,9 @@ virNetworkDefUpdatePortGroup(virNetworkDefPtr def, sizeof(*def->portGroups) * (def->nPortGroups - ii - 1)); def->nPortGroups--; ignore_value(VIR_REALLOC_N(def->portGroups, def->nPortGroups)); + } else { + virNetworkDefUpdateUnknownCommand(command); + goto cleanup; } ret = 0; -- 1.7.11.4
On 09/21/12 21:46, Laine Stump wrote:
Every level of the code for virNetworkUpdate was assuming that some other level was checking for validity of the "command" arg, but none actually were. The result was that an invalid command code would do nothing, but also report success.
Since the command code isn't used until the very lowest level backend functions, that's where I put the check. I made a separate one-line function to log the error. The compiler would have combined the identical strings used by multiple calls if I'd just called virReportError directly in each location, but sending them all to the same string in the source guards against inadvertant divergence (which would lead to extra work for translators.) --- src/conf/network_conf.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
ACK. Peter
On 09/21/2012 04:13 PM, Peter Krempa wrote:
On 09/21/12 21:46, Laine Stump wrote:
Every level of the code for virNetworkUpdate was assuming that some other level was checking for validity of the "command" arg, but none actually were. The result was that an invalid command code would do nothing, but also report success.
Since the command code isn't used until the very lowest level backend functions, that's where I put the check. I made a separate one-line function to log the error. The compiler would have combined the identical strings used by multiple calls if I'd just called virReportError directly in each location, but sending them all to the same string in the source guards against inadvertant divergence (which would lead to extra work for translators.) --- src/conf/network_conf.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
ACK.
Thanks, I pushed this one too.
<interface> elements are location inside the <forward> element of a network. There is only one <forward> element in any network, but it might have many <interface> elements. This element only contains a single attribute, "dev", which is the name of a network device (e.g. "eth0"). Since there is only a single attribute, the modify operation isn't supported for this "section", only add-first, add-last, and delete. Also, note that it's not permitted to delete an interface from the list while any guest is using it. We may later decide this is safe (because removing it from the list really only excludes it from consideration in future guest allocations of interfaces, but doesn't affect any guests currently connected), but for now this limitation seems prudent (of course when changing the persistent config, this limitation doesn't apply, because the persistent config doesn't support the concept of "in used"). Another limitation - it is also possible for the interfraces in this list to be described by PCI address rather than netdev name. However, I noticed while writing this function that we currently don't support defining interfaces that way in config - the only method of getting interfaces specified as <adress type='pci' ..../> instead of <interface dev='xx'/> is to provide a <pf dev='yy'/> element under forward, and let the entries in the interface list be automatically populated with the virtual functions (VF) of the physical function device given in <pg>. As with the other virNetworkUpdate section backends, support for this section is completely contained within a single static function, no other changes were required, and only functions already called from elsewhere within the same file are used in the new content for this existing function (i.e., adding this code should not cause a new build problem on any platform). --- src/conf/network_conf.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 94 insertions(+), 2 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index a2d82d4..4f853e3 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2620,8 +2620,100 @@ virNetworkDefUpdateForwardInterface(virNetworkDefPtr def, /* virNetworkUpdateFlags */ unsigned int fflags ATTRIBUTE_UNUSED) { - virNetworkDefUpdateNoSupport(def, "forward interface"); - return -1; + int ii, ret = -1; + virNetworkForwardIfDef iface; + + memset(&iface, 0, sizeof(iface)); + + if (virNetworkDefUpdateCheckElementName(def, ctxt->node, "interface") < 0) + goto cleanup; + + if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("forward interface entries cannot be modified, " + "only added or deleted")); + goto cleanup; + } + + /* parsing this is so simple that it doesn't have its own function */ + iface.type = VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NETDEV; + if (!(iface.device.dev = virXMLPropString(ctxt->node, "dev"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing dev attribute in <interface> element")); + } + + /* check if an <interface> with same dev name already exists */ + for (ii = 0; ii < def->nForwardIfs; ii++) { + if (def->forwardIfs[ii].type + == VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NETDEV && + STREQ(iface.device.dev, def->forwardIfs[ii].device.dev)) + break; + } + + if ((command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) || + (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST)) { + + if (ii < def->nForwardIfs) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("there is an existing interface entry " + "in network '%s' that matches " + "\"<interface dev='%s'>\""), + def->name, iface.device.dev); + goto cleanup; + } + + /* add to beginning/end of list */ + if (VIR_REALLOC_N(def->forwardIfs, def->nForwardIfs +1) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST) { + def->forwardIfs[def->nForwardIfs] = iface; + } else { /* implied (command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) */ + memmove(def->forwardIfs + 1, def->forwardIfs, + sizeof(*def->forwardIfs) * def->nForwardIfs); + def->forwardIfs[0] = iface; + } + def->nForwardIfs++; + memset(&iface, 0, sizeof(iface)); + + } else if (command == VIR_NETWORK_UPDATE_COMMAND_DELETE) { + + if (ii == def->nForwardIfs) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("couldn't find an interface entry " + "in network '%s' matching <interface dev='%s'>"), + def->name, iface.device.dev); + goto cleanup; + } + + /* fail if the interface is being used */ + if (def->forwardIfs[ii].connections > 0) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("unable to delete interface '%s' " + "in network '%s'. It is currently being used " + " by %d domains."), + iface.device.dev, def->name, + def->forwardIfs[ii].connections); + goto cleanup; + } + + /* remove it */ + virNetworkForwardIfDefClear(&def->forwardIfs[ii]); + memmove(def->forwardIfs + ii, def->forwardIfs + ii + 1, + sizeof(*def->forwardIfs) * (def->nForwardIfs - ii - 1)); + def->nForwardIfs--; + ignore_value(VIR_REALLOC_N(def->forwardIfs, def->nForwardIfs)); + } else { + virNetworkDefUpdateUnknownCommand(command); + goto cleanup; + } + + ret = 0; +cleanup: + virNetworkForwardIfDefClear(&iface); + return ret; } static int -- 1.7.11.4
On 09/21/12 21:46, Laine Stump wrote:
<interface> elements are location inside the <forward> element of a network. There is only one <forward> element in any network, but it might have many <interface> elements. This element only contains a single attribute, "dev", which is the name of a network device (e.g. "eth0").
Since there is only a single attribute, the modify operation isn't supported for this "section", only add-first, add-last, and delete. Also, note that it's not permitted to delete an interface from the list while any guest is using it. We may later decide this is safe (because removing it from the list really only excludes it from consideration in future guest allocations of interfaces, but doesn't affect any guests currently connected), but for now this limitation seems prudent (of course when changing the persistent config, this limitation doesn't apply, because the persistent config doesn't support the concept of "in used").
s/used/use/
Another limitation - it is also possible for the interfraces in this
s/interfraces/interfaces/
list to be described by PCI address rather than netdev name. However, I noticed while writing this function that we currently don't support defining interfaces that way in config - the only method of getting interfaces specified as <adress type='pci' ..../> instead of
s/adress/address/
<interface dev='xx'/> is to provide a <pf dev='yy'/> element under forward, and let the entries in the interface list be automatically populated with the virtual functions (VF) of the physical function device given in <pg>.
As with the other virNetworkUpdate section backends, support for this section is completely contained within a single static function, no other changes were required, and only functions already called from elsewhere within the same file are used in the new content for this existing function (i.e., adding this code should not cause a new build problem on any platform). --- src/conf/network_conf.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 94 insertions(+), 2 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index a2d82d4..4f853e3 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2620,8 +2620,100 @@ virNetworkDefUpdateForwardInterface(virNetworkDefPtr def, /* virNetworkUpdateFlags */ unsigned int fflags ATTRIBUTE_UNUSED)
In the function header there are multiple arguments marked as unused, but the new code uses them. You should unmark "command" and "ctxt" as unused.
{ - virNetworkDefUpdateNoSupport(def, "forward interface"); - return -1; + int ii, ret = -1; + virNetworkForwardIfDef iface; + + memset(&iface, 0, sizeof(iface)); + + if (virNetworkDefUpdateCheckElementName(def, ctxt->node, "interface") < 0) + goto cleanup; + + if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("forward interface entries cannot be modified, " + "only added or deleted")); + goto cleanup; + } + + /* parsing this is so simple that it doesn't have its own function */ + iface.type = VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NETDEV; + if (!(iface.device.dev = virXMLPropString(ctxt->node, "dev"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing dev attribute in <interface> element"));
goto cleanup; missing?
+ } + + /* check if an <interface> with same dev name already exists */ + for (ii = 0; ii < def->nForwardIfs; ii++) { + if (def->forwardIfs[ii].type + == VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NETDEV &&
I'd rather have a >80 cols line than break expressions in half. But this isn't really relevant to discuss in context of this patch.
+ STREQ(iface.device.dev, def->forwardIfs[ii].device.dev)) + break; + } + + if ((command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) || + (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST)) { + + if (ii < def->nForwardIfs) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("there is an existing interface entry " + "in network '%s' that matches " + "\"<interface dev='%s'>\""), + def->name, iface.device.dev); + goto cleanup; + } + + /* add to beginning/end of list */ + if (VIR_REALLOC_N(def->forwardIfs, def->nForwardIfs +1) < 0) {
Add space after +.
+ virReportOOMError(); + goto cleanup; + } + + if (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST) { + def->forwardIfs[def->nForwardIfs] = iface; + } else { /* implied (command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) */ + memmove(def->forwardIfs + 1, def->forwardIfs, + sizeof(*def->forwardIfs) * def->nForwardIfs); + def->forwardIfs[0] = iface; + } + def->nForwardIfs++; + memset(&iface, 0, sizeof(iface)); + + } else if (command == VIR_NETWORK_UPDATE_COMMAND_DELETE) { + + if (ii == def->nForwardIfs) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("couldn't find an interface entry " + "in network '%s' matching <interface dev='%s'>"), + def->name, iface.device.dev); + goto cleanup; + } + + /* fail if the interface is being used */ + if (def->forwardIfs[ii].connections > 0) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("unable to delete interface '%s' " + "in network '%s'. It is currently being used " + " by %d domains."), + iface.device.dev, def->name, + def->forwardIfs[ii].connections); + goto cleanup; + } + + /* remove it */ + virNetworkForwardIfDefClear(&def->forwardIfs[ii]); + memmove(def->forwardIfs + ii, def->forwardIfs + ii + 1, + sizeof(*def->forwardIfs) * (def->nForwardIfs - ii - 1)); + def->nForwardIfs--; + ignore_value(VIR_REALLOC_N(def->forwardIfs, def->nForwardIfs)); + } else { + virNetworkDefUpdateUnknownCommand(command); + goto cleanup; + } + + ret = 0; +cleanup: + virNetworkForwardIfDefClear(&iface); + return ret; }
static int
ACK with the nits fixed. Peter
On 09/21/2012 04:45 PM, Peter Krempa wrote:
On 09/21/12 21:46, Laine Stump wrote:
<interface> elements are location inside the <forward> element of a network. There is only one <forward> element in any network, but it might have many <interface> elements. This element only contains a single attribute, "dev", which is the name of a network device (e.g. "eth0").
Since there is only a single attribute, the modify operation isn't supported for this "section", only add-first, add-last, and delete. Also, note that it's not permitted to delete an interface from the list while any guest is using it. We may later decide this is safe (because removing it from the list really only excludes it from consideration in future guest allocations of interfaces, but doesn't affect any guests currently connected), but for now this limitation seems prudent (of course when changing the persistent config, this limitation doesn't apply, because the persistent config doesn't support the concept of "in used").
s/used/use/
Another limitation - it is also possible for the interfraces in this
s/interfraces/interfaces/
list to be described by PCI address rather than netdev name. However, I noticed while writing this function that we currently don't support defining interfaces that way in config - the only method of getting interfaces specified as <adress type='pci' ..../> instead of
s/adress/address/
<interface dev='xx'/> is to provide a <pf dev='yy'/> element under forward, and let the entries in the interface list be automatically populated with the virtual functions (VF) of the physical function device given in <pg>.
As with the other virNetworkUpdate section backends, support for this section is completely contained within a single static function, no other changes were required, and only functions already called from elsewhere within the same file are used in the new content for this existing function (i.e., adding this code should not cause a new build problem on any platform). --- src/conf/network_conf.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 94 insertions(+), 2 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index a2d82d4..4f853e3 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2620,8 +2620,100 @@ virNetworkDefUpdateForwardInterface(virNetworkDefPtr def, /* virNetworkUpdateFlags */ unsigned int fflags ATTRIBUTE_UNUSED)
In the function header there are multiple arguments marked as unused, but the new code uses them. You should unmark "command" and "ctxt" as unused.
Right. I'd forgotten that with the earlier section functions, but managed to catch it myself before posting them...
{ - virNetworkDefUpdateNoSupport(def, "forward interface"); - return -1; + int ii, ret = -1; + virNetworkForwardIfDef iface; + + memset(&iface, 0, sizeof(iface)); + + if (virNetworkDefUpdateCheckElementName(def, ctxt->node, "interface") < 0) + goto cleanup; + + if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("forward interface entries cannot be modified, " + "only added or deleted")); + goto cleanup; + } + + /* parsing this is so simple that it doesn't have its own function */ + iface.type = VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NETDEV; + if (!(iface.device.dev = virXMLPropString(ctxt->node, "dev"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing dev attribute in <interface> element"));
goto cleanup; missing?
Eep. :-/. The level of "nits" in this patch has convinced me to *not* try and convince anyone to let this go in before the release. It's true that it's isolated changes, and they are only adding missing functionality to a new feature (so they shouldn't affect any existing functionality), but anyway there are other pieces that are also still missing (dns-host, dns-srv, and dns-text are the next ones to tackle), so this can just as well go in after the release with the others.
+ } + + /* check if an <interface> with same dev name already exists */ + for (ii = 0; ii < def->nForwardIfs; ii++) { + if (def->forwardIfs[ii].type + == VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NETDEV &&
I'd rather have a >80 cols line than break expressions in half. But this isn't really relevant to discuss in context of this patch.
I'm on the fence on this issue (except in the case of help messages (which we seem to not care about - see the output of "virsh help" on an 80 column display for example) and commit logs (where we do pretty good). I'll cut off the original message here for brevity - I'm fixing all the issues you found and will push after 0.10.2 is released. Thanks!
participants (3)
-
Eric Blake -
Laine Stump -
Peter Krempa