Re: [libvirt] [PATCH] qemu: allocate network connections sooner during domain startup
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.
> + 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.
Thanks for reviewing!