[libvirt] [PATCH] qemu: allocate network connections sooner during domain startup

[Michal Privoznik]

On 07.05.2013 16:47, Laine Stump wrote:
> On 05/07/2013 05:52 AM, Michal Privoznik wrote:
>> On 06.05.2013 22:16, Laine Stump wrote:
>>>  
>> I'd expect a one line comment here at least to give a reason why we are
>> skipping VIR_DOMAIN_NET_TYPE_HOSTDEV. Something like:
>> /* hostdev interfaces already handled by qemuNetworkPrepareDevices */
>>
>> It's obvious now, but if we get later to this code it won't be IMO.
> 
> 
> Makes sense. The dual nature of these device does tend to be confusing.
> 
> 
> 
>>> +            if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV)
>>> +                continue;
>>> +
>>>              /* VLANs are not used with -netdev, so don't record them */
>>>              if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) &&
>>>                  virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))
>> I actually posted a patch which moves the whole block into a separate
>> function, which is what we should do:
>>
>> https://www.redhat.com/archives/libvir-list/2013-April/msg01550.html
> 
> 
> Ah. I missed that whole series while I was busy trying to get VFIO
> working before the 1.0.5 freeze. I should go back now and review it.
> 
> 
> 
>>> +    /* network devices must be "prepared" before hostdevs, because
>>> +     * setting up a network device might create a new hostdev that
>>> +     * will need to be setup.
>>> +     */
>>> +    VIR_DEBUG("Preparing network devices");
>> Is it worth mentioning s/network/network host/?
> 
> 
> My intent is the qemuNetworkPrepareDevices() will eventually handle the
> setup required for *all* guest network devices, not just those that are
> passed-through host devices. The only reason I didn't do it that was
> right away is because the fd's for tap and vhost-net devices are
> required in qemuBuildCommandLine(), so we need to decide on the best
> place to stow those away where qemuBuildCommandLine() can retrieve them.

Right, I don't feel comfortable allocating resources in
qemuBuildCommandLine neither. So maybe my patch set becomes outdated
after all. However, if we are gonna move all the allocation code out of
the qemuBuildCommandLine we should remember now, that there are gonna be
several FDs being passed (multiple for /dev/net/tun and for /dev/vhost-net).

> 
> 
>>> +    if (qemuNetworkPrepareDevices(vm->def) < 0)
>>> +       goto cleanup;
>>> +
>>>      /* Must be run before security labelling */
>>>      VIR_DEBUG("Preparing host devices");
>>>      if (qemuPrepareHostDevices(driver, vm->def, !migrateFrom) < 0)
>>>
>> ACK if you add the comment. The debug message fix is optional.
> 
> 
> Okay. I'm going to add the comment. I think the debug message is proper
> as-is, for the reason I give above.

Yep, now it makes perfect sense to not alter the debug message.

> 
> Thanks for reviewing!

You're welcome.

Michal