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

[Laine Stump <laine@xxxxxxxxx>]

On 01/14/2011 08:22 AM, Daniel P. Berrange wrote:
> On Thu, Jan 13, 2011 at 01:45:28AM -0500, 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,
> > but am including it here just to get the patches on the list. I could
> > live with it this way, or with any of the following (anyone have a
> > strong opinion?) (note that in all cases, nothing specified means "try
> > to use vhost, but fallback to userland if necessary")
> >
> >     vhost='on|off'
> >     vhost='required|disabled'
> >     mode='vhost|qemu'
> >     mode='kernel|user'
> >     backend='kernel|user'
> I wanted 'name=' becasue that matches<driver>  usage in<disk>.
> This rules out the first options completely, since we can just
> play with attribute values. kernel|user is nice, except when
> QEMU invent  vhost2, and now 'kernel' is no longer unique :-(
> Also we used 'name=qemu' already in<disk>  to refer to the
> in-QEMU disk backend, and there's likely to be a 'vhost' backend
> for disks too in the future. So in summary I think 'name=vhost|qemu'
> is the best optionl.


Okay, I'm convinced! :-)


> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index b4df38c..04ed502 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -184,6 +184,10 @@ VIR_ENUM_IMPL(virDomainNet, VIR_DOMAIN_NET_TYPE_LAST,
> >                 "internal",
> >                 "direct")
> >
> > +VIR_ENUM_IMPL(virDomainNetBackend, VIR_DOMAIN_NET_BACKEND_TYPE_LAST,
> > +              "qemu",
> > +              "vhost")
> > +
> >   VIR_ENUM_IMPL(virDomainChrChannelTarget,
> >                 VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_LAST,
> >                 "guestfwd",
> > @@ -2289,6 +2293,7 @@ virDomainNetDefParseXML(virCapsPtr caps,
> >       char *address = NULL;
> >       char *port = NULL;
> >       char *model = NULL;
> > +    char *backend = NULL;
> >       char *filter = NULL;
> >       char *internal = NULL;
> >       char *devaddr = NULL;
> > @@ -2371,6 +2376,8 @@ virDomainNetDefParseXML(virCapsPtr caps,
> >                   script = virXMLPropString(cur, "path");
> >               } else if (xmlStrEqual (cur->name, BAD_CAST "model")) {
> >                   model = virXMLPropString(cur, "type");
> > +            } else if (xmlStrEqual (cur->name, BAD_CAST "driver")) {
> > +                backend = virXMLPropString(cur, "name");
> >               } else if (xmlStrEqual (cur->name, BAD_CAST "filterref")) {
> >                   filter = virXMLPropString(cur, "filter");
> >                   VIR_FREE(filterparams);
> > @@ -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"),
> > +                                 backend);
> > +            goto error;
> > +        }
> > +        def->backend = b;
> > +        def->backend_specified = 1;
> > +    }
> >       if (filter != NULL) {
> >           switch (def->type) {
> >           case VIR_DOMAIN_NET_TYPE_ETHERNET:
> > @@ -2584,6 +2603,7 @@ cleanup:
> >       VIR_FREE(script);
> >       VIR_FREE(bridge);
> >       VIR_FREE(model);
> > +    VIR_FREE(backend);
> >       VIR_FREE(filter);
> >       VIR_FREE(type);
> >       VIR_FREE(internal);
> > @@ -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) {
> > +            virBufferVSprintf(buf, "<driver name='%s'/>\n",
> > +                              virDomainNetBackendTypeToString(def->backend));
> > +        }
> > +    }
> >       if (def->filter) {
> >           virBufferEscapeString(buf, "<filterref filter='%s'",
> >                                 def->filter);
> > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> > index a459a22..451ccad 100644
> > --- a/src/conf/domain_conf.h
> > +++ b/src/conf/domain_conf.h
> > @@ -292,6 +292,13 @@ enum virDomainNetType {
> >       VIR_DOMAIN_NET_TYPE_LAST,
> >   };
> >
> > +/* the backend driver used for virtio interfaces */
> > +enum virDomainNetBackendType {
> > +    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;
> I don't really like this pattern since it is at odds with the way
> we handle defaults elsewhere which is to include the default as
> the first enum value, eg
>
>    enum virDomainNetBackendType {
>      VIR_DOMAIN_NET_BACKEND_TYPE_DEFAULT,      /* Try best available */
>      VIR_DOMAIN_NET_BACKEND_TYPE_QEMU,         /* userland */
>      VIR_DOMAIN_NET_BACKEND_TYPE_VHOST,        /* kernel */
>
>      VIR_DOMAIN_NET_BACKEND_TYPE_LAST,
>    };
>
> And then we do
>
>    if (def->backend)
>       ...print XML...
>
> when formatting the XML, so that the 'name=default' never
> actually appears in the XML output.

Right. What I didn't like was that, using the VIR_ENUM_IMPL() macro, we 
would end up giving the user the ability to put <driver name='default'/> 
into the XML, which would then disappear. In his review, Eric pointed 
out the elegant solution of simply generating an error during parse if 
they try to enter that value; everything else will already do what we 
want. So that's what I'm doing in V2.

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list