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>. > > Oh, and also - sorry Eric, but I didn't have the brain cells left > tonight to add this new bit to the documentation, and I really want to > get the patch up/in now, so that will have to wait for a followup next > week :-) I'll have to live with that, but come closer to the 0.8.8 release date, watch for the reminders if you haven't followed through :) Meanwhile, https://bugzilla.redhat.com/show_bug.cgi?id=643050#c3 hints that we may yet need to make an explicit third state (particularly if virsh dumpxml always lists the chosen driver rather than staying silent in the default case) - it's easier to add a third state later than it is to wish we hadn't added it up front, so I'm glad that your initial patch only proposes two valid strings in the XML. > 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). [for comparison with your sndbuf patch - there, you have to use sndbuf_specified, since sndbuf==0 is a valid but non-default configuration. But here, using a three-state enum provides a perfect default without having to track two variables, at the cost of having to check at parse time that only two of the three states can be explicitly used, because the default third state happens by the absence of an explicit request.] > @@ -6275,9 +6295,14 @@ virDomainNetDefFormat(virBufferPtr buf, > if (def->ifname) > virBufferEscapeString(buf, " <target dev='%s'/>\n", > def->ifname); > - if (def->model) > + if (def->model) { > virBufferEscapeString(buf, " <model type='%s'/>\n", > def->model); > + if (STREQ(def->model, "virtio") && def->backend_specified) { with a tri-state enum, here, the condition would be def->backend != VIR_DOMAIN_NET_BACKEND_TYPE_DEFAULT > > +/* the backend driver used for virtio interfaces */ > +enum virDomainNetBackendType { and here, you would have VIR_DOMAIN_NET_BACKEND_TYPE_DEFAULT = 0, /* try kernel, but fall back to userland */ > + VIR_DOMAIN_NET_BACKEND_TYPE_QEMU, /* userland */ > + VIR_DOMAIN_NET_BACKEND_TYPE_VHOST, /* kernel */ > + > + VIR_DOMAIN_NET_BACKEND_TYPE_LAST, > +}; > > /* the mode type for macvtap devices */ > enum virDomainNetdevMacvtapType { > @@ -310,6 +317,8 @@ struct _virDomainNetDef { > enum virDomainNetType type; > unsigned char mac[VIR_MAC_BUFLEN]; > char *model; > + enum virDomainNetBackendType backend; > + int backend_specified : 1; and here you could lose backend_specified. I guess another advantage of keeping a tri-state enum rather than two variables is that if the request to have an explicit name='default' ever materializes in the future, we would already have the enum set up for that. > + > + *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: ACK. -- Eric Blake eblake@xxxxxxxxxx +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list