On 06/24/2016 11:19 AM, Laine Stump wrote:
> 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).
>
That's fine - keep it here.
>
>>
[...]
>>> + /* 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
>
By patch 25 I'm surprised I even noticed it.
Explanation accepted - ACK
John
Anyone less resistant to change can always move it later.
>
>> 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.
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list