[libvirt] [PATCH 2/2] bridge: don't crash on bandwidth unplug with no bandwidth

[Michal Privoznik]

On 27.06.2013 11:56, Ján Tomko wrote:
> On 06/27/2013 09:54 AM, Michal Privoznik wrote:
>> On 21.06.2013 19:30, Ján Tomko wrote:
>>> If networkUnplugBandwidth is called on a network which has
>>> no bandwidth defined, print a warning instead of crashing.
>>>
>>> This can happen when destroying a domain with bandwith if
>>> bandwidth was removed from the network after the domain was
>>> started.
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=975359
>>> ---
>>>  src/network/bridge_driver.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>>> index f7c2470..72a3f70 100644
>>> --- a/src/network/bridge_driver.c
>>> +++ b/src/network/bridge_driver.c
>>> @@ -4808,6 +4808,11 @@ networkUnplugBandwidth(virNetworkObjPtr net,
>>>  
>>>      if (iface->data.network.actual &&
>>>          iface->data.network.actual->class_id) {
>>> +        if (!net->def->bandwidth || !net->def->bandwidth->in) {
>>> +            VIR_WARN("Network %s has no bandwidth but unplug requested",
>>> +                     net->def->name);
>>> +            goto cleanup;
>>> +        }
>>>          /* we must remove class from bridge */
>>>          new_rate = net->def->bandwidth->in->average;
>>>  
>>>
>>
>> So the problem is, if user starts a domain with interface which has
>> @floor set. The @floor requires a bandwidth to be set on the network the
>> interface is to be plugged into. Then he destroys the network, clear
>> <bandwidth/> from the network definition and starts the network again. I
>> think this is the place which should be fixed. We should deny removing
>> <bandwidth/> in case there's at least one interface attached with
>> @floor. Similarly, we refuse to start domain if the corresponding
>> network doesn't have any <bandwidth/> configured.
> 
> We refuse to start domains when the network isn't active too, even if they
> don't have bandwidth, yet we allow the networks to be shut down afterwards.
> 
> I don't think we want to forbid shutting networks with bandwidth down (which
> essentially removes all the bandwidth from it).
> 
> Jan

Right. Good point, if user wants to shoot himself in the foot, we
shouldn't disallow him to do that (there's just too much of cases which
we would have to check). Libvirt's not foolproof. What we should do - or
rather not do is SIGSEGV-ing if user pull the trigger.

ACK then

Michal