[libvirt] [PATCH] qemu: check for vhostusers bandwidth

[Michal Privoznik]

On 09/10/2018 05:06 PM, Peter Krempa wrote:
> On Mon, Sep 10, 2018 at 16:30:59 +0200, Roland Schulz wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1524230
> 
> Please describe your change in the commit message. A bugzilla may not
> give enough reasoning for it.
> 
>>
>> Signed-off-by: Roland Schulz <schullzroll at gmail.com>
>> ---
>>  src/qemu/qemu_command.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index ff9589f593..284c2709fc 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -8244,6 +8244,8 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver,
>>                                virQEMUCapsPtr qemuCaps,
>>                                unsigned int bootindex)
>>  {
>> +    virNetDevBandwidthPtr actualBandwidth = virDomainNetGetActualBandwidth(net);
>> +    virDomainNetType actualType = virDomainNetGetActualType(net);
>>      virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>>      char *chardev = NULL;
>>      char *netdev = NULL;
>> @@ -8257,6 +8259,19 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver,
>>          goto cleanup;
>>      }
>>  
>> +    /* Set bandwidth or warn if requested and not supported. */
>> +    if (actualBandwidth) {
>> +        if (virNetDevSupportBandwidth(actualType)) {
>> +            if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false,
>> +                                      !virDomainNetTypeSharesHostView(net)) < 0)
>> +                goto cleanup;
> 
> This is a very convoluted dead branch.
> 
> qemuBuildVhostuserCommandLine gets called only when
> actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER and virNetDevSupportBandwidth
> returns false for that value.
> 
>> +        } else {
>> +            VIR_WARN("setting bandwidth on interfaces of "
>> +                     "type '%s' is not implemented yet",
>> +                     virDomainNetTypeToString(actualType));
> 
> Reporting a warning is almost pointless. It only gets logged but the
> user does not get notified. Is this a hard failure where we can error
> out?

I did send a patch for that quite a while ago:

https://www.redhat.com/archives/libvir-list/2015-January/msg00171.html

and it was decided to just warn users instead of throwing and error and
denying starting such domain.

I don't mind revisiting that decision though. But we have backward
compatibility to bear in mind.

Michal