Re: [libvirt] [PATCH 2/2] conf: Introduce the virDomainNetModel enumeration

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.

If not, then I'll probably not attempt to push this forward.

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.