[libvirt] [PATCH] qemu_hotplug: Allow QoS update in qemuDomainChangeNet

[Laine Stump]

On 10/01/2013 09:07 AM, Michal Privoznik wrote:
> The qemuDomainChangeNet() is called when 'virsh update-device' is
> invoked on a NIC. Currently, we fail to update the QoS even though
> we have routines for that.
>
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_hotplug.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index f06930e..41b942f 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2062,8 +2062,6 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
>          virDomainNetGetActualDirectMode(olddev) != virDomainNetGetActualDirectMode(olddev) ||
>          !virNetDevVPortProfileEqual(virDomainNetGetActualVirtPortProfile(olddev),
>                                      virDomainNetGetActualVirtPortProfile(newdev)) ||
> -        !virNetDevBandwidthEqual(virDomainNetGetActualBandwidth(olddev),
> -                                 virDomainNetGetActualBandwidth(newdev)) ||
>          !virNetDevVlanEqual(virDomainNetGetActualVlan(olddev),
>                              virDomainNetGetActualVlan(newdev))) {
>          needReconnect = true;
> @@ -2081,6 +2079,18 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
>          goto cleanup;
>      }
>  
> +    if (!virNetDevBandwidthEqual(virDomainNetGetActualBandwidth(olddev),
> +                                 virDomainNetGetActualBandwidth(newdev))) {
> +        if (virNetDevBandwidthSet(newdev->ifname,
> +                                  virDomainNetGetActualBandwidth(newdev),
> +                                  false) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("cannot set bandwidth limits on %s"),
> +                           newdev->ifname);
> +            goto cleanup;
> +        }
> +        needReplaceDevDef = true;
> +    }
>      if (needBridgeChange) {
>          if (qemuDomainChangeNetBridge(dom->conn, vm, olddev, newdev) < 0)
>              goto cleanup;

qemuDomainChangeNet probably seems a bit mixed up, but the intent was to
first perform a bunch of checks and set flags as we determined actions
that need to be performed. Then, after all the flags have been set,
perform those actions. The reasoning for doing it this way is that in
some cases a larger scale action (e.g. unplug and replug the host side
of the interface, which isn't actually supported right now due to
deficiencies in qemu) would end up implicitly performing the smaller
scale actions (e.g. setting the bandwidth) anyway, thus rendering it
redundant to do those actions separately (so their boolean would be reset).

For that reason, I would prefer if a "needBandwidthSet" boolean was
added, and set to true when the old bandwidth doesn't equal the new (up
around line 2060), then down below (where you've added the new chunk in
this patch) you would just do "if (needBandwidthSet) &&
virNetDevBandwidthSet(...) < 0) { error } and set needReplaceDevDef.

I know that seems like just extra work and confusion for nothing, but if
we are ever able to replug the host-side of an interface, it will end up
being very useful.