[libvirt] [PATCH] qemu: check for vhostusers bandwidth
https://bugzilla.redhat.com/show_bug.cgi?id=1524230 Signed-off-by: Roland Schulz <schullzroll@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; + } else { + VIR_WARN("setting bandwidth on interfaces of " + "type '%s' is not implemented yet", + virDomainNetTypeToString(actualType)); + } + } + switch ((virDomainChrType)net->data.vhostuser->type) { case VIR_DOMAIN_CHR_TYPE_UNIX: if (!(chardev = qemuBuildChrChardevStr(logManager, secManager, -- 2.17.1
On Mon, Sep 10, 2018 at 16:30:59 +0200, Roland Schulz wrote:
Please describe your change in the commit message. A bugzilla may not give enough reasoning for it.
Signed-off-by: Roland Schulz <schullzroll@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?
+ } + } + switch ((virDomainChrType)net->data.vhostuser->type) { case VIR_DOMAIN_CHR_TYPE_UNIX: if (!(chardev = qemuBuildChrChardevStr(logManager, secManager, -- 2.17.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 09/10/2018 05:06 PM, Peter Krempa wrote:
On Mon, Sep 10, 2018 at 16:30:59 +0200, Roland Schulz wrote:
Please describe your change in the commit message. A bugzilla may not give enough reasoning for it.
Signed-off-by: Roland Schulz <schullzroll@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
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:
Please describe your change in the commit message. A bugzilla may not give enough reasoning for it.
Signed-off-by: Roland Schulz <schullzroll@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.
participants (3)
-
Michal Privoznik -
Peter Krempa -
Roland Schulz