[libvirt] [libvirt PATCH v2 09/44] Remove qemuDomainSupportsNetdev

[Ján Tomko]

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 at redhat.com>
>
>-- 
>Andrea Bolognani / Red Hat / Virtualization
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180412/bb3b54ed/attachment-0001.sig>