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)) {
                        "%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;
                 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)) {
                            "%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) {
                            "%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.


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 <abolo...@redhat.com>

Andrea Bolognani / Red Hat / Virtualization

Attachment: signature.asc
Description: Digital signature

libvir-list mailing list

Reply via email to