Vhostuser doesn't support bandwidth and due to backwards compatibility
it was decided to just warn users instead of erroring out

https://bugzilla.redhat.com/show_bug.cgi?id=1524230

Signed-off-by: Roland Schulz <schullzroll@gmail.com>
---
 src/qemu/qemu_command.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ff9589f593..011e2b45af 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,13 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver,
         goto cleanup;
     }
 
+    /* Warn if unsupported bandwidth requested */
+    if (actualBandwidth && !virNetDevSupportBandwidth(actualType)) {
+            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

I think the subject needs to be changed, to:

"qemu: Alter logic for interface command line bandwidth warning"

On 09/11/2018 05:13 AM, Roland Schulz wrote:
> Vhostuser doesn't support bandwidth and due to backwards compatibility
> it was decided to just warn users instead of erroring out

Since it was brought up in review, go look at Michal's commit 04cf99a6b
where qemuBuildInterfaceCommandLine was modified to generate a warning
message:

+    /* Set bandwidth or warn if requested and not supported. */
+    actualBandwidth = virDomainNetGetActualBandwidth(net);
+    if (actualBandwidth) {
+        if (virNetDevSupportBandwidth(actualType)) {
+            if (virNetDevBandwidthSet(net->ifname, actualBandwidth,
false) < 0)
+                goto cleanup;
+        } else {
+            VIR_WARN("setting bandwidth on interfaces of "
+                     "type '%s' is not implemented yet",
+                     virDomainNetTypeToString(actualType));
+        }
+    }

I would seem that the message and warning was generated "early enough"
so that it should have worked properly; however, closer inspection of
that change would note that the call to qemuBuildVhostuserCommandLine
would never even reach that.

Then *lots* of interceding changes (too many to note) altered the logic,
to move switches and other things in front of that check, but still the
vhostuser would avoid it.

So what to do? Well rather than duplicate code and since it's "just" a
warning, we could alter the code as follows:

Before the switch for actual type where we end up building the command
line or calling  qemuInterface*Connect, we have something like:

    /* Check and warn now if requested, but not supported in case the
     * subsequent switch jumps to cleanup */
    actualBandwidth = virDomainNetGetActualBandwidth(net);
    if (actualBandwidth && !virNetDevSupportBandwidth(actualType)) {
        VIR_WARN("setting bandwidth on interfaces of "
                 "type '%s' is not implemented yet",
                 virDomainNetTypeToString(actualType));
        actualBandwidth = NULL;
    }

and alter the current code to be (removing the comment since it's
obvious we're setting...)

    if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false,
                              !virDomainNetTypeSharesHostView(net)) < 0)
        goto cleanup;

NB: If you look at virNetDevBandwidthSet you'll note:

    if (!bandwidth) {
        /* nothing to be enabled */
        ret = 0;
        goto cleanup;
    }

so it does nothing and we don't need any "if (actualBandwidth &&" guard.

The "downside" to this?

Well for any actualType that fails virNetDevSupportBandwidth, but would
otherwise fall through to the existing code, we'll now get a warning.
Fortunately, limited to just VIR_DOMAIN_NET_TYPE_HOSTDEV.

BTW: When you generate your next patch, please reference Michal's commit
and of course an abbreviated version of the above to essentially state
that we want to warn on improper bandwidth prior to calls that may jump
to cleanup before the existing check/warning.

> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1524230
> 
> Signed-off-by: Roland Schulz <schullzroll@gmail.com>
> ---
>  src/qemu/qemu_command.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ff9589f593..011e2b45af 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,13 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver,
>          goto cleanup;
>      }
>  
> +    /* Warn if unsupported bandwidth requested */
> +    if (actualBandwidth && !virNetDevSupportBandwidth(actualType)) {
> +            VIR_WARN(_("setting bandwidth on interfaces of "
> +                     "type '%s' is not implemented yet"),
> +                     virDomainNetTypeToString(actualType));
> +    }
> +

FWIW:

1. The indention is off - 4 spaces after the "if", thus the VIR_WARN
would fall under the "a" in actual.

2. If you had run make check syntax-check you would have been told:

prohibit_gettext_markup
src/qemu/qemu_command.c:8252:            VIR_WARN(_("setting bandwidth
on interfaces of "
maint.mk: do not mark these strings for translation
make: *** [cfg.mk:776: sc_prohibit_gettext_markup] Error 1

John

>      switch ((virDomainChrType)net->data.vhostuser->type) {
>      case VIR_DOMAIN_CHR_TYPE_UNIX:
>          if (!(chardev = qemuBuildChrChardevStr(logManager, secManager,
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list