On 11/24/21 14:32, lin huang wrote:
> From: linhuang <[email protected]>
>
> Userspace tunnel doesn't have a valid device in the kernel. So
> get_ifindex() function (ioctl) always get error during
> adding a port, deleting a port or updating a port status.
>
> The info log is
> "2021-08-29T09:17:39.830Z|00059|netdev_linux|INFO|ioctl(SIOCGIFINDEX)
> on vxlan_sys_4789 device failed: No such device"
>
> If there are a lot of userspace tunnel ports on a bridge, the
> iface_refresh_netdev_status() function will spend a lot of time.
>
> So ignore userspace tunnel port ioctl(SIOCGIFINDEX) operation, just
> return -ENODEV.
>
> Signed-off-by: Lin Huang <[email protected]>
> Test-by: Mike Pattrick <[email protected]>
> Reviewed-by: Aaron Conole <[email protected]>
> Reviewed-by: Ilya Maximets <[email protected]>
Hello. Thanks for v3. And sorry for delays.
One comment about the commit message: please, don't add tags
that wasn't actually provided in previous reviews. Also,
patch changed noticeably between versions, so tags can not
be preserved in this case.
Some code comments inline.
> ---
> lib/netdev-vport.c | 4 +++-
> vswitchd/bridge.c | 2 ++
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index 499c029..f0ff02b 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -1151,8 +1151,10 @@ netdev_vport_get_ifindex(const struct netdev *netdev_)
> {
> char buf[NETDEV_VPORT_NAME_BUFSIZE];
> const char *name = netdev_vport_get_dpif_port(netdev_, buf, sizeof(buf));
> + const char *dpif_type = netdev_get_dpif_type(netdev_);
>
> - return linux_get_ifindex(name);
> + return (dpif_type && !strcmp(dpif_type, "system")
> + ? linux_get_ifindex(name) : -ENODEV);
The operator precedence seems tricky here and hard to understand.
Another problem is that if dpif_type is not defined, we should
default to executing linux_get_ifindex() instead of returning
an error.
Suggesting to re-write as an 'if' condition like this:
if (dpif_type && strcmp(dpif_type, "system")) {
/* Not a system device. */
return -ENODEV;
}
return linux_get_ifindex(name);
What do you think?
Another option is to replace the 'dpif_type' NULL check with the
ovs_assert(dpif_type), because we're not expecting it to be NULL
with this change.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
