[libvirt] [PATCH 4/4] network: backend for virNetworkUpdate of interface list

[Laine Stump]

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!