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

[Peter Krempa]

On Tue, Sep 11, 2018 at 09:10:43 +0200, Michal Privoznik wrote:
> 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.

That is the stuff that should have been in the commit message in the
first place.

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

Actually I don't mind it being a warning if it is justified e.g. by not
wanting to break existing configs.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180911/5ab05059/attachment-0001.sig>