2:46 p.m.
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.
2:46 p.m.
New subject: [libvirt] [PATCH 1/4] network: don't refresh iptables rules on networks without them
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
3:04 p.m.
New subject: [libvirt] [PATCH 1/4] network: don't refresh iptables rules on networks without them
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3:37 p.m.
New subject: [libvirt] [PATCH 1/4] network: don't refresh iptables rules on networks without them
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.
2:46 p.m.
New subject: [libvirt] [PATCH 2/4] network: make virNetworkObjUpdate error detection/recovery better
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
3:13 p.m.
New subject: [libvirt] [PATCH 2/4] network: make virNetworkObjUpdate error detection/recovery better
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3:37 p.m.
New subject: [libvirt] [PATCH 2/4] network: make virNetworkObjUpdate error detection/recovery better
On 09/21/2012 04:13 PM, Eric Blake wrote:
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.
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.
2:46 p.m.
New subject: [libvirt] [PATCH 3/4] network: log error for unknown virNetworkUpdate command codes
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
3:13 p.m.
New subject: [libvirt] [PATCH 3/4] network: log error for unknown virNetworkUpdate command codes
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
8:14 p.m.
New subject: [libvirt] [PATCH 3/4] network: log error for unknown virNetworkUpdate command codes
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.
2:46 p.m.
New subject: [libvirt] [PATCH 4/4] network: backend for virNetworkUpdate of interface list
<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
3:45 p.m.
New subject: [libvirt] [PATCH 4/4] network: backend for virNetworkUpdate of interface list
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
8:46 p.m.
New subject: [libvirt] [PATCH 4/4] network: backend for virNetworkUpdate of interface list
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!
4446
days inactive
4447
days old
12 comments
3 participants
participants (3)
-
Eric Blake -
Laine Stump -
Peter Krempa