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: 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. 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