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

[Laine Stump]

On 01/13/2011 03:59 PM, Eric Blake wrote:
> 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>.

Yep, I'm in the club :-)

>
>> 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.


And especially since the rng is only used in tests that validate the 
rng, I'm inclined to leave it optional...


>> @@ -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/


Right. I think this is the result of the copy-paste source I chose 
(search for "Unkown"). I need to send a patch for that.


>> +                                 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).

Good idea. I chose to generate an error on name='default'. Shortens up 
the code, eliminates non-zero default; an all around win!


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

Okay. That's actually pre-existing code that I moved slightly, but I'll 
use this oppurtunity to fix it.


> 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:

V2 coming up!