Re: [libvirt PATCH v3 1/1] Ignore EPERM on attempts to clear VF VLAN ID
Hi Daniel,
Thanks a lot for the review, I'll send a v4 with the requested changes included.
On Tue, Nov 16, 2021 at 9:55 PM Daniel P. Berrangé <berrange redhat.com> wrote:
> +int
> +virNetDevSendVfSetLinkRequest(const char *ifname, int vfInfoType,
> + const void *payload, const size_t payloadLen)
I think it would be desirable for this method to be introduced
in a patch on its own, separate from the patch that then splits
virNetDevSetVfConfig into 2 parts, and separate from one that
adds EPERM handling.
Our general guideline is that refactoring should never be mixed
with functional behaviour changes, as this has often resulted
in surprise regressions that were diguised by the mixing.
Ack, agreed. Apologies for not doing it right away - there is
definitely merit in doing it as you describe.
> +
> + requestError = virNetDevSendVfSetLinkRequest(ifname, IFLA_VF_VLAN,
> + &ifla_vf_vlan,
sizeof(ifla_vf_vlan));
> +
> + /* If vlanid is 0 - we are attempting to clear an existing VLAN id.
> + * An EPERM received at this stage is an indicator that the embedded
> + * switch is not exposed to this host and the network driver is not
> + * able to set a VLAN for a VF. */
> + if (requestError == -EPERM && vlanid == 0) {
This metod is taking a plain "int vlanid", but the eventual
caller of this method is taking "virNetDevVlan *vlan".
IOW, the caller can distinguish between two scenarios
- vlan is NULL => set vlanid to 0
- vlan is non-NULL and user specified vlanid of 0
I think it is reasonable to ignore EPERM in the first
scenario for the reasons you describe wrt hardware
driver restrictions.
I'm less sure it is a good idea to ignore EPERM when
it wasn an explicit user configuration request to set
vlanid to 0. It feels like we should be continuing to
report an error if we can't honour an explicit user
request - its a sign the user shouldn't have been
settng the vlan in the first place.
Agreed, I can convert virNetDevSetVfConfig and virNetDevSetVfVlan to
accept a pointer to a vlan tag and ignore EPERM in virNetDevSetVfVlan
only when the VLAN pointer is NULL.
For the use-case this patch is introduced, the higher-level software
(Nova) does not specify a VLAN when formatting a device XML provided
to Libvirt.
https://github.com/openstack/nova/blob/e28afc564700a1a35e3bf0269687d57342...
designer.set_vif_host_backend_hw_veb(
conf, 'hostdev', vif.dev_address, None)
https://github.com/openstack/nova/blob/e28afc564700a1a35e3bf0269687d57342...
And the resulting device XML looks like this:
<interface type='hostdev' managed='yes'>
<mac address='fa:16:3e:f4:ff:3c'/>
<driver name='vfio'/>
<source>
<address type='pci' domain='0x0000' bus='0x82'
slot='0x08'
function='0x2'/>
</source>
<alias name='hostdev0'/>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x03'
function='0x0'/>
</interface>
As you say, with that we can preserve the VLAN clearing behavior when
the VLAN pointer is NULL if it succeeds and ignore EPERM if it
doesn't. And if clearing is explicit fail on EPERM and other errors.
Best Regards,
Dmitrii Shcherbakov
LP/oftc: dmitriis