Re: [libvirt] [PATCH] qemu: allocate network connections sooner during domain startup
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