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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]