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!