1. Patchew
  2. Libvirt
  3. View series

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

Roland Schulz posted 1 patch 5 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180910143059.11167-1-schullzroll@gmail.com
Test syntax-check passed
There is a newer version of this series
src/qemu/qemu_command.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
[libvirt] [PATCH] qemu: check for vhostusers bandwidth
Posted by Roland Schulz 5 years, 7 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: check for vhostusers bandwidth
Posted by Peter Krempa 5 years, 7 months ago 
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@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: check for vhostusers bandwidth
Posted by Michal Privoznik 5 years, 7 months ago 
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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: check for vhostusers bandwidth
Posted by Peter Krempa 5 years, 7 months ago 
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@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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.