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! -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list