Re: [libvirt] [libvirt PATCH v2 09/44] Remove qemuDomainSupportsNetdev
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.
> -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;
> @@ -8577,8 +8568,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
> if (!(nic = qemuBuildNicStr(net, "nic,", vlan)))
> goto cleanup;
> virCommandAddArgList(cmd, "-net", nic, NULL);
> -}
> -if (!qemuDomainSupportsNetdev(def, qemuCaps, net)) {
> +
> if (!(host = qemuBuildHostNetStr(net, driver,
> ',', vlan,
> tapfdName, tapfdSize,
Incidentally, this makes the flow of the function much easier to
follow. Nice!
> @@ -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,
Re: [libvirt] [libvirt PATCH v2 09/44] Remove qemuDomainSupportsNetdev
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.
