[libvirt] [PATCH v4 03/29] conf: don't pass interface type into virNetDevBandwidthParse

[Laine Stump]

On 4/18/19 6:32 AM, Daniel P. Berrangé wrote:
> On Wed, Apr 17, 2019 at 04:33:54PM -0400, Laine Stump wrote:
>> On 4/17/19 1:19 PM, Daniel P. Berrangé wrote:
>>> The virNetDevBandwidthParse method uses the interface type to decide
>>> whether to allow use of the "floor" parameter. Using the interface
>>> type is not convenient as callers may not have that available, but
>>> still wish to allow use of "floor".
>>
>> Also, this is one of the things that gave rise to the distinction between
>> actualType == NETWORK and actualType == BRIDGE, and this patch allows us to
>> eliminate that (hopefully) without causing breakage.
> Well sort of.  If using a plain bridge virtual network you can't
> use floor - that's only valid for routed networks. Trying to enforce
> this at parse time is doomed though - you can only corrrectly report
> this at runtime as you need the context of the virtual network itself.
>
> This was already an issue when parsing the top level netdef - only
> the actual netdef had any better checking.
>
> I'm almost inclined to remove all the logic preventing "floor" at
> XML parse time and rely on runtime checks entirely.


Except that the same parse function is used for the <bandwidth> element 
in <network>, which never allows floor to be set. For the <bandwidth> in 
an interface though, you're correct that it's impossible to know beforehand.


>
>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index 51aa48f421..0df3c2ed49 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -11270,7 +11270,7 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
>>>        if (bandwidth_node &&
>>>            virNetDevBandwidthParse(&actual->bandwidth,
>>>                                    bandwidth_node,
>>> -                                actual->type) < 0)
>>> +                                def->type == VIR_DOMAIN_NET_TYPE_NETWORK) < 0)
>>
>> This bit here ^^^ doesn't compile, since def is undefined. You would need to
>> check "parent->type" instead.
> Should be "actual->type" in fact.


I was thinking in terms of making it still correct in the "new world 
order" where actual->type will always be bridge rather than network. But 
then later I saw that this line is removed in the next patch anyway, so 
it's kind of irrelevant - for the state of everything at the point of 
*this* patch, the two are equivalent.