On 12/11/2014 02:18 PM, Matthew Rosato wrote:
> On 12/11/2014 01:35 PM, Laine Stump wrote:
>> On 09/16/2014 04:50 PM, Matthew Rosato wrote:
>>>
>>> #include "cpu/cpu.h"
>>> #include "datatypes.h"
>>> @@ -2947,6 +2948,12 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm,
>>> qemuDomainObjPrivatePtr priv = vm->privateData;
>>> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>>>
>>> + /* Bring up netdevs before starting CPUs */
>>> + if (reason != VIR_DOMAIN_RUNNING_UNPAUSED &&
>>> + reason != VIR_DOMAIN_RUNNING_SAVE_CANCELED) {
>>> + qemuInterfaceStartDevices(vm->def);
>>> + }
>> Matthew,
>>
>>
>> I've already pushed your patch, but am trying to add to it for another
>> related purpose and this bit is bothering me. What is the reason for not
>> calling qemuInterfaceStartDevices() when reason is
>> VIR_DOMAIN_RUNNING_UNPAUSED or VIR_DOMAIN_RUNNING_SAVE_CANCELED? Is it
>> just to avoid setting IFF_UP on an interface that is already IFF_UP?
>>
>> If that's the only reason, I would prefer to have it *always* called
>> when the CPUs are started (and a corresponding
>> qemuInterfaceStopDevices() called when the CPUs are stopped). Otherwise,
>> it looks to me like it is possible in some situations (migration error
>> recovery perhaps? search for VIR_DOMAIN_RUNNING_UNPAUSED) to have the
>> CPUs running, but the macvtap interfaces *not* IFF_UP.
>>
>> Do you see (or did you experience?) a problem calling
>> qemuInterfaceStartDevices() for all reasons?
>>
> Hi Laine,
>
> I did not experience any problems calling qemuInterfaceStartDevices()
> unconditionally, that's actually how I originally wrote the code -- I
> added these conditional statements based on a review comment to avoid
> unnecessary IFF_UPs.
>
> I'd be fine with the call being unconditional again.
Okay. I've gone back through the archives of the 3 versions of the patch
and understand the origin. I've also tested out pausing/unpausing a
guest with a macvtap interface when this extra condition is removed, and
networking continues to work with no problems. Since there is no
specific experienced reason for this restriction on when to call
qemuInterface StartDevices() (but rather was just you making reviewers
happy :-)), I'm going to send a patch to remove that conditional, then
another patch to add qemuInterfaceStopDevices(), and finally one that
adds/removes bridge fdb entries for tap devices when the network has
macTableManager='libvirt'.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list