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

[Erik Skultety]

On 10/01/2014 05:39 PM, John Ferlan wrote:
>
>
> 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...
>

Hmm, I think this last thing you mentioned won't be necessary at all, if 
you further inspect virNetDevBandwidthCopy(), we're passing by reference 
and the second action that takes place is assigning NULL to destination 
pointer. Sent v2 series.

Erik
> 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
>>
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>