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/e28afc564700a1a35e3bf0269687d5734251b88a/nova/virt/libvirt/vif.py#L479-L485
designer.set_vif_host_backend_hw_veb(
conf, 'hostdev', vif.dev_address, None)
https://github.com/openstack/nova/blob/e28afc564700a1a35e3bf0269687d5734251b88a/nova/virt/libvirt/designer.py#L97-L105
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
