| Message ID | MEYP282MB3302674FCE1E157ED173BDB3CD9D9@MEYP282MB3302.AUSP282.PROD.OUTLOOK.COM |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] 回复: [PATCH v2] netdev-vport : Fix userspace tunnel ioctl(SIOCGIFINDEX) info logs. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | fail | apply and check: fail |
Hello linhuang,
This is a good idea, but I've run into a few issues with this patch.
There's still one instance of "type" that needs to be changed to
dpif_type to fix compile.
Also, the behaviour of strcmp is undefined if one of the pointers is
NULL. On my system, it throws SIGSEGV. This causes a bunch of the tests
to fail.
Somethign like:
if (dpif_type != NULL && !strcmp(dpif_type, "system")) {
return linux_get_ifindex(name);
} else {
return -ENODEV;
}
However, even with this change, I'm still getting some test failures.
Cheers,
MKP
On Sat, 2021-11-20 at 13:12 +0000, lin huang wrote:
> From: linhuang <linhuang@ruijie.com.cn>
>
> 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: linhuang <linhuang@ruijie.com.cn>
> Reviewed-by: Aaron Conole <aconole@redhat.com>
> ---
> lib/dpif.c | 2 +-
> lib/netdev-offload.c | 2 --
> lib/netdev-vport.c | 3 ++-
> 3 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 8c4aed4..7adb620 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -362,8 +362,8 @@ do_open(const char *name, const char *type, bool create, struct dpif **dpifp)
> }
>
> err = netdev_open(dpif_port.name, dpif_port.type, &netdev);
> -
> if (!err) {
> + netdev_set_dpif_type(netdev, dpif_type_str);
> netdev_ports_insert(netdev, dpif_type_str, &dpif_port);
> netdev_close(netdev);
> } else {
> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c
> index 8075cfb..1221170 100644
> --- a/lib/netdev-offload.c
> +++ b/lib/netdev-offload.c
> @@ -596,8 +596,6 @@ netdev_ports_insert(struct netdev *netdev, const char *dpif_type,
> data->ifindex = -1;
> }
>
> - netdev_set_dpif_type(netdev, dpif_type);
> -
> hmap_insert(&port_to_netdev, &data->portno_node,
> netdev_ports_hash(dpif_port->port_no, dpif_type));
> ovs_rwlock_unlock(&netdev_hmap_rwlock);
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index 499c029..ad24933 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -1151,8 +1151,9 @@ 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 !strcmp(type, "system") ? linux_get_ifindex(name) : -ENODEV;
> }
>
> #define NETDEV_VPORT_GET_IFINDEX netdev_vport_get_ifindex
> --
> 2.12.2
>
> ________________________________________
> 发件人: dev <ovs-dev-bounces@openvswitch.org> 代表 lin huang <miterv@outlook.com>
> 发送时间: 2021年11月17日 12:39
> 收件人: dev
> 抄送: Ben Pfaff; i.maximets
> 主题: Re: [ovs-dev] [PATCH v2] netdev-vport : Fix userspace tunnel ioctl(SIOCGIFINDEX) info logs.
>
> hi all,
>
> pls review my code.
>
> Regards,
> Lin Huang
>
> From: lin huang<mailto:miterv@outlook.com>
> Date: 2021-10-25 13:16
> To: dev@openvswitch.org<mailto:dev@openvswitch.org>
> CC: Aaron Conole<mailto:aconole@redhat.com>; i.maximets@ovn.org<mailto:i.maximets@ovn.org>
> Subject: [PATCH v2] netdev-vport : Fix userspace tunnel ioctl(SIOCGIFINDEX) info logs.
> From: linhuang <linhuang@ruijie.com.cn>
>
> 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: linhuang <linhuang@ruijie.com.cn>
> Reviewed-by: Aaron Conole <aconole@redhat.com>
> ---
> lib/netdev-offload.c | 6 ++++--
> lib/netdev-vport.c | 3 ++-
> vswitchd/bridge.c | 2 ++
> 3 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c
> index 8075cfbd8..00b7515cf 100644
> --- a/lib/netdev-offload.c
> +++ b/lib/netdev-offload.c
> @@ -577,7 +577,9 @@ netdev_ports_insert(struct netdev *netdev, const char *dpif_type,
> struct dpif_port *dpif_port)
> {
> struct port_to_netdev_data *data;
> - int ifindex = netdev_get_ifindex(netdev);
> + int ifindex;
> +
> + netdev_set_dpif_type(netdev, dpif_type);
> ovs_rwlock_wrlock(&netdev_hmap_rwlock);
> if (netdev_ports_lookup(dpif_port->port_no, dpif_type)) {
> @@ -589,6 +591,7 @@ netdev_ports_insert(struct netdev *netdev, const char *dpif_type,
> data->netdev = netdev_ref(netdev);
> dpif_port_clone(&data->dpif_port, dpif_port);
> + ifindex = netdev_get_ifindex(netdev);
> if (ifindex >= 0) {
> data->ifindex = ifindex;
> hmap_insert(&ifindex_to_port, &data->ifindex_node, ifindex);
> @@ -596,7 +599,6 @@ netdev_ports_insert(struct netdev *netdev, const char *dpif_type,
> data->ifindex = -1;
> }
> - netdev_set_dpif_type(netdev, dpif_type);
> hmap_insert(&port_to_netdev, &data->portno_node,
> netdev_ports_hash(dpif_port->port_no, dpif_type));
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index 499c0291c..411ac343a 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -1151,8 +1151,9 @@ 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 *type = netdev_get_dpif_type(netdev_);
> - return linux_get_ifindex(name);
> + return (strcmp(type, "netdev")) ? linux_get_ifindex(name) : -ENODEV;
> }
> #define NETDEV_VPORT_GET_IFINDEX netdev_vport_get_ifindex
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index c790a56ad..473d79acd 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -2052,6 +2052,8 @@ iface_do_create(const struct bridge *br,
> goto error;
> }
> + netdev_set_dpif_type(netdev, br->type);
> +
> error = iface_set_netdev_config(iface_cfg, netdev, errp);
> if (error) {
> goto error;
> --
> 2.12.2
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/lib/dpif.c b/lib/dpif.c index 8c4aed4..7adb620 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -362,8 +362,8 @@ do_open(const char *name, const char *type, bool create, struct dpif **dpifp) } err = netdev_open(dpif_port.name, dpif_port.type, &netdev); - if (!err) { + netdev_set_dpif_type(netdev, dpif_type_str); netdev_ports_insert(netdev, dpif_type_str, &dpif_port); netdev_close(netdev); } else { diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c index 8075cfb..1221170 100644 --- a/lib/netdev-offload.c +++ b/lib/netdev-offload.c @@ -596,8 +596,6 @@ netdev_ports_insert(struct netdev *netdev, const char *dpif_type, data->ifindex = -1; } - netdev_set_dpif_type(netdev, dpif_type); - hmap_insert(&port_to_netdev, &data->portno_node, netdev_ports_hash(dpif_port->port_no, dpif_type)); ovs_rwlock_unlock(&netdev_hmap_rwlock); diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index 499c029..ad24933 100644 --- a/lib/netdev-vport.c +++ b/lib/netdev-vport.c @@ -1151,8 +1151,9 @@ 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 !strcmp(type, "system") ? linux_get_ifindex(name) : -ENODEV; } #define NETDEV_VPORT_GET_IFINDEX netdev_vport_get_ifindex -- 2.12.2 ________________________________________ 发件人: dev <ovs-dev-bounces@openvswitch.org> 代表 lin huang <miterv@outlook.com> 发送时间: 2021年11月17日 12:39 收件人: dev 抄送: Ben Pfaff; i.maximets 主题: Re: [ovs-dev] [PATCH v2] netdev-vport : Fix userspace tunnel ioctl(SIOCGIFINDEX) info logs. hi all, pls review my code. Regards, Lin Huang From: lin huang<mailto:miterv@outlook.com> Date: 2021-10-25 13:16 To: dev@openvswitch.org<mailto:dev@openvswitch.org> CC: Aaron Conole<mailto:aconole@redhat.com>; i.maximets@ovn.org<mailto:i.maximets@ovn.org> Subject: [PATCH v2] netdev-vport : Fix userspace tunnel ioctl(SIOCGIFINDEX) info logs. From: linhuang <linhuang@ruijie.com.cn> 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: linhuang <linhuang@ruijie.com.cn> Reviewed-by: Aaron Conole <aconole@redhat.com> --- lib/netdev-offload.c | 6 ++++-- lib/netdev-vport.c | 3 ++- vswitchd/bridge.c | 2 ++ 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c index 8075cfbd8..00b7515cf 100644 --- a/lib/netdev-offload.c +++ b/lib/netdev-offload.c @@ -577,7 +577,9 @@ netdev_ports_insert(struct netdev *netdev, const char *dpif_type, struct dpif_port *dpif_port) { struct port_to_netdev_data *data; - int ifindex = netdev_get_ifindex(netdev); + int ifindex; + + netdev_set_dpif_type(netdev, dpif_type); ovs_rwlock_wrlock(&netdev_hmap_rwlock); if (netdev_ports_lookup(dpif_port->port_no, dpif_type)) { @@ -589,6 +591,7 @@ netdev_ports_insert(struct netdev *netdev, const char *dpif_type, data->netdev = netdev_ref(netdev); dpif_port_clone(&data->dpif_port, dpif_port); + ifindex = netdev_get_ifindex(netdev); if (ifindex >= 0) { data->ifindex = ifindex; hmap_insert(&ifindex_to_port, &data->ifindex_node, ifindex); @@ -596,7 +599,6 @@ netdev_ports_insert(struct netdev *netdev, const char *dpif_type, data->ifindex = -1; } - netdev_set_dpif_type(netdev, dpif_type); hmap_insert(&port_to_netdev, &data->portno_node, netdev_ports_hash(dpif_port->port_no, dpif_type)); diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index 499c0291c..411ac343a 100644 --- a/lib/netdev-vport.c +++ b/lib/netdev-vport.c @@ -1151,8 +1151,9 @@ 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 *type = netdev_get_dpif_type(netdev_); - return linux_get_ifindex(name); + return (strcmp(type, "netdev")) ? linux_get_ifindex(name) : -ENODEV; } #define NETDEV_VPORT_GET_IFINDEX netdev_vport_get_ifindex diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index c790a56ad..473d79acd 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -2052,6 +2052,8 @@ iface_do_create(const struct bridge *br, goto error; } + netdev_set_dpif_type(netdev, br->type); + error = iface_set_netdev_config(iface_cfg, netdev, errp); if (error) { goto error; -- 2.12.2