Re: [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.