Re: [libvirt] [PATCH 25/28] conf: support host-side IP/route information in <interface>

On 06/24/2016 09:48 AM, John Ferlan wrote: >

>> +virDomainNetDefValidate(const virDomainNetDef *net) >> +{ >> + if ((net->hostIP.nroutes || net->hostIP.nips) && >> + net->type != VIR_DOMAIN_NET_TYPE_ETHERNET) { >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >> + _("Invalid attempt to set network interface " >> + "host-side IP route and/or address info on " >> + "interface of type '%s'. This is only >> supported " >> + "on interfaces of type 'ethernet'"), >> + virDomainNetTypeToString(net->type)); >> + return -1; >> + } >> + return 0; >> +} > It seems as though you are *adding* a new element - thus, this could not > be present on a currently running domain, so wouldn't the "more correct" > placement be the PostParse API's ? I don't think we're losing any functionality by putting it here (other than that if someone edits the config files directly on disk and then restart libvirtd they won't see an error; but then they'll be getting what they deserve). And I think its function fits better with the Validate function than with the PostParse callback (which admittedly does have some validating done in it, but that's just because the Validate callbacks are a fairly recent addition. In general, I think anything that is just validating the data in a domain as a whole should be done in the Validate callbacks, and things that modify the domain or devices should be in the postparse callbacks. Eventually pure validation should migrate, and for now at least new stuff should be added in that way. (Note that I still think that basic validation that doesn't require knowledge of any other part of the XML should still be done directly in the parse (numeric ranges, etc; basically anything that is described in the RNG).

>

>> + /* if sourceLines == 0 - no <source> info at all so far >> + * sourceLines == 1 - first line writte, no terminating ">" > s/writte/written > > > I think the Validate should be a PostParse - your thoughts... See above. I think you're just resistent to change :-P

> The > 'contents' of the change are ACKable, I just think the placement is a > bit off. Then of course there's the whole doing multiple things here > (there could conceivably be 3 patches out of this). > > I "assume" there are other XML2XML tests that ensure all the following > magic is correct since you added one for the new data... Yes. The unit tests caught problems with it initially, so they are there.