[libvirt] [PATCH 1/2] qemu: Fix updating bandwidth limits in live XML

[John Ferlan]

On 10/01/2014 11:17 AM, Erik Skultety wrote:
> On 10/01/2014 01:55 AM, John Ferlan wrote:
>>
>>
>> On 09/22/2014 06:41 AM, Erik Skultety wrote:
>>> When trying to update bandwidth limits on a running domain, limits get
>>> updated in our internal structures, however XML parser reads
>>> bandwidth limits from network 'actual' definition. Commiting this patch
>>
>> s/Commiting/Committing
>>
>>> it is now available to update bandwidth 'actual' definition as well,
>>> thus updating domain runtime XML
>>> ---
>>>   src/qemu/qemu_driver.c | 12 ++++++++++++
>>>   1 file changed, 12 insertions(+)
>>>
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index 702d3cc..ede8880 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -10028,7 +10028,19 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
>>>           } else {
>>>               net->bandwidth = NULL;
>>>           }
>>> +
>>> +        if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
>>> +                virNetDevBandwidthFree(net->data.network.actual->bandwidth);
>>
>> This will set net->data.network.actual->bandwidth to NULL
>>
>> It's also remove it when net->bandwidth == NULL thus
>> causing actual to be lost, which it doesn't seem is
>> desired, but perhaps it is.  Gues
>>
>>> +                if (!net->bandwidth ||
>>> +                    virNetDevBandwidthCopy(&net->data.network.actual->bandwidth,
>>> +                                           net->bandwidth) < 0)
>>> +                    net->data.network.actual->bandwidth = NULL;
>>
>> Making this irrelevant, but I wonder if the < 0 here meant to do
>> something else perhaps?
>>
>>> +        }
>>
>> The above hunk needs some space formatting.  Also since the
>> virNetDevBandwidthCopy() has a if (!src) check, "(!net->bandwidth) ||"
>> is unnecessary.
>>
>> if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
>>      virNetDevBandwidthFree(net->data.network.actual->bandwidth);
>>      if (virNetDevBandwidthCopy(&net->data.network.actual->bandwidth,
>>                                 net->bandwidth) < 0)
>>          goto cleanup
>> }
>>
> Looks better, thanks Jon, but you'd still loose 'actual' in the hunk 
> above, maybe add a check like
> 
> if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
>      net->bandwidth)
> 
> if you want to preserve 'actual' when net->bandwidth is NULL??

Right - I thought about that too, but then I thought the purpose of
making the change to actual was to copy what happened to net->bandwidth.

Looking at the hunk just above:

        virNetDevBandwidthFree(net->bandwidth);
        if (newBandwidth->in || newBandwidth->out) {
            net->bandwidth = newBandwidth;
            newBandwidth = NULL;
        } else {
            net->bandwidth = NULL;
        }

That says to me in this live path, we're freeing net->bandwidth and only
replacing it with 'newBandwidth' if in/out were found.

Thus, it seems reasonable that we'd want to remove the bandwidth from
actual in this case as well which we wouldn't do if the the "&&
net->bandwidth" was added to the condition.

Also, upon further reflection the "net->data.network.actual->bandwidth =
NULL;" after the virNetDevBandwidthFree() will be necessary since we're
passing by value and not reference...

John


>> Whether this is what is "expected" is perhaps something Laine can answer...
>>
>> John
>>
>>> +
>>> +        if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
>>> +            goto cleanup;
>>>       }
>>> +
>>>       if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
>>>           if (!persistentNet->bandwidth) {
>>>               persistentNet->bandwidth = bandwidth;
>>>
>>
>> --
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
> 
> Erik
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>