On Wed, Jul 26, 2017 at 14:20:05 +0200, Andrea Bolognani wrote:
> Up until now, we have stored the model name for network
> interfaces as raw strings in virDomainNetDef: this is
> suboptimal for a number of reasons, such as having to
> perform relatively expensive string allocations and
> comparisons all the time and not giving the compiler
> the opportunity to have our back in certain situations.
>
> Replace the strings with an enumeration similar to the
> ones we already use for pretty much everything else.
>
> Signed-off-by: Andrea Bolognani <abologna@xxxxxxxxxx>
> ---
> Most drivers already performed pretty strict validation
> of the model name, so there should be no problems there;
> the QEMU driver, however, though it would be a good idea
> to just accept any string that possibly kinda resembled
> a valid model name.
>
> The models I've included in the enumeration are those
> that were already referenced somewhere else in libvirt,
> but there's no guarantee that other model names are not
> used in the wild.
>
> One possibility would be to also add (a subset of) all
> models QEMU ever supported, even though some of them
> might have never been used, just to be safe; on the
> other side of the spectrum, we could go with the minimal
> set and possibly add more if breakages are reported.
I don't think that upstream would ever consider the last option.
> @@ -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.
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
