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

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.

>> 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.