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

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(a)gmail.com&gt; >> --- >> 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.