On Wed, Apr 11, 2018 at 07:20:46PM +0200, Andrea Bolognani wrote:
On Mon, 2018-04-09 at 17:20 +0200, Ján Tomko wrote:Now that we assume QEMU_CAPS_NETDEV, the only thing left to check is whether we need to use the legacy -net syntax because of a non-conforming armchitecture.I see you're having "pun" writing these commit messages ;) [...]diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 05cc4903a4..4e8c4a7bd4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8217,7 +8217,7 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver, unsigned int queues = net->driver.virtio.queues; char *nic = NULL; - if (!qemuDomainSupportsNetdev(def, qemuCaps, net)) { + if (!qemuDomainSupportsNicdev(def, net)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Netdev support unavailable"));With the change, this error message becomes misleading: it's not that -netdev support is unavailable, it's just that -device can't be used for the NIC and we can't (won't?) use -netdev without it. I guess you can just s/Netdev/nicdev/ and call it a day, it's not like it makes the error message any harder, or easier, to grasp.@@ -8552,23 +8552,14 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, goto cleanup; } - /* Possible combinations: - * - * 1. Old way: -net nic,model=e1000,vlan=1 -net tap,vlan=1 - * 2. Semi-new: -device e1000,vlan=1 -net tap,vlan=1 - * 3. Best way: -netdev type=tap,id=netdev1 -device e1000,id=netdev1 - * - * NB, no support for -netdev without use of -device - */I think you should leave the comment in, because most of it still applies even in our brave new, legacy-free world. Basically all you should do is drop option 2, and (optionally) rework option 3 a little. The result could look like * Old way: -net nic,model=e1000,vlan=1 -net tap,vlan=1 * New way: -device e1000,id=netdev1 -netdev type=tap,id=netdev1 I suggest reworking the "new way" line because I think having the guest part and host part listed in the same order both times makes the whole thing easier to understand.
I added a note instead of reworking, to keep them in command-line order: NB: The backend and frontend are reversed
- if (qemuDomainSupportsNetdev(def, qemuCaps, net)) { + if (qemuDomainSupportsNicdev(def, net)) { if (!(host = qemuBuildHostNetStr(net, driver, ',', vlan, tapfdName, tapfdSize, vhostfdName, vhostfdSize))) goto cleanup; virCommandAddArgList(cmd, "-netdev", host, NULL); - } - if (qemuDomainSupportsNicdev(def, net)) { + if (!(nic = qemuBuildNicDevStr(def, net, vlan, bootindex, vhostfdSize, qemuCaps))) goto cleanup; @@ -8658,7 +8648,7 @@ qemuBuildNetCommandLine(virQEMUDriverPtr driver, int vlan; /* VLANs are not used with -netdev, so don't record them */ - if (qemuDomainSupportsNetdev(def, qemuCaps, net)) + if (qemuDomainSupportsNicdev(def, net)) vlan = -1; else vlan = i;Again, you probably want to mention that -netdev requires -device, so that the comment won't look completely out of place or require developers to be intimately aware of how the two work together. [...]diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c145c42bcd..8aacd8376f 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -956,7 +956,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, queueSize = net->driver.virtio.queues; if (!queueSize) queueSize = 1; - if (!qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) { + if (!qemuDomainSupportsNicdev(vm->def, net)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Netdev support unavailable")); goto cleanup;Same as the first instance.diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c index cebb490221..24c0174bf9 100644 --- a/src/qemu/qemu_interface.c +++ b/src/qemu/qemu_interface.c @@ -646,7 +646,7 @@ qemuInterfaceOpenVhostNet(virDomainDefPtr def, * option), don't try to open the device. */ if (!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_VHOST_NET) && - qemuDomainSupportsNetdev(def, qemuCaps, net))) { + qemuDomainSupportsNicdev(def, net))) { if (net->driver.virtio.name == VIR_DOMAIN_NET_BACKEND_TYPE_VHOST) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("vhost-net is not supported with "The full comment, only half of which is contained in the hunk, is If qemu doesn't support vhost-net mode (including the -netdev command option), don't try to open the device. Once again, it should point to -device rather than -netdev.
I made it point to both, since we assumed -device support much earlier. Jano
Trusting that you'll tweak both comments and error messages so that they will not confuse the next soul wandering through these lands, Reviewed-by: Andrea Bolognani <abologna@xxxxxxxxxx> -- Andrea Bolognani / Red Hat / Virtualization
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
