Re: [libvirt] [PATCH] Add XML config switch to enable/disable vhost-net support

On 01/12/2011 11:45 PM, Laine Stump wrote: > This patch is in response to > > https://bugzilla.redhat.com/show_bug.cgi?id=643050 > > The existing libvirt support for the vhost-net backend to the virtio > network driver happens automatically - if the vhost-net device is > available, it is always enabled, otherwise the standard userland > virtio backend is used. > > This patch makes it possible to force whether or not vhost-net is used > with a bit of XML. Adding a<driver> element to the interface XML, eg: > > <interface type="network"> > <model type="virtio"/> > <driver name="vhost"/> > > will force use of vhost-net (if it's not available, the domain will > fail to start). if driver name="qemu", vhost-net will not be used even > if it is available. > > If there is no<driver name='xxx'/> in the config, libvirt will revert > to the pre-existing automatic behavior - use vhost-net if it's > available, and userland backend if vhost-net isn't available. > --- > > Note that I don't really like the "name='vhost|qemu'" nomenclature, ... > (So far the strongest opinion has been for the current "name='vhost|qemu'") Also, using name= as the attribute name matches the fact that we already have<driver name='xyz'> for<disk>.

> diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng > index a524e4b..6d0654d 100644 > --- a/docs/schemas/domain.rng > +++ b/docs/schemas/domain.rng > @@ -1005,6 +1005,19 @@ > </element> > </optional> > <optional> > +<element name="driver"> > +<optional> > +<attribute name="name"> Thinking aloud: This permits<driver/> with no attributes, which looks weird. For comparison with the<disk> element, the rng requires the driver element has to have at least one attribute from the set (name, cache), which means name is optional there. Which means on the one hand,<disk> will never have<driver/> without attributes, but on the other hand, why should name be mandatory here if it is optional there. I guess that means this is fine to keep the<optional> here.

> @@ -2558,6 +2565,18 @@ virDomainNetDefParseXML(virCapsPtr caps, > model = NULL; > } > > + if ((backend != NULL)&& > + (def->model&& STREQ(def->model, "virtio"))) { > + int b; > + if ((b = virDomainNetBackendTypeFromString(backend))< 0) { > + virDomainReportError(VIR_ERR_INTERNAL_ERROR, > + _("Unkown interface<driver name='%s'> has been specified"), s/Unkown/Unknown/

> + backend); > + goto error; > + } > + def->backend = b; > + def->backend_specified = 1; It seems like our debate on IRC has been whether this two-variable tracking is better than a three-way enum value. If we go with the three-way enum (default, qemu, vhost), then you only need one variable, but this line of code needs one extra check that rejects STREQ(backend,"default") (or else the XML parsing would silently accept name='default' but discard it).

> + *vhostfd = open("/dev/vhost-net", O_RDWR, 0); The third argument is not necessary, since the second does not include O_CREAT.

I can live with the patch as-is (once you fix the spelling and open() nits), but if you have time, I wouldn't mind a v2 with the approach of a tri-state enum. So, you have a weak: