On Thu, Jul 27, 2017 at 13:05:16 +0200, Andrea Bolognani wrote:
> On Wed, 2017-07-26 at 18:21 +0200, Peter Krempa wrote:
> > > @@ -10330,18 +10349,20 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> > > * i82551 i82557b i82559er ne2k_pci pcnet rtl8139 e1000 virtio
> > > * QEMU PPC64 supports spapr-vlan
> > > */
> > > - if (model != NULL) {
> > > - if (strspn(model, NET_MODEL_CHARS) < strlen(model)) {
> > > - virReportError(VIR_ERR_INVALID_ARG, "%s",
> > > - _("Model name contains invalid characters"));
> > > + if (model) {
> > > + if ((val = virDomainNetModelTypeFromString(model)) < 0 ||
> > > + val == VIR_DOMAIN_NET_MODEL_NONE) {
> > > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > > + _("Unknown interface <model type='%s'> "
> > > + "has been specified"),
> > > + model);
> > > goto error;
> >
> > So this is the infamous case, when we will drop configs which were
> > previously accepted. I'm not entirely persuaded that this is a great
> > idea, since we usually try to avoid this at all costs.
> >
> > That's most probably also the reason why nobody bothered to change it
> > yet.
> >
> > I think you'll need to store the original string in case when it can't
> > be parsed and use that one and accept such definition even in case when
> > the model is unknown.
> >
> > Thanks to the validation callback you can then make sure that drivers
> > accept only values which can be parsed and users still have an option to
> > edit the definition and replace it.
> >
> > Losing it is not acceptable.
> >
> > NACK to full removal of the string.
>
> Would you still be against it if the enumeration was extended
> to include every NIC model ever supported by QEMU and Xen? As
> mentioned, other drivers already perform their own validation
> and only accept a very limited number of models, so there's
> no risk of regression there.
I don't think that will help. It will prevent missing any working
configuration, but it will still make any invalid configuration vanish.
Unfortunately, restricting the list of accepted values can't be done
carelessly.
> If not, then I'll probably not attempt to push this forward.
You know, that's the reason why it stayed as is until now :).
> Your suggestion to keep storing the raw string and use it
> during validation would be workable, but throwing together
> a quick PoC confirmed my initial impression that it would not
> result in a clear win, if anything because of the additional
> logic making everything less straightforward.
I think the validation part still might make sense, even while we will
still store it as a string. The validation callback may at least make
sure that from this point, only sane strings will be put into the XML.
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
