[libvirt] [PATCH 2/3] qemu: move runtime netdev validation into a separate function

[Laine Stump]

On 9/13/19 4:02 PM, Michal Prívozník wrote:
> On 9/13/19 4:52 PM, Laine Stump wrote:
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -5300,6 +5300,86 @@ qemuDomainWatchdogDefValidate(const virDomainWatchdogDef *dev,
>>   }
>>   
>>   
>> +
> 
> 3 empty lines instead of 2.


:-O

> 
>> +int
>> +qemuDomainValidateActualNetDef(const virDomainNetDef *net,
>> +                               virQEMUCapsPtr qemuCaps)
>> +{
>> +    /*
>> +     * Validations that can only be properly checked at runtime (after
>> +     * an <interface type='network'> has been resolved to its actual
>> +     * type.
>> +     *
>> +     * (In its current form this function can still be called before
>> +     * the actual type has been resolved (e.g. at domain definition
>> +     * time), but only if the validations would SUCCEED for
>> +     * type='network'.)
>> +     */
>> +    virDomainNetType actualType = virDomainNetGetActualType(net);
>> +
>> +    /* Only tap/macvtap devices support multiqueue. */
>> +    if (net->driver.virtio.queues > 1) {
> 
> I don't think that this is right. Take VIR_DOMAIN_NET_TYPE_USER for instance. It doesn't allow anything but queues == 0.

Right, I hadn't thought of the other non-tap types.

> 
>> +
>> +        if (!(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
>> +              actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
>> +              actualType == VIR_DOMAIN_NET_TYPE_DIRECT ||
>> +              actualType == VIR_DOMAIN_NET_TYPE_ETHERNET ||
>> +              actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER)) {
>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +                           _("multiqueue network is not supported for: %s"),
>> +                           virDomainNetTypeToString(actualType));
>> +            return -1;
>> +        }
>> +> +        if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
> 
> This is actually where a single queue can be permitred. At least according to the original code.

> 
>> +            qemuCaps && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VHOSTUSER_MULTIQUEUE)) {
> 
> NULL is never passed to qemuCaps, so no need to check it.
> 
>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> +                           _("multiqueue network is not supported for vhost-user "
>> +                             "with this QEMU binary"));
>> +            return -1;
>> +        }
>> +    }
>> +
>> +    /*
>> +     * Only standard tap devices support nwfilter rules, and even then only
>> +     * when *not* connected to an OVS bridge or midonet (indicated by having
>> +     * a <virtualport> element in the config)
>> +     */
>> +    if (net->filter) {
>> +        virNetDevVPortProfilePtr vport = virDomainNetGetActualVirtPortProfile(net);
>> +        if (!(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
>> +              actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
>> +              actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)) {
>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +                           _("filterref is not supported for "
>> +                             "network interfaces of type %s"),
>> +                           virDomainNetTypeToString(actualType));
>> +            return -1;
>> +        }
>> +        if (vport && vport->virtPortType != VIR_NETDEV_VPORT_PROFILE_NONE) {
>> +            /* currently none of the defined virtualport types support iptables */
>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +                           _("filterref is not supported for "
>> +                             "network interfaces with virtualport type %s"),
>> +                           virNetDevVPortTypeToString(vport->virtPortType));
>> +            return -1;
>> +        }
>> +    }
>> +
>> +    if (net->backend.tap &&
>> +        !(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
>> +          actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
>> +          actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)) {
>> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +                       _("Custom tap device path is not supported for: %s"),
>> +                       virDomainNetTypeToString(actualType));
>> +        return -1;
>> +    }
>> +
>> +    return 0;
>> + }
> 
> s/^ //


That's strange - I ran make syntax-check multiple times, and it always 
finds those...


> 
> ACK with this squashed in:
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index ebbe1a85db..fa0dd888c8 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5300,7 +5300,6 @@ qemuDomainWatchdogDefValidate(const virDomainWatchdogDef *dev,
>   }
>   
>   
> -
>   int
>   qemuDomainValidateActualNetDef(const virDomainNetDef *net,
>                                  virQEMUCapsPtr qemuCaps)
> @@ -5318,8 +5317,7 @@ qemuDomainValidateActualNetDef(const virDomainNetDef *net,
>       virDomainNetType actualType = virDomainNetGetActualType(net);
>   
>       /* Only tap/macvtap devices support multiqueue. */
> -    if (net->driver.virtio.queues > 1) {
> -
> +    if (net->driver.virtio.queues > 0) {
>           if (!(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
>                 actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
>                 actualType == VIR_DOMAIN_NET_TYPE_DIRECT ||
> @@ -5331,8 +5329,9 @@ qemuDomainValidateActualNetDef(const virDomainNetDef *net,
>               return -1;
>           }
>   
> -        if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
> -            qemuCaps && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VHOSTUSER_MULTIQUEUE)) {
> +        if (net->driver.virtio.queues > 1 &&
> +            actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
> +            !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VHOSTUSER_MULTIQUEUE)) {
>               virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                              _("multiqueue network is not supported for vhost-user "
>                                "with this QEMU binary"));
> @@ -5377,7 +5376,7 @@ qemuDomainValidateActualNetDef(const virDomainNetDef *net,
>       }
>   
>       return 0;
> - }
> +}


That all looks agreeable to me.

Thanks!