All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net 0/2] openvswitch: syzbot splat fix and introduce selftest
@ 2022-10-19 18:30 Aaron Conole
  2022-10-19 18:30 ` [PATCH net 1/2] openvswitch: switch from WARN to pr_warn Aaron Conole
  2022-10-19 18:30 ` [PATCH net 2/2] selftests: add openvswitch selftest suite Aaron Conole
  0 siblings, 2 replies; 9+ messages in thread
From: Aaron Conole @ 2022-10-19 18:30 UTC (permalink / raw)
  To: netdev
  Cc: Pravin B Shelar, Jakub Kicinski, David S. Miller, Paolo Abeni,
	Eric Dumazet, Thomas Graf, Kevin Sprague, dev, Eelco Chaudron,
	Ilya Maximets, Shuah Khan, linux-kernel, linux-kselftest

Syzbot recently caught a splat when dropping features from
openvswitch datapaths that are in-use.  The WARN() call is
definitely too large a hammer for the situation, so change
to pr_warn.

Second patch in the series introduces a new selftest suite which
can help show that an issue is fixed.  This change might be
more suited to net-next tree, so it has been separated out
as an additional patch and can be either applied to either tree
based on preference.

Aaron Conole (2):
  openvswitch: switch from WARN to pr_warn
  selftests: add openvswitch selftest suite

 MAINTAINERS                                   |   1 +
 net/openvswitch/datapath.c                    |   3 +-
 tools/testing/selftests/Makefile              |   1 +
 .../selftests/net/openvswitch/Makefile        |  13 +
 .../selftests/net/openvswitch/openvswitch.sh  | 216 +++++++++
 .../selftests/net/openvswitch/ovs-dpctl.py    | 411 ++++++++++++++++++
 6 files changed, 644 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/net/openvswitch/Makefile
 create mode 100755 tools/testing/selftests/net/openvswitch/openvswitch.sh
 create mode 100644 tools/testing/selftests/net/openvswitch/ovs-dpctl.py

-- 
2.34.3


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH net 1/2] openvswitch: switch from WARN to pr_warn
  2022-10-19 18:30 [PATCH net 0/2] openvswitch: syzbot splat fix and introduce selftest Aaron Conole
@ 2022-10-19 18:30 ` Aaron Conole
  2022-10-20 18:40   ` Ilya Maximets
  2022-10-19 18:30 ` [PATCH net 2/2] selftests: add openvswitch selftest suite Aaron Conole
  1 sibling, 1 reply; 9+ messages in thread
From: Aaron Conole @ 2022-10-19 18:30 UTC (permalink / raw)
  To: netdev
  Cc: Pravin B Shelar, Jakub Kicinski, David S. Miller, Paolo Abeni,
	Eric Dumazet, Thomas Graf, Kevin Sprague, dev, Eelco Chaudron,
	Ilya Maximets, Shuah Khan, linux-kernel, linux-kselftest

As noted by Paolo Abeni, pr_warn doesn't generate any splat and can still
preserve the warning to the user that feature downgrade occurred.  We
likely cannot introduce other kinds of checks / enforcement here because
syzbot can generate different genl versions to the datapath.

Reported-by: syzbot+31cde0bef4bbf8ba2d86@syzkaller.appspotmail.com
Fixes: 44da5ae5fbea ("openvswitch: Drop user features if old user space attempted to create datapath")
Cc: Thomas Graf <tgraf@suug.ch>
Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 net/openvswitch/datapath.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index c8a9075ddd0a..155263e73512 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -1616,7 +1616,8 @@ static void ovs_dp_reset_user_features(struct sk_buff *skb,
 	if (IS_ERR(dp))
 		return;
 
-	WARN(dp->user_features, "Dropping previously announced user features\n");
+	pr_warn("%s: Dropping previously announced user features\n",
+		ovs_dp_name(dp));
 	dp->user_features = 0;
 }
 
-- 
2.34.3


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH net 2/2] selftests: add openvswitch selftest suite
  2022-10-19 18:30 [PATCH net 0/2] openvswitch: syzbot splat fix and introduce selftest Aaron Conole
  2022-10-19 18:30 ` [PATCH net 1/2] openvswitch: switch from WARN to pr_warn Aaron Conole
@ 2022-10-19 18:30 ` Aaron Conole
  2022-10-20 13:21   ` Ilya Maximets
  1 sibling, 1 reply; 9+ messages in thread
From: Aaron Conole @ 2022-10-19 18:30 UTC (permalink / raw)
  To: netdev
  Cc: Pravin B Shelar, Jakub Kicinski, David S. Miller, Paolo Abeni,
	Eric Dumazet, Thomas Graf, Kevin Sprague, dev, Eelco Chaudron,
	Ilya Maximets, Shuah Khan, linux-kernel, linux-kselftest

Previous commit resolves a WARN splat that can be difficult to reproduce,
but with the ovs-dpctl.py utility, it can be trivial.  Introduce a test
case which creates a DP, and then downgrades the feature set.  This will
include a utility 'ovs-dpctl.py' that can be extended to do additional
work.

Signed-off-by: Aaron Conole <aconole@redhat.com>
Signed-off-by: Kevin Sprague <ksprague0711@gmail.com>
---
 MAINTAINERS                                   |   1 +
 tools/testing/selftests/Makefile              |   1 +
 .../selftests/net/openvswitch/Makefile        |  13 +
 .../selftests/net/openvswitch/openvswitch.sh  | 216 +++++++++
 .../selftests/net/openvswitch/ovs-dpctl.py    | 411 ++++++++++++++++++
 5 files changed, 642 insertions(+)
 create mode 100644 tools/testing/selftests/net/openvswitch/Makefile
 create mode 100755 tools/testing/selftests/net/openvswitch/openvswitch.sh
 create mode 100644 tools/testing/selftests/net/openvswitch/ovs-dpctl.py

diff --git a/MAINTAINERS b/MAINTAINERS
index abbe88e1c50b..295a6b0fbe26 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15434,6 +15434,7 @@ S:	Maintained
 W:	http://openvswitch.org
 F:	include/uapi/linux/openvswitch.h
 F:	net/openvswitch/
+F:	tools/testing/selftests/net/openvswitch/
 
 OPERATING PERFORMANCE POINTS (OPP)
 M:	Viresh Kumar <vireshk@kernel.org>
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 0464b2c6c1e4..f07aef7c592c 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -49,6 +49,7 @@ TARGETS += net
 TARGETS += net/af_unix
 TARGETS += net/forwarding
 TARGETS += net/mptcp
+TARGETS += net/openvswitch
 TARGETS += netfilter
 TARGETS += nsfs
 TARGETS += pidfd
diff --git a/tools/testing/selftests/net/openvswitch/Makefile b/tools/testing/selftests/net/openvswitch/Makefile
new file mode 100644
index 000000000000..2f1508abc826
--- /dev/null
+++ b/tools/testing/selftests/net/openvswitch/Makefile
@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: GPL-2.0
+
+top_srcdir = ../../../../..
+
+CFLAGS =  -Wall -Wl,--no-as-needed -O2 -g -I$(top_srcdir)/usr/include $(KHDR_INCLUDES)
+
+TEST_PROGS := openvswitch.sh
+
+TEST_FILES := ovs-dpctl.py
+
+EXTRA_CLEAN := test_netlink_checks
+
+include ../../lib.mk
diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
new file mode 100755
index 000000000000..bebc20f157dc
--- /dev/null
+++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
@@ -0,0 +1,216 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+#
+# OVS kernel module self tests
+
+# Kselftest framework requirement - SKIP code is 4.
+ksft_skip=4
+
+PAUSE_ON_FAIL=no
+VERBOSE=0
+TRACING=0
+
+tests="
+	netlink_checks				ovsnl: validate netlink attrs and settings"
+
+info() {
+    [ $VERBOSE = 0 ] || echo $*
+}
+
+ovs_base=`pwd`
+sbxs=
+sbx_add () {
+	info "adding sandbox '$1'"
+
+	sbxs="$sbxs $1"
+
+	NO_BIN=0
+
+	# Create sandbox.
+	local d="$ovs_base"/$1
+	if [ -e $d ]; then
+		info "removing $d"
+		rm -rf "$d"
+	fi
+	mkdir "$d" || return 1
+	ovs_setenv $1
+}
+
+ovs_exit_sig() {
+	[ -e ${ovs_dir}/cleanup ] && . "$ovs_dir/cleanup"
+}
+
+on_exit() {
+	echo "$1" > ${ovs_dir}/cleanup.tmp
+	cat ${ovs_dir}/cleanup >> ${ovs_dir}/cleanup.tmp
+	mv ${ovs_dir}/cleanup.tmp ${ovs_dir}/cleanup
+}
+
+ovs_setenv() {
+	sandbox=$1
+
+	ovs_dir=$ovs_base${1:+/$1}; export ovs_dir
+
+	test -e ${ovs_dir}/cleanup || : > ${ovs_dir}/cleanup
+}
+
+ovs_sbx() {
+	if test "X$2" != X; then
+		(ovs_setenv $1; shift; "$@" >> ${ovs_dir}/debug.log)
+	else
+		ovs_setenv $1
+	fi
+}
+
+ovs_add_dp () {
+	info "Adding DP/Bridge IF: sbx:$1 dp:$2 {$3, $4, $5}"
+	ovs_sbx "$1" python3 $ovs_base/ovs-dpctl.py add-dp "$2" "$3" "$4" "$5" || return 1
+	on_exit "ovs_sbx $1 python3 $ovs_base/ovs-dpctl.py del-dp $2;"
+}
+
+usage() {
+	echo
+	echo "$0 [OPTIONS] [TEST]..."
+	echo "If no TEST argument is given, all tests will be run."
+	echo
+	echo "Options"
+	echo "  -t: capture traffic via tcpdump"
+	echo "  -v: verbose"
+	echo "  -p: pause on failure"
+	echo
+	echo "Available tests${tests}"
+	exit 1
+}
+
+# netlink_validation
+# - Create a dp
+# - check no warning with "old version" simulation
+test_netlink_checks () {
+	sbx_add "test_netlink_checks" || return 1
+
+	info "setting up new DP"
+	ovs_add_dp "test_netlink_checks" nv0 || return 1
+	# now try again
+	PRE_TEST=$(dmesg | grep -E "RIP: [0-9a-fA-Fx]+:ovs_dp_cmd_new\+")
+	ovs_add_dp "test_netlink_checks" nv0 -V 0 || return 1
+	POST_TEST=$(dmesg | grep -E "RIP: [0-9a-fA-Fx]+:ovs_dp_cmd_new\+")
+	if [ "$PRE_TEST" != "$POST_TEST" ]; then
+		info "failed - gen warning"
+		return 1
+	fi
+
+	return 0
+}
+
+run_test() {
+	(
+	tname="$1"
+	tdesc="$2"
+
+	if ! lsmod | grep openvswitch >/dev/null 2>&1; then
+		stdbuf -o0 printf "TEST: %-60s  [NOMOD]\n" "${tdesc}"
+		return $ksft_skip
+	fi
+
+	if python3 ovs-dpctl.py help 2>&1 | \
+	     grep "Need to install the python" >/dev/null 2>&1; then
+		stdbuf -o0 printf "TEST: %-60s  [PYLIB]\n" "${tdesc}"
+		return $ksft_skip
+	fi
+	printf "TEST: %-60s  [START]\n" "${tname}"
+
+	unset IFS
+
+	eval test_${tname}
+	ret=$?
+
+	if [ $ret -eq 0 ]; then
+		printf "TEST: %-60s  [ OK ]\n" "${tdesc}"
+		ovs_exit_sig
+		rm -rf "$ovs_dir"
+	elif [ $ret -eq 1 ]; then
+		printf "TEST: %-60s  [FAIL]\n" "${tdesc}"
+		if [ "${PAUSE_ON_FAIL}" = "yes" ]; then
+			echo
+			echo "Pausing. Logs in $ovs_dir/. Hit enter to continue"
+			read a
+		fi
+		ovs_exit_sig
+		[ "${PAUSE_ON_FAIL}" = "yes" ] || rm -rf "$ovs_dir"
+		exit 1
+	elif [ $ret -eq $ksft_skip ]; then
+		printf "TEST: %-60s  [SKIP]\n" "${tdesc}"
+	elif [ $ret -eq 2 ]; then
+		rm -rf test_${tname}
+		run_test "$1" "$2"
+	fi
+
+	return $ret
+	)
+	ret=$?
+	case $ret in
+		0)
+			[ $all_skipped = true ] && [ $exitcode=$ksft_skip ] && exitcode=0
+			all_skipped=false
+		;;
+		$ksft_skip)
+			[ $all_skipped = true ] && exitcode=$ksft_skip
+		;;
+		*)
+			all_skipped=false
+			exitcode=1
+		;;
+	esac
+
+	return $ret
+}
+
+
+exitcode=0
+desc=0
+all_skipped=true
+
+while getopts :pvt o
+do
+	case $o in
+	p) PAUSE_ON_FAIL=yes;;
+	v) VERBOSE=1;;
+	t) if which tcpdump > /dev/null 2>&1; then
+		TRACING=1
+	   else
+		echo "=== tcpdump not available, tracing disabled"
+	   fi
+	   ;;
+	*) usage;;
+	esac
+done
+shift $(($OPTIND-1))
+
+IFS="	
+"
+
+for arg do
+	# Check first that all requested tests are available before running any
+	command -v > /dev/null "test_${arg}" || { echo "=== Test ${arg} not found"; usage; }
+done
+
+name=""
+desc=""
+for t in ${tests}; do
+	[ "${name}" = "" ]	&& name="${t}"	&& continue
+	[ "${desc}" = "" ]	&& desc="${t}"
+
+	run_this=1
+	for arg do
+		[ "${arg}" != "${arg#--*}" ] && continue
+		[ "${arg}" = "${name}" ] && run_this=1 && break
+		run_this=0
+	done
+	if [ $run_this -eq 1 ]; then
+		run_test "${name}" "${desc}"
+	fi
+	name=""
+	desc=""
+done
+
+exit ${exitcode}
diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
new file mode 100644
index 000000000000..791d76b7adcd
--- /dev/null
+++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
@@ -0,0 +1,411 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: GPL-2.0
+
+# Controls the openvswitch module.  Part of the kselftest suite, but
+# can be used for some diagnostic purpose as well.
+
+import logging
+import multiprocessing
+import socket
+import struct
+import sys
+
+try:
+    from libnl.attr import NLA_NESTED, NLA_STRING, NLA_U32, NLA_UNSPEC
+    from libnl.attr import nla_get_string, nla_get_u32
+    from libnl.attr import nla_put, nla_put_string, nla_put_u32
+    from libnl.attr import nla_policy
+
+    from libnl.error import errmsg
+
+    from libnl.genl.ctrl import genl_ctrl_resolve
+    from libnl.genl.genl import genl_connect, genlmsg_parse, genlmsg_put
+
+    from libnl.handlers import nl_cb_alloc, nl_cb_set
+    from libnl.handlers import NL_CB_CUSTOM, NL_CB_MSG_IN, NL_CB_VALID
+    from libnl.handlers import NL_OK, NL_STOP
+
+    from libnl.linux_private.netlink import NLM_F_ACK, NLM_F_DUMP
+    from libnl.linux_private.netlink import NLM_F_REQUEST, NLMSG_DONE
+
+    from libnl.msg import NL_AUTO_SEQ, nlmsg_alloc, nlmsg_hdr
+
+    from libnl.nl import NLMSG_ERROR, nl_recvmsgs_default, nl_send_auto
+    from libnl.socket_ import nl_socket_alloc, nl_socket_set_cb
+    from libnl.socket_ import nl_socket_get_local_port
+except ModuleNotFoundError:
+    print("Need to install the python libnl3 library.")
+    print("Exiting without error.")
+    exit(0)
+
+
+global sk
+global ovs_families
+
+OVS_DATAPATH_FAMILY = "ovs_datapath"
+OVS_VPORT_FAMILY = "ovs_vport"
+OVS_FLOW_FAMILY = "ovs_flow"
+OVS_PACKET_FAMILY = "ovs_packet"
+OVS_METER_FAMILY = "ovs_meter"
+OVS_CT_LIMIT_FAMILY = "ovs_ct_limit"
+
+OVS_DATAPATH_VERSION = 2
+OVS_HDR_LEN = 4
+OVS_DP_CMD_NEW = 1
+OVS_DP_CMD_DEL = 2
+OVS_DP_CMD_GET = 3
+OVS_DP_CMD_SET = 4
+
+OVS_DP_F_VPORT_PIDS = 1 << 1
+OVS_DP_F_DISPATCH_UPCALL_PER_CPU = 1 << 3
+
+OVS_DP_ATTR_NAME = 1
+OVS_DP_ATTR_UPCALL_PID = 2
+OVS_DP_ATTR_STATS = 3
+OVS_DP_ATTR_MEGAFLOW_STATS = 4
+OVS_DP_ATTR_USER_FEATURES = 5
+OVS_DP_ATTR_PAD = 6
+OVS_DP_ATTR_MASKS_CACHE_SIZE = 7
+OVS_DP_ATTR_PER_CPU_PIDS = 8
+OVS_DP_ATTR_MAX = 8
+
+OVS_VPORT_CMD_NEW = 1
+OVS_VPORT_CMD_DEL = 2
+OVS_VPORT_CMD_GET = 3
+OVS_VPORT_CMD_SET = 4
+
+OVS_VPORT_ATTR_PORT_NO = 1
+OVS_VPORT_ATTR_TYPE = 2
+OVS_VPORT_ATTR_NAME = 3
+OVS_VPORT_ATTR_OPTIONS = 4
+OVS_VPORT_ATTR_UPCALL_PID = 5
+OVS_VPORT_ATTR_STATS = 6
+OVS_VPORT_ATTR_PAD = 7
+OVS_VPORT_ATTR_IFINDEX = 8
+OVS_VPORT_ATTR_NETNSID = 9
+OVS_VPORT_ATTR_MAX = 9
+
+OVS_VPORT_TYPE_NETDEV = 1
+OVS_VPORT_TYPE_INTERNAL = 2
+OVS_VPORT_TYPE_GRE = 3
+OVS_VPORT_TYPE_VXLAN = 4
+OVS_VPORT_TYPE_GENEVE = 5
+OVS_VPORT_TYPE_MAX = 5
+
+
+def nl_sk_transaction(msg, sk, cb):
+    nl_socket_set_cb(sk, cb)
+    ret = nl_send_auto(sk, msg)
+    if ret < 0:
+        print("send error: ", end='')
+        print(errmsg[abs(ret)])
+    ret = nl_recvmsgs_default(sk)
+    if ret < 0:
+        print("recv error: ", end='')
+        print(errmsg[abs(ret)])
+    return ret
+
+
+def if_exists(ifname):
+    try:
+        socket.if_nametoindex(ifname)
+        return True
+    except OSError:
+        return False
+
+
+def get_family(ovs_family_name):
+    """
+    Retrieve a GENL family ID via the global nl socket
+    Returns: family ID for the requested family name
+    """
+    global sk
+    if sk is None:
+        raise ConnectionError("sk not correctly setup")
+    numid = genl_ctrl_resolve(sk, ovs_family_name.encode('utf-8'))
+    return numid
+
+
+def dpctl_netlink_init():
+    """
+    Initializes the global netlink socket, and ovs familly dictionary
+    Returns: 0 on success, any other value is error
+    """
+    global sk, ovs_families
+    sk = nl_socket_alloc()
+    ret = genl_connect(sk)
+    if ret:
+        print(errmsg[abs(ret)])
+        sk = None
+        return ret
+    ovs_families = {}
+    family_probe = [OVS_DATAPATH_FAMILY, OVS_VPORT_FAMILY, OVS_FLOW_FAMILY,
+                    OVS_PACKET_FAMILY, OVS_METER_FAMILY, OVS_CT_LIMIT_FAMILY]
+    for family in family_probe:
+        ovs_families[family] = get_family(family)
+        if ovs_families[family] == -1:
+            return -1
+    return 0
+
+
+def parse_dp_msg(nlh, target_dict):
+    dp_dict = {}
+    attrs = dict((i, None) for i in range(OVS_DP_ATTR_MAX))
+    dp_policy = dict((i, None) for i in range(OVS_DP_ATTR_MAX))
+    dp_policy.update({
+        OVS_DP_ATTR_NAME: nla_policy(type_=NLA_STRING, maxlen=15),
+        OVS_DP_ATTR_UPCALL_PID: nla_policy(type_=NLA_U32),
+        OVS_DP_ATTR_STATS: nla_policy(type_=NLA_NESTED),
+        OVS_DP_ATTR_MEGAFLOW_STATS: nla_policy(type_=NLA_NESTED),
+        OVS_DP_ATTR_USER_FEATURES: nla_policy(type_=NLA_U32),
+        OVS_DP_ATTR_MASKS_CACHE_SIZE: nla_policy(type_=NLA_U32),
+        OVS_DP_ATTR_PER_CPU_PIDS: nla_policy(type_=NLA_UNSPEC)
+    })
+    ret = genlmsg_parse(nlh, 4, attrs, OVS_DP_ATTR_MAX, dp_policy)
+    if ret:
+        print("Error parsing datapath")
+        return -1
+    if attrs[1] is None:
+        print("Error?")
+    dp_name = nla_get_string(attrs[1]).decode('utf-8')
+    b = bytes(attrs[OVS_DP_ATTR_STATS].payload)
+    stats = struct.unpack("=QQQQ", b[:32])
+    dp_dict[OVS_DP_ATTR_STATS] = stats
+    b = bytes(attrs[OVS_DP_ATTR_MEGAFLOW_STATS].payload)
+    stats = struct.unpack("=QIIQQ", b[:32])
+    dp_dict[OVS_DP_ATTR_MEGAFLOW_STATS] = [stats[i] for i in (0, 1, 3)]
+    dp_dict[OVS_DP_ATTR_MASKS_CACHE_SIZE] = nla_get_u32(
+        attrs[OVS_DP_ATTR_MASKS_CACHE_SIZE])
+    target_dict[dp_name] = dp_dict
+
+
+def show_dp_cb(msg, dp_dict):
+    nlh = nlmsg_hdr(msg)
+    if nlh.nlmsg_type == NLMSG_DONE:
+        retn = NL_STOP
+    parse_dp_msg(nlh, dp_dict)
+    retn = NL_OK
+    return retn
+
+
+def show_vport_cb(msg, dp_vport_dict):
+    dp, vport_dict = dp_vport_dict
+    nlh = nlmsg_hdr(msg)
+    retn = None
+    if nlh.nlmsg_type == NLMSG_DONE:
+        retn = NL_STOP
+    attrs = dict((i, None) for i in range(OVS_DP_ATTR_MAX))
+    port_policy = dict((i, None) for i in range(OVS_VPORT_ATTR_MAX))
+    port_policy.update({
+            OVS_VPORT_ATTR_PORT_NO: nla_policy(type_=NLA_U32),
+            OVS_VPORT_ATTR_TYPE: nla_policy(type_=NLA_U32),
+            OVS_VPORT_ATTR_NAME: nla_policy(type_=NLA_STRING, maxlen=15),
+            OVS_VPORT_ATTR_OPTIONS: nla_policy(type_=NLA_NESTED),
+            OVS_VPORT_ATTR_UPCALL_PID: nla_policy(type_=NLA_UNSPEC),
+            OVS_VPORT_ATTR_STATS: nla_policy(type_=NLA_NESTED),
+            OVS_VPORT_ATTR_IFINDEX: nla_policy(type_=NLA_U32),
+        })
+    genlmsg_parse(nlh, OVS_HDR_LEN, attrs, OVS_DP_ATTR_MAX, port_policy)
+    if attrs[1] is not None:
+        port_info = "Port " + str(nla_get_u32(attrs[1])) + ": "
+        if attrs[3] is not None:
+            port_info += nla_get_string(attrs[3]).decode('utf-8')
+            if attrs[OVS_VPORT_ATTR_TYPE] is not None:
+                port_type = nla_get_u32(attrs[OVS_VPORT_ATTR_TYPE])
+                if port_type == OVS_VPORT_TYPE_INTERNAL:
+                    port_info += " (internal)"
+    vport_dict[dp].append(port_info)
+    if retn is None:
+        retn = NL_OK
+    return retn
+
+
+def dpctl_show_print(dp_info, vport_info):
+    for i in dp_info:
+        print("{}".format(i))
+        indent = 2 * " "
+        fields = ("Hit", "Missed", "Lost", "Flows")
+        f_zip = zip(fields, dp_info[i][OVS_DP_ATTR_STATS])
+        format_list = [val for pair in f_zip for val in pair]
+        out_string = indent + "Lookups: {}: {} {}: {} {}: {}\n"
+        out_string += indent + "{}: {}"
+        print(out_string.format(*format_list))
+        fields = ("Hit", "Total", "Hit")
+        f_zip = zip(fields, dp_info[i][OVS_DP_ATTR_MEGAFLOW_STATS])
+        format_list = [val for pair in f_zip for val in pair]
+        out_string = indent + "Masks: {}: {} {}: {}\n"
+        out_string += indent + "Cache: {}: {}"
+        print(out_string.format(*format_list))
+        print("Caches:\n" + indent + "Masks-cache: size: {}".
+              format(dp_info[i][OVS_DP_ATTR_MASKS_CACHE_SIZE]))
+        indent = 4 * " "
+        for port in vport_info[i]:
+            print(indent + port)
+
+
+def dpctl_show(dp=None):
+    global sk, ovs_families
+    cb_dp_show = nl_cb_alloc(NL_CB_CUSTOM)
+    dp_info = {}
+    vport_info = {}
+    nl_cb_set(cb_dp_show, NL_CB_VALID, NL_CB_CUSTOM, show_dp_cb, dp_info)
+    msg_dpctl_get = nlmsg_alloc()
+    if dp is not None:
+        if not if_exists(dp):
+            print("That interface does not exist.")
+            return -1
+        flag = NLM_F_REQUEST
+    else:
+        flag = NLM_F_DUMP
+    genlmsg_put(msg_dpctl_get, 0, NL_AUTO_SEQ,
+                ovs_families[OVS_DATAPATH_FAMILY], OVS_HDR_LEN,
+                flag, OVS_DP_CMD_GET, OVS_DATAPATH_VERSION)
+    if dp is not None:
+        nla_put_string(msg_dpctl_get, OVS_DP_ATTR_NAME, dp.encode('utf-8'))
+    nl_sk_transaction(msg_dpctl_get, sk, cb_dp_show)
+    vport_info = dict((i, []) for i in dp_info)
+    # for each datapath, call down and ask it to tell us its vports.
+    for dp in vport_info:
+        msg_vport_get = nlmsg_alloc()
+        ba = genlmsg_put(msg_vport_get, 0, NL_AUTO_SEQ,
+                         ovs_families[OVS_VPORT_FAMILY], OVS_HDR_LEN,
+                         NLM_F_DUMP, OVS_VPORT_CMD_GET, OVS_DATAPATH_VERSION)
+        ba[0:OVS_HDR_LEN] = struct.pack('=I', socket.if_nametoindex(dp))
+        cb_vport_show = nl_cb_alloc(NL_CB_CUSTOM)
+        nl_cb_set(cb_vport_show, NL_CB_VALID, NL_CB_CUSTOM,
+                  show_vport_cb, (dp, vport_info))
+        nl_sk_transaction(msg_vport_get, sk, cb_vport_show)
+    dpctl_show_print(dp_info, vport_info)
+
+
+def mod_cb(msg, add):
+    nlh = nlmsg_hdr(msg)
+    if nlh.nlmsg_type == NLMSG_ERROR:
+        b = nlh.payload
+        s = struct.unpack('=i', b[:4])[0]
+        if s:
+            print(errmsg[abs(s)])
+            return NL_STOP
+    action = "added" if add else "deleted"
+    print("Successfully {} the datapath.".format(action))
+    return NL_OK
+
+
+def dpctl_mod_dp(args, add=True, setpid=False, hdrval=None):
+    global ovs_families, sk
+
+    dp = args[0]
+    cmd = OVS_DP_CMD_NEW if add else OVS_DP_CMD_DEL
+    msg_dpctl_cmd = nlmsg_alloc()
+
+    userfeatures = 0
+    if hdrval is None:
+        hdrver = OVS_DATAPATH_VERSION
+        userfeatures = OVS_DP_F_VPORT_PIDS
+    else:
+        segment = hdrval.find(":")
+        if segment == -1:
+            segment = len(hdrval)
+        hdrver = int(hdrval[:segment], 0)
+        if len(hdrval[:segment]):
+            userfeatures = int(hdrval[:segment], 0)
+
+    genlmsg_put(msg_dpctl_cmd, 0, NL_AUTO_SEQ,
+                ovs_families[OVS_DATAPATH_FAMILY], OVS_HDR_LEN,
+                NLM_F_ACK, cmd, hdrver)
+
+    nla_put_u32(msg_dpctl_cmd, OVS_DP_ATTR_UPCALL_PID, 0)
+    nla_put_string(msg_dpctl_cmd, OVS_DP_ATTR_NAME, dp.encode('utf-8'))
+
+    if setpid:
+        userfeatures &= ~OVS_DP_F_VPORT_PIDS
+        userfeatures |= OVS_DP_F_DISPATCH_UPCALL_PER_CPU
+        procarray = None
+        nproc = multiprocessing.cpu_count()
+        for i in range(nproc):
+            if procarray is not None:
+                procarray += struct.pack("=I", nl_socket_get_local_port(sk))
+            else:
+                procarray = struct.pack('=I', nl_socket_get_local_port(sk))
+        nla_put(msg_dpctl_cmd, OVS_DP_ATTR_UPCALL_PID, len(procarray),
+                procarray)
+    nla_put_u32(msg_dpctl_cmd, OVS_DP_ATTR_USER_FEATURES, userfeatures)
+    cb_dp_mod = nl_cb_alloc(NL_CB_CUSTOM)
+    nl_cb_set(cb_dp_mod, NL_CB_MSG_IN, NL_CB_CUSTOM, mod_cb, add)
+    return nl_sk_transaction(msg_dpctl_cmd, sk, cb_dp_mod)
+
+
+def dpctl_add_dp(dp):
+    setpid = False
+    dphdr = None
+    if len(dp) > 1:
+        for i in range(len(dp)):
+            if dp[i] == '-u':
+                setpid = True
+            elif dp[i] == '-V':
+                i += 1
+                dphdr = dp[i]
+
+    return dpctl_mod_dp(dp, True, setpid, dphdr)
+
+
+def dpctl_del_dp(dp):
+    args = [dp]
+    return dpctl_mod_dp(args, False)
+
+
+def help(errStr=None):
+    """
+    Display a help message, include errStr if there was an error.
+    Return: None
+    """
+    if errStr is None:
+        print("ovs-dpctl.py: openvswitch module controller")
+    else:
+        print(errStr)
+    print("usage:")
+    print("  show [DP]\t\t\tDispay information about all datapaths, or DP")
+    print("  add-dp DP\t\t\tAdd new datapath DP")
+    print("  del-dp DP\t\t\tDelete local datapath DP")
+
+
+def main(argv):
+    if len(argv) < 2:
+        help()
+        return 0
+    count = 1
+    for arg in argv[1:]:
+        count += 1
+        if arg in ("-v", "--verbose"):
+            logging.basicConfig(level=logging.DEBUG)
+        if arg in ("-h", "--help", "help"):
+            help()
+            return 0
+        if arg == "show":
+            dpctl_netlink_init()
+            if len(argv) <= count:
+                dpctl_show()
+            else:
+                dpctl_show(argv[count])
+            return 0
+        elif arg == "add-dp":
+            dpctl_netlink_init()
+            if len(argv) < 3:   # 3rd arg should be DP name or additional opts
+                help("Missing a DP name")
+                return -1
+            else:
+                dpctl_add_dp(argv[count:])
+            return 0
+        elif arg == "del-dp":
+            dpctl_netlink_init()
+            if len(argv) < 3:   # 3rd arg MUST be DP name
+                help("Missing a DP name")
+                return -1
+            else:
+                dpctl_del_dp(argv[count])
+            return 0
+    return 0
+
+
+if __name__ == "__main__":
+    sys.exit(main(sys.argv))
-- 
2.34.3


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH net 2/2] selftests: add openvswitch selftest suite
  2022-10-19 18:30 ` [PATCH net 2/2] selftests: add openvswitch selftest suite Aaron Conole
@ 2022-10-20 13:21   ` Ilya Maximets
  2022-10-20 15:32     ` Aaron Conole
  0 siblings, 1 reply; 9+ messages in thread
From: Ilya Maximets @ 2022-10-20 13:21 UTC (permalink / raw)
  To: Aaron Conole, netdev
  Cc: i.maximets, Pravin B Shelar, Jakub Kicinski, David S. Miller,
	Paolo Abeni, Eric Dumazet, Thomas Graf, Kevin Sprague, dev,
	Eelco Chaudron, Shuah Khan, linux-kernel, linux-kselftest

On 10/19/22 20:30, Aaron Conole wrote:
> Previous commit resolves a WARN splat that can be difficult to reproduce,
> but with the ovs-dpctl.py utility, it can be trivial.  Introduce a test
> case which creates a DP, and then downgrades the feature set.  This will
> include a utility 'ovs-dpctl.py' that can be extended to do additional
> work.
> 
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> Signed-off-by: Kevin Sprague <ksprague0711@gmail.com>
> ---
>  MAINTAINERS                                   |   1 +
>  tools/testing/selftests/Makefile              |   1 +
>  .../selftests/net/openvswitch/Makefile        |  13 +
>  .../selftests/net/openvswitch/openvswitch.sh  | 216 +++++++++
>  .../selftests/net/openvswitch/ovs-dpctl.py    | 411 ++++++++++++++++++
>  5 files changed, 642 insertions(+)
>  create mode 100644 tools/testing/selftests/net/openvswitch/Makefile
>  create mode 100755 tools/testing/selftests/net/openvswitch/openvswitch.sh
>  create mode 100644 tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index abbe88e1c50b..295a6b0fbe26 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15434,6 +15434,7 @@ S:	Maintained
>  W:	http://openvswitch.org
>  F:	include/uapi/linux/openvswitch.h
>  F:	net/openvswitch/
> +F:	tools/testing/selftests/net/openvswitch/
>  
>  OPERATING PERFORMANCE POINTS (OPP)
>  M:	Viresh Kumar <vireshk@kernel.org>

...

> +exit ${exitcode}
> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> new file mode 100644
> index 000000000000..791d76b7adcd
> --- /dev/null
> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> @@ -0,0 +1,411 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0
> +
> +# Controls the openvswitch module.  Part of the kselftest suite, but
> +# can be used for some diagnostic purpose as well.
> +
> +import logging
> +import multiprocessing
> +import socket
> +import struct
> +import sys
> +
> +try:
> +    from libnl.attr import NLA_NESTED, NLA_STRING, NLA_U32, NLA_UNSPEC
> +    from libnl.attr import nla_get_string, nla_get_u32
> +    from libnl.attr import nla_put, nla_put_string, nla_put_u32
> +    from libnl.attr import nla_policy
> +
> +    from libnl.error import errmsg
> +
> +    from libnl.genl.ctrl import genl_ctrl_resolve
> +    from libnl.genl.genl import genl_connect, genlmsg_parse, genlmsg_put
> +
> +    from libnl.handlers import nl_cb_alloc, nl_cb_set
> +    from libnl.handlers import NL_CB_CUSTOM, NL_CB_MSG_IN, NL_CB_VALID
> +    from libnl.handlers import NL_OK, NL_STOP
> +
> +    from libnl.linux_private.netlink import NLM_F_ACK, NLM_F_DUMP
> +    from libnl.linux_private.netlink import NLM_F_REQUEST, NLMSG_DONE
> +
> +    from libnl.msg import NL_AUTO_SEQ, nlmsg_alloc, nlmsg_hdr
> +
> +    from libnl.nl import NLMSG_ERROR, nl_recvmsgs_default, nl_send_auto
> +    from libnl.socket_ import nl_socket_alloc, nl_socket_set_cb
> +    from libnl.socket_ import nl_socket_get_local_port
> +except ModuleNotFoundError:
> +    print("Need to install the python libnl3 library.")


Hey, Aaron and Kevin.  Selftests sounds like a very important and
long overdue thing to add.  Thanks for working on this!

I have some worries about the libnl3 library though.  It doesn't
seem to be maintained well.  It it maintained by a single person,
it it was at least 3 different single persons over the last 7
years via forks.  It didn't get any significant development done
since 2015 as well and no commits at all for a last 1.5 years.
It is not packaged by any major distributions.
I'm talking about https://github.com/coolshou/libnl3 .  Please,
correct me if that is not the right one.  There are too many
libraies with the name libnl out there...  That is also not a great
sign.

The C library libnl (https://github.com/thom311/libnl) seems to
be well maintained in general.  It has experimental python
bindings which are not really supported much.  Python bindings
received only 2 actual code-changing commits in the last 7 years.
Both of them are just python 2/3 compatibility changes.
Maybe that is not that big of a deal since it's not really a
real python library, but a wrapper on top of a main C library.
However, I didn't find these python bindings to be packaged in
distributions.  And they seem to be not available in pip as well.
So, building them is kind of a pain.

There is another option which is pyroute2.  It positions itself
primarily as a netlink library and it does include an
pyroute2.netlink module indeed:
  https://github.com/svinota/pyroute2/tree/master/pyroute2/netlink
See the __init__.py for usage examples.

This one looks to me like the most trustworthy.  It is actively
used by everyone in the python networking world, e.g. by OpenStack.
And it is actively developed and maintained unlike other
netlink-related python projects.  It is also packaged in most of the
main distributions, so it's easy to install and use.  Many people
already have it installed for other purposes.

TBH, I didn't compare the functionality, but I'd expect that most
of the things we need are implemented.

What do you think?



On the other note, I'm not a real python developer, but the code
looks more like a C than a python even for me.  Firstly, I'd say
that it would be great to maintain some coding style, e.g. by
checking with flake8 and/or black.  See some issues/suggestions
provided by these tools below.

Secondly, we shouldd at least use argparse for argument parsing.
It's part of the standard library since python 3.2, so doens't
require any special dependencies to be installed.

Some parts of the code can probably be re-written to be more
"pythonic" as well, but I won't dive into that for now.  I didn't
review the code deep enough for that.

Best regards, Ilya Maximets.

$ flake8 --ignore=E203,W504,W503 ./dpctl.py
./dpctl.py:14:38: H301: one import per line
./dpctl.py:15:42: H301: one import per line
./dpctl.py:16:35: H301: one import per line
./dpctl.py:22:45: H301: one import per line
./dpctl.py:24:43: H301: one import per line
./dpctl.py:25:44: H301: one import per line
./dpctl.py:26:37: H301: one import per line
./dpctl.py:28:54: H301: one import per line
./dpctl.py:29:58: H301: one import per line
./dpctl.py:31:38: H301: one import per line
./dpctl.py:33:37: H301: one import per line
./dpctl.py:34:46: H301: one import per line
./dpctl.py:118:1: H404: multi line docstring should start without a leading new line
./dpctl.py:118:1: H405: multi line docstring summary not separated with an empty line
./dpctl.py:119:1: H404: multi line docstring should start without a leading new line
./dpctl.py:119:1: H405: multi line docstring summary not separated with an empty line
./dpctl.py:120:1: H404: multi line docstring should start without a leading new line
./dpctl.py:120:1: H405: multi line docstring summary not separated with an empty line
./dpctl.py:121:1: H404: multi line docstring should start without a leading new line
./dpctl.py:121:1: H405: multi line docstring summary not separated with an empty line
./dpctl.py:130:1: H404: multi line docstring should start without a leading new line
./dpctl.py:130:1: H405: multi line docstring summary not separated with an empty line
./dpctl.py:131:1: H404: multi line docstring should start without a leading new line
./dpctl.py:131:1: H405: multi line docstring summary not separated with an empty line
./dpctl.py:132:1: H404: multi line docstring should start without a leading new line
./dpctl.py:132:1: H405: multi line docstring summary not separated with an empty line
./dpctl.py:133:1: H404: multi line docstring should start without a leading new line
./dpctl.py:133:1: H405: multi line docstring summary not separated with an empty line
./dpctl.py:200:13: E126 continuation line over-indented for hanging indent
./dpctl.py:207:9: E121 continuation line under-indented for hanging indent
./dpctl.py:358:1: H404: multi line docstring should start without a leading new line
./dpctl.py:358:1: H405: multi line docstring summary not separated with an empty line
./dpctl.py:359:1: H404: multi line docstring should start without a leading new line
./dpctl.py:359:1: H405: multi line docstring summary not separated with an empty line
./dpctl.py:360:1: H404: multi line docstring should start without a leading new line
./dpctl.py:360:1: H405: multi line docstring summary not separated with an empty line
./dpctl.py:361:1: H404: multi line docstring should start without a leading new line
./dpctl.py:361:1: H405: multi line docstring summary not separated with an empty line

Black also provides some readability suggestions like this:

$ black -l 80 -S --check --diff dpctl.py 
--- dpctl.py    2022-10-19 21:24:34.352403 +0000
+++ dpctl.py    2022-10-20 13:15:29.095154 +0000
@@ -137,12 +137,18 @@
     if ret:
         print(errmsg[abs(ret)])
         sk = None
         return ret
     ovs_families = {}
-    family_probe = [OVS_DATAPATH_FAMILY, OVS_VPORT_FAMILY, OVS_FLOW_FAMILY,
-                    OVS_PACKET_FAMILY, OVS_METER_FAMILY, OVS_CT_LIMIT_FAMILY]
+    family_probe = [
+        OVS_DATAPATH_FAMILY,
+        OVS_VPORT_FAMILY,
+        OVS_FLOW_FAMILY,
+        OVS_PACKET_FAMILY,
+        OVS_METER_FAMILY,
+        OVS_CT_LIMIT_FAMILY,
+    ]
     for family in family_probe:
         ovs_families[family] = get_family(family)
         if ovs_families[family] == -1:
             return -1
     return 0
@@ -150,19 +156,21 @@
 
 def parse_dp_msg(nlh, target_dict):
     dp_dict = {}
     attrs = dict((i, None) for i in range(OVS_DP_ATTR_MAX))
     dp_policy = dict((i, None) for i in range(OVS_DP_ATTR_MAX))
-    dp_policy.update({
-        OVS_DP_ATTR_NAME: nla_policy(type_=NLA_STRING, maxlen=15),
-        OVS_DP_ATTR_UPCALL_PID: nla_policy(type_=NLA_U32),
-        OVS_DP_ATTR_STATS: nla_policy(type_=NLA_NESTED),
-        OVS_DP_ATTR_MEGAFLOW_STATS: nla_policy(type_=NLA_NESTED),
-        OVS_DP_ATTR_USER_FEATURES: nla_policy(type_=NLA_U32),
-        OVS_DP_ATTR_MASKS_CACHE_SIZE: nla_policy(type_=NLA_U32),
-        OVS_DP_ATTR_PER_CPU_PIDS: nla_policy(type_=NLA_UNSPEC)
-    })
+    dp_policy.update(
+        {
+            OVS_DP_ATTR_NAME: nla_policy(type_=NLA_STRING, maxlen=15),
+            OVS_DP_ATTR_UPCALL_PID: nla_policy(type_=NLA_U32),
+            OVS_DP_ATTR_STATS: nla_policy(type_=NLA_NESTED),
+            OVS_DP_ATTR_MEGAFLOW_STATS: nla_policy(type_=NLA_NESTED),
+            OVS_DP_ATTR_USER_FEATURES: nla_policy(type_=NLA_U32),
+            OVS_DP_ATTR_MASKS_CACHE_SIZE: nla_policy(type_=NLA_U32),
+            OVS_DP_ATTR_PER_CPU_PIDS: nla_policy(type_=NLA_UNSPEC),
+        }
+    )
     ret = genlmsg_parse(nlh, 4, attrs, OVS_DP_ATTR_MAX, dp_policy)
     if ret:
         print("Error parsing datapath")
         return -1
     if attrs[1] is None:
@@ -173,11 +181,12 @@
     dp_dict[OVS_DP_ATTR_STATS] = stats
     b = bytes(attrs[OVS_DP_ATTR_MEGAFLOW_STATS].payload)
     stats = struct.unpack("=QIIQQ", b[:32])
     dp_dict[OVS_DP_ATTR_MEGAFLOW_STATS] = [stats[i] for i in (0, 1, 3)]
     dp_dict[OVS_DP_ATTR_MASKS_CACHE_SIZE] = nla_get_u32(
-        attrs[OVS_DP_ATTR_MASKS_CACHE_SIZE])
+        attrs[OVS_DP_ATTR_MASKS_CACHE_SIZE]
+    )
     target_dict[dp_name] = dp_dict
 
 
 def show_dp_cb(msg, dp_dict):
     nlh = nlmsg_hdr(msg)
@@ -194,19 +203,21 @@
     retn = None
     if nlh.nlmsg_type == NLMSG_DONE:
         retn = NL_STOP
     attrs = dict((i, None) for i in range(OVS_DP_ATTR_MAX))
     port_policy = dict((i, None) for i in range(OVS_VPORT_ATTR_MAX))
-    port_policy.update({
+    port_policy.update(
+        {
             OVS_VPORT_ATTR_PORT_NO: nla_policy(type_=NLA_U32),
             OVS_VPORT_ATTR_TYPE: nla_policy(type_=NLA_U32),
             OVS_VPORT_ATTR_NAME: nla_policy(type_=NLA_STRING, maxlen=15),
             OVS_VPORT_ATTR_OPTIONS: nla_policy(type_=NLA_NESTED),
             OVS_VPORT_ATTR_UPCALL_PID: nla_policy(type_=NLA_UNSPEC),
             OVS_VPORT_ATTR_STATS: nla_policy(type_=NLA_NESTED),
             OVS_VPORT_ATTR_IFINDEX: nla_policy(type_=NLA_U32),
-        })
+        }
+    )
     genlmsg_parse(nlh, OVS_HDR_LEN, attrs, OVS_DP_ATTR_MAX, port_policy)
     if attrs[1] is not None:
         port_info = "Port " + str(nla_get_u32(attrs[1])) + ": "
         if attrs[3] is not None:
             port_info += nla_get_string(attrs[3]).decode('utf-8')
@@ -234,12 +245,17 @@
         f_zip = zip(fields, dp_info[i][OVS_DP_ATTR_MEGAFLOW_STATS])
         format_list = [val for pair in f_zip for val in pair]
         out_string = indent + "Masks: {}: {} {}: {}\n"
         out_string += indent + "Cache: {}: {}"
         print(out_string.format(*format_list))
-        print("Caches:\n" + indent + "Masks-cache: size: {}".
-              format(dp_info[i][OVS_DP_ATTR_MASKS_CACHE_SIZE]))
+        print(
+            "Caches:\n"
+            + indent
+            + "Masks-cache: size: {}".format(
+                dp_info[i][OVS_DP_ATTR_MASKS_CACHE_SIZE]
+            )
+        )
         indent = 4 * " "
         for port in vport_info[i]:
             print(indent + port)
 
 
@@ -255,27 +271,46 @@
             print("That interface does not exist.")
             return -1
         flag = NLM_F_REQUEST
     else:
         flag = NLM_F_DUMP
-    genlmsg_put(msg_dpctl_get, 0, NL_AUTO_SEQ,
-                ovs_families[OVS_DATAPATH_FAMILY], OVS_HDR_LEN,
-                flag, OVS_DP_CMD_GET, OVS_DATAPATH_VERSION)
+    genlmsg_put(
+        msg_dpctl_get,
+        0,
+        NL_AUTO_SEQ,
+        ovs_families[OVS_DATAPATH_FAMILY],
+        OVS_HDR_LEN,
+        flag,
+        OVS_DP_CMD_GET,
+        OVS_DATAPATH_VERSION,
+    )
     if dp is not None:
         nla_put_string(msg_dpctl_get, OVS_DP_ATTR_NAME, dp.encode('utf-8'))
     nl_sk_transaction(msg_dpctl_get, sk, cb_dp_show)
     vport_info = dict((i, []) for i in dp_info)
     # for each datapath, call down and ask it to tell us its vports.
     for dp in vport_info:
         msg_vport_get = nlmsg_alloc()
-        ba = genlmsg_put(msg_vport_get, 0, NL_AUTO_SEQ,
-                         ovs_families[OVS_VPORT_FAMILY], OVS_HDR_LEN,
-                         NLM_F_DUMP, OVS_VPORT_CMD_GET, OVS_DATAPATH_VERSION)
+        ba = genlmsg_put(
+            msg_vport_get,
+            0,
+            NL_AUTO_SEQ,
+            ovs_families[OVS_VPORT_FAMILY],
+            OVS_HDR_LEN,
+            NLM_F_DUMP,
+            OVS_VPORT_CMD_GET,
+            OVS_DATAPATH_VERSION,
+        )
         ba[0:OVS_HDR_LEN] = struct.pack('=I', socket.if_nametoindex(dp))
         cb_vport_show = nl_cb_alloc(NL_CB_CUSTOM)
-        nl_cb_set(cb_vport_show, NL_CB_VALID, NL_CB_CUSTOM,
-                  show_vport_cb, (dp, vport_info))
+        nl_cb_set(
+            cb_vport_show,
+            NL_CB_VALID,
+            NL_CB_CUSTOM,
+            show_vport_cb,
+            (dp, vport_info),
+        )
         nl_sk_transaction(msg_vport_get, sk, cb_vport_show)
     dpctl_show_print(dp_info, vport_info)
 
 
 def mod_cb(msg, add):
@@ -308,13 +343,20 @@
             segment = len(hdrval)
         hdrver = int(hdrval[:segment], 0)
         if len(hdrval[:segment]):
             userfeatures = int(hdrval[:segment], 0)
 
-    genlmsg_put(msg_dpctl_cmd, 0, NL_AUTO_SEQ,
-                ovs_families[OVS_DATAPATH_FAMILY], OVS_HDR_LEN,
-                NLM_F_ACK, cmd, hdrver)
+    genlmsg_put(
+        msg_dpctl_cmd,
+        0,
+        NL_AUTO_SEQ,
+        ovs_families[OVS_DATAPATH_FAMILY],
+        OVS_HDR_LEN,
+        NLM_F_ACK,
+        cmd,
+        hdrver,
+    )
 
     nla_put_u32(msg_dpctl_cmd, OVS_DP_ATTR_UPCALL_PID, 0)
     nla_put_string(msg_dpctl_cmd, OVS_DP_ATTR_NAME, dp.encode('utf-8'))
 
     if setpid:
@@ -325,12 +367,13 @@
         for i in range(nproc):
             if procarray is not None:
                 procarray += struct.pack("=I", nl_socket_get_local_port(sk))
             else:
                 procarray = struct.pack('=I', nl_socket_get_local_port(sk))
-        nla_put(msg_dpctl_cmd, OVS_DP_ATTR_UPCALL_PID, len(procarray),
-                procarray)
+        nla_put(
+            msg_dpctl_cmd, OVS_DP_ATTR_UPCALL_PID, len(procarray), procarray
+        )
     nla_put_u32(msg_dpctl_cmd, OVS_DP_ATTR_USER_FEATURES, userfeatures)
     cb_dp_mod = nl_cb_alloc(NL_CB_CUSTOM)
     nl_cb_set(cb_dp_mod, NL_CB_MSG_IN, NL_CB_CUSTOM, mod_cb, add)
     return nl_sk_transaction(msg_dpctl_cmd, sk, cb_dp_mod)
 
@@ -388,19 +431,19 @@
             else:
                 dpctl_show(argv[count])
             return 0
         elif arg == "add-dp":
             dpctl_netlink_init()
-            if len(argv) < 3:   # 3rd arg should be DP name or additional opts
+            if len(argv) < 3:  # 3rd arg should be DP name or additional opts
                 help("Missing a DP name")
                 return -1
             else:
                 dpctl_add_dp(argv[count:])
             return 0
         elif arg == "del-dp":
             dpctl_netlink_init()
-            if len(argv) < 3:   # 3rd arg MUST be DP name
+            if len(argv) < 3:  # 3rd arg MUST be DP name
                 help("Missing a DP name")
                 return -1
             else:
                 dpctl_del_dp(argv[count])
             return 0
---

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net 2/2] selftests: add openvswitch selftest suite
  2022-10-20 13:21   ` Ilya Maximets
@ 2022-10-20 15:32     ` Aaron Conole
  2022-10-20 16:33       ` Ilya Maximets
  0 siblings, 1 reply; 9+ messages in thread
From: Aaron Conole @ 2022-10-20 15:32 UTC (permalink / raw)
  To: Ilya Maximets
  Cc: netdev, Pravin B Shelar, Jakub Kicinski, David S. Miller,
	Paolo Abeni, Eric Dumazet, Thomas Graf, Kevin Sprague, dev,
	Eelco Chaudron, Shuah Khan, linux-kernel, linux-kselftest

Hi Ilya,

Ilya Maximets <i.maximets@ovn.org> writes:

> On 10/19/22 20:30, Aaron Conole wrote:
>> Previous commit resolves a WARN splat that can be difficult to reproduce,
>> but with the ovs-dpctl.py utility, it can be trivial.  Introduce a test
>> case which creates a DP, and then downgrades the feature set.  This will
>> include a utility 'ovs-dpctl.py' that can be extended to do additional
>> work.
>> 
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> Signed-off-by: Kevin Sprague <ksprague0711@gmail.com>
>> ---
>>  MAINTAINERS                                   |   1 +
>>  tools/testing/selftests/Makefile              |   1 +
>>  .../selftests/net/openvswitch/Makefile        |  13 +
>>  .../selftests/net/openvswitch/openvswitch.sh  | 216 +++++++++
>>  .../selftests/net/openvswitch/ovs-dpctl.py    | 411 ++++++++++++++++++
>>  5 files changed, 642 insertions(+)
>>  create mode 100644 tools/testing/selftests/net/openvswitch/Makefile
>>  create mode 100755 tools/testing/selftests/net/openvswitch/openvswitch.sh
>>  create mode 100644 tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index abbe88e1c50b..295a6b0fbe26 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -15434,6 +15434,7 @@ S:	Maintained
>>  W:	http://openvswitch.org
>>  F:	include/uapi/linux/openvswitch.h
>>  F:	net/openvswitch/
>> +F:	tools/testing/selftests/net/openvswitch/
>>  
>>  OPERATING PERFORMANCE POINTS (OPP)
>>  M:	Viresh Kumar <vireshk@kernel.org>
>
> ...
>
>> +exit ${exitcode}
>> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> new file mode 100644
>> index 000000000000..791d76b7adcd
>> --- /dev/null
>> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> @@ -0,0 +1,411 @@
>> +#!/usr/bin/env python3
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +# Controls the openvswitch module.  Part of the kselftest suite, but
>> +# can be used for some diagnostic purpose as well.
>> +
>> +import logging
>> +import multiprocessing
>> +import socket
>> +import struct
>> +import sys
>> +
>> +try:
>> +    from libnl.attr import NLA_NESTED, NLA_STRING, NLA_U32, NLA_UNSPEC
>> +    from libnl.attr import nla_get_string, nla_get_u32
>> +    from libnl.attr import nla_put, nla_put_string, nla_put_u32
>> +    from libnl.attr import nla_policy
>> +
>> +    from libnl.error import errmsg
>> +
>> +    from libnl.genl.ctrl import genl_ctrl_resolve
>> +    from libnl.genl.genl import genl_connect, genlmsg_parse, genlmsg_put
>> +
>> +    from libnl.handlers import nl_cb_alloc, nl_cb_set
>> +    from libnl.handlers import NL_CB_CUSTOM, NL_CB_MSG_IN, NL_CB_VALID
>> +    from libnl.handlers import NL_OK, NL_STOP
>> +
>> +    from libnl.linux_private.netlink import NLM_F_ACK, NLM_F_DUMP
>> +    from libnl.linux_private.netlink import NLM_F_REQUEST, NLMSG_DONE
>> +
>> +    from libnl.msg import NL_AUTO_SEQ, nlmsg_alloc, nlmsg_hdr
>> +
>> +    from libnl.nl import NLMSG_ERROR, nl_recvmsgs_default, nl_send_auto
>> +    from libnl.socket_ import nl_socket_alloc, nl_socket_set_cb
>> +    from libnl.socket_ import nl_socket_get_local_port
>> +except ModuleNotFoundError:
>> +    print("Need to install the python libnl3 library.")
>
>
> Hey, Aaron and Kevin.  Selftests sounds like a very important and
> long overdue thing to add.  Thanks for working on this!
>
> I have some worries about the libnl3 library though.  It doesn't
> seem to be maintained well.  It it maintained by a single person,
> it it was at least 3 different single persons over the last 7
> years via forks.  It didn't get any significant development done
> since 2015 as well and no commits at all for a last 1.5 years.
> It is not packaged by any major distributions.

:-/  On my fedora:

  11:12:24 aconole@RHTPC1VM0NT ~$ dnf search python3-libnl3
  Last metadata expiration check: 1 day, 0:25:11 ago on Wed 19 Oct 2022 10:47:21 AM EDT.
  ===================== Name Exactly Matched: python3-libnl3 =====================
  python3-libnl3.x86_64 : libnl3 binding for Python 3


And I can use it:

  11:18:39 aconole@RHTPC1VM0NT {(6a5c83bdd991...)} ~/git/linux/tools/testing/selftests/net/openvswitch$ sudo python3 ./ovs-dpctl.py show
  foop
    Lookups: Hit: 0 Missed: 0 Lost: 0
    Flows: 0
    Masks: Hit: 0 Total: 0
    Cache: Hit: 0
  Caches:
    Masks-cache: size: 256
      Port 0: foop (internal)
  11:18:43 aconole@RHTPC1VM0NT {(6a5c83bdd991...)} ~/git/linux/tools/testing/selftests/net/openvswitch$ rpm -qa | grep python3-libnl3
  python3-libnl3-3.5.0-6.fc34.x86_64
  11:19:01 aconole@RHTPC1VM0NT {(6a5c83bdd991...)} ~/git/linux/tools/testing/selftests/net/openvswitch$ 

Was there some place you did not find it?

> I'm talking about https://github.com/coolshou/libnl3 .  Please,
> correct me if that is not the right one.  There are too many
> libraies with the name libnl out there...  That is also not a great
> sign.

Yes, this is the project.  We did look at some of the ones you
mentioned, but didn't find much.

It is a sparse landscape of projects that provide netlink support in
python.

> The C library libnl (https://github.com/thom311/libnl) seems to
> be well maintained in general.  It has experimental python
> bindings which are not really supported much.  Python bindings
> received only 2 actual code-changing commits in the last 7 years.
> Both of them are just python 2/3 compatibility changes.
> Maybe that is not that big of a deal since it's not really a
> real python library, but a wrapper on top of a main C library.
> However, I didn't find these python bindings to be packaged in
> distributions.  And they seem to be not available in pip as well.
> So, building them is kind of a pain.

Well, the python libnl3 should be installable via pip3.  Ex:

  11:27:15 aconole@RHTPC1VM0NT ~$ pip3 install libnl3
  Defaulting to user installation because normal site-packages is not writeable
  Collecting libnl3
    Using cached libnl3-0.3.0-py3-none-any.whl (89 kB)
  Installing collected packages: libnl3
  Successfully installed libnl3-0.3.0

So I guess that is worth something.

At least on Fedora it is installable from distribution as well.

> There is another option which is pyroute2.  It positions itself
> primarily as a netlink library and it does include an
> pyroute2.netlink module indeed:
>   https://github.com/svinota/pyroute2/tree/master/pyroute2/netlink
> See the __init__.py for usage examples.
>
> This one looks to me like the most trustworthy.  It is actively
> used by everyone in the python networking world, e.g. by OpenStack.
> And it is actively developed and maintained unlike other
> netlink-related python projects.  It is also packaged in most of the
> main distributions, so it's easy to install and use.  Many people
> already have it installed for other purposes.
>
> TBH, I didn't compare the functionality, but I'd expect that most
> of the things we need are implemented.
>
> What do you think?

We can certainly look at switching, but having a quick glance, it seems
pyroute2 expects to provide the genl commands as well, so they would
want us to create an ovs module in pyroute2 that includes all of the ovs
family support.  Of course, we can always do this just in our module,
but I think it isn't the way pyroute2 project wants to be structured.
More like a library that provides all the command functionality.

> On the other note, I'm not a real python developer, but the code
> looks more like a C than a python even for me.  Firstly, I'd say
> that it would be great to maintain some coding style, e.g. by
> checking with flake8 and/or black.  See some issues/suggestions
> provided by these tools below.

Agreed.  BTW, on the rhel8 system I developed on:

  [root@wsfd-netdev60 openvswitch]# flake8 ./ovs-dpctl.py 
  [root@wsfd-netdev60 openvswitch]# 

So, I guess it is probably that I should have used a different system to
do the flake8 checks.

> Secondly, we shouldd at least use argparse for argument parsing.
> It's part of the standard library since python 3.2, so doens't
> require any special dependencies to be installed.

Okay - I can switch to argparse.  TBH, I haven't kept up with python
standard library for some time.

> Some parts of the code can probably be re-written to be more
> "pythonic" as well, but I won't dive into that for now.  I didn't
> review the code deep enough for that.

I have difficulty sometimes understanding what it means to be "Real
Python (tm)" - I don't plan to change things too much.  I can certainly
switch to using argparse, but unless you give something you want to
change, I would not change anything.

> Best regards, Ilya Maximets.
>
> $ flake8 --ignore=E203,W504,W503 ./dpctl.py
> ./dpctl.py:14:38: H301: one import per line
> ./dpctl.py:15:42: H301: one import per line
> ./dpctl.py:16:35: H301: one import per line
> ./dpctl.py:22:45: H301: one import per line
> ./dpctl.py:24:43: H301: one import per line
> ./dpctl.py:25:44: H301: one import per line
> ./dpctl.py:26:37: H301: one import per line
> ./dpctl.py:28:54: H301: one import per line
> ./dpctl.py:29:58: H301: one import per line
> ./dpctl.py:31:38: H301: one import per line
> ./dpctl.py:33:37: H301: one import per line
> ./dpctl.py:34:46: H301: one import per line
> ./dpctl.py:118:1: H404: multi line docstring should start without a leading new line
> ./dpctl.py:118:1: H405: multi line docstring summary not separated with an empty line
> ./dpctl.py:119:1: H404: multi line docstring should start without a leading new line
> ./dpctl.py:119:1: H405: multi line docstring summary not separated with an empty line
> ./dpctl.py:120:1: H404: multi line docstring should start without a leading new line
> ./dpctl.py:120:1: H405: multi line docstring summary not separated with an empty line
> ./dpctl.py:121:1: H404: multi line docstring should start without a leading new line
> ./dpctl.py:121:1: H405: multi line docstring summary not separated with an empty line
> ./dpctl.py:130:1: H404: multi line docstring should start without a leading new line
> ./dpctl.py:130:1: H405: multi line docstring summary not separated with an empty line
> ./dpctl.py:131:1: H404: multi line docstring should start without a leading new line
> ./dpctl.py:131:1: H405: multi line docstring summary not separated with an empty line
> ./dpctl.py:132:1: H404: multi line docstring should start without a leading new line
> ./dpctl.py:132:1: H405: multi line docstring summary not separated with an empty line
> ./dpctl.py:133:1: H404: multi line docstring should start without a leading new line
> ./dpctl.py:133:1: H405: multi line docstring summary not separated with an empty line
> ./dpctl.py:200:13: E126 continuation line over-indented for hanging indent
> ./dpctl.py:207:9: E121 continuation line under-indented for hanging indent
> ./dpctl.py:358:1: H404: multi line docstring should start without a leading new line
> ./dpctl.py:358:1: H405: multi line docstring summary not separated with an empty line
> ./dpctl.py:359:1: H404: multi line docstring should start without a leading new line
> ./dpctl.py:359:1: H405: multi line docstring summary not separated with an empty line
> ./dpctl.py:360:1: H404: multi line docstring should start without a leading new line
> ./dpctl.py:360:1: H405: multi line docstring summary not separated with an empty line
> ./dpctl.py:361:1: H404: multi line docstring should start without a leading new line
> ./dpctl.py:361:1: H405: multi line docstring summary not separated with an empty line
>
> Black also provides some readability suggestions like this:

Okay, I'll install black on my system and do some development there.
Additionally, I will use flake8 on my system.

> $ black -l 80 -S --check --diff dpctl.py 
> --- dpctl.py    2022-10-19 21:24:34.352403 +0000
> +++ dpctl.py    2022-10-20 13:15:29.095154 +0000
> @@ -137,12 +137,18 @@
>      if ret:
>          print(errmsg[abs(ret)])
>          sk = None
>          return ret
>      ovs_families = {}
> -    family_probe = [OVS_DATAPATH_FAMILY, OVS_VPORT_FAMILY, OVS_FLOW_FAMILY,
> -                    OVS_PACKET_FAMILY, OVS_METER_FAMILY, OVS_CT_LIMIT_FAMILY]
> +    family_probe = [
> +        OVS_DATAPATH_FAMILY,
> +        OVS_VPORT_FAMILY,
> +        OVS_FLOW_FAMILY,
> +        OVS_PACKET_FAMILY,
> +        OVS_METER_FAMILY,
> +        OVS_CT_LIMIT_FAMILY,
> +    ]
>      for family in family_probe:
>          ovs_families[family] = get_family(family)
>          if ovs_families[family] == -1:
>              return -1
>      return 0
> @@ -150,19 +156,21 @@
>  
>  def parse_dp_msg(nlh, target_dict):
>      dp_dict = {}
>      attrs = dict((i, None) for i in range(OVS_DP_ATTR_MAX))
>      dp_policy = dict((i, None) for i in range(OVS_DP_ATTR_MAX))
> -    dp_policy.update({
> -        OVS_DP_ATTR_NAME: nla_policy(type_=NLA_STRING, maxlen=15),
> -        OVS_DP_ATTR_UPCALL_PID: nla_policy(type_=NLA_U32),
> -        OVS_DP_ATTR_STATS: nla_policy(type_=NLA_NESTED),
> -        OVS_DP_ATTR_MEGAFLOW_STATS: nla_policy(type_=NLA_NESTED),
> -        OVS_DP_ATTR_USER_FEATURES: nla_policy(type_=NLA_U32),
> -        OVS_DP_ATTR_MASKS_CACHE_SIZE: nla_policy(type_=NLA_U32),
> -        OVS_DP_ATTR_PER_CPU_PIDS: nla_policy(type_=NLA_UNSPEC)
> -    })
> +    dp_policy.update(
> +        {
> +            OVS_DP_ATTR_NAME: nla_policy(type_=NLA_STRING, maxlen=15),
> +            OVS_DP_ATTR_UPCALL_PID: nla_policy(type_=NLA_U32),
> +            OVS_DP_ATTR_STATS: nla_policy(type_=NLA_NESTED),
> +            OVS_DP_ATTR_MEGAFLOW_STATS: nla_policy(type_=NLA_NESTED),
> +            OVS_DP_ATTR_USER_FEATURES: nla_policy(type_=NLA_U32),
> +            OVS_DP_ATTR_MASKS_CACHE_SIZE: nla_policy(type_=NLA_U32),
> +            OVS_DP_ATTR_PER_CPU_PIDS: nla_policy(type_=NLA_UNSPEC),
> +        }
> +    )
>      ret = genlmsg_parse(nlh, 4, attrs, OVS_DP_ATTR_MAX, dp_policy)
>      if ret:
>          print("Error parsing datapath")
>          return -1
>      if attrs[1] is None:
> @@ -173,11 +181,12 @@
>      dp_dict[OVS_DP_ATTR_STATS] = stats
>      b = bytes(attrs[OVS_DP_ATTR_MEGAFLOW_STATS].payload)
>      stats = struct.unpack("=QIIQQ", b[:32])
>      dp_dict[OVS_DP_ATTR_MEGAFLOW_STATS] = [stats[i] for i in (0, 1, 3)]
>      dp_dict[OVS_DP_ATTR_MASKS_CACHE_SIZE] = nla_get_u32(
> -        attrs[OVS_DP_ATTR_MASKS_CACHE_SIZE])
> +        attrs[OVS_DP_ATTR_MASKS_CACHE_SIZE]
> +    )
>      target_dict[dp_name] = dp_dict
>  
>  
>  def show_dp_cb(msg, dp_dict):
>      nlh = nlmsg_hdr(msg)
> @@ -194,19 +203,21 @@
>      retn = None
>      if nlh.nlmsg_type == NLMSG_DONE:
>          retn = NL_STOP
>      attrs = dict((i, None) for i in range(OVS_DP_ATTR_MAX))
>      port_policy = dict((i, None) for i in range(OVS_VPORT_ATTR_MAX))
> -    port_policy.update({
> +    port_policy.update(
> +        {
>              OVS_VPORT_ATTR_PORT_NO: nla_policy(type_=NLA_U32),
>              OVS_VPORT_ATTR_TYPE: nla_policy(type_=NLA_U32),
>              OVS_VPORT_ATTR_NAME: nla_policy(type_=NLA_STRING, maxlen=15),
>              OVS_VPORT_ATTR_OPTIONS: nla_policy(type_=NLA_NESTED),
>              OVS_VPORT_ATTR_UPCALL_PID: nla_policy(type_=NLA_UNSPEC),
>              OVS_VPORT_ATTR_STATS: nla_policy(type_=NLA_NESTED),
>              OVS_VPORT_ATTR_IFINDEX: nla_policy(type_=NLA_U32),
> -        })
> +        }
> +    )
>      genlmsg_parse(nlh, OVS_HDR_LEN, attrs, OVS_DP_ATTR_MAX, port_policy)
>      if attrs[1] is not None:
>          port_info = "Port " + str(nla_get_u32(attrs[1])) + ": "
>          if attrs[3] is not None:
>              port_info += nla_get_string(attrs[3]).decode('utf-8')
> @@ -234,12 +245,17 @@
>          f_zip = zip(fields, dp_info[i][OVS_DP_ATTR_MEGAFLOW_STATS])
>          format_list = [val for pair in f_zip for val in pair]
>          out_string = indent + "Masks: {}: {} {}: {}\n"
>          out_string += indent + "Cache: {}: {}"
>          print(out_string.format(*format_list))
> -        print("Caches:\n" + indent + "Masks-cache: size: {}".
> -              format(dp_info[i][OVS_DP_ATTR_MASKS_CACHE_SIZE]))
> +        print(
> +            "Caches:\n"
> +            + indent
> +            + "Masks-cache: size: {}".format(
> +                dp_info[i][OVS_DP_ATTR_MASKS_CACHE_SIZE]
> +            )
> +        )
>          indent = 4 * " "
>          for port in vport_info[i]:
>              print(indent + port)
>  
>  
> @@ -255,27 +271,46 @@
>              print("That interface does not exist.")
>              return -1
>          flag = NLM_F_REQUEST
>      else:
>          flag = NLM_F_DUMP
> -    genlmsg_put(msg_dpctl_get, 0, NL_AUTO_SEQ,
> -                ovs_families[OVS_DATAPATH_FAMILY], OVS_HDR_LEN,
> -                flag, OVS_DP_CMD_GET, OVS_DATAPATH_VERSION)
> +    genlmsg_put(
> +        msg_dpctl_get,
> +        0,
> +        NL_AUTO_SEQ,
> +        ovs_families[OVS_DATAPATH_FAMILY],
> +        OVS_HDR_LEN,
> +        flag,
> +        OVS_DP_CMD_GET,
> +        OVS_DATAPATH_VERSION,
> +    )
>      if dp is not None:
>          nla_put_string(msg_dpctl_get, OVS_DP_ATTR_NAME, dp.encode('utf-8'))
>      nl_sk_transaction(msg_dpctl_get, sk, cb_dp_show)
>      vport_info = dict((i, []) for i in dp_info)
>      # for each datapath, call down and ask it to tell us its vports.
>      for dp in vport_info:
>          msg_vport_get = nlmsg_alloc()
> -        ba = genlmsg_put(msg_vport_get, 0, NL_AUTO_SEQ,
> -                         ovs_families[OVS_VPORT_FAMILY], OVS_HDR_LEN,
> -                         NLM_F_DUMP, OVS_VPORT_CMD_GET, OVS_DATAPATH_VERSION)
> +        ba = genlmsg_put(
> +            msg_vport_get,
> +            0,
> +            NL_AUTO_SEQ,
> +            ovs_families[OVS_VPORT_FAMILY],
> +            OVS_HDR_LEN,
> +            NLM_F_DUMP,
> +            OVS_VPORT_CMD_GET,
> +            OVS_DATAPATH_VERSION,
> +        )
>          ba[0:OVS_HDR_LEN] = struct.pack('=I', socket.if_nametoindex(dp))
>          cb_vport_show = nl_cb_alloc(NL_CB_CUSTOM)
> -        nl_cb_set(cb_vport_show, NL_CB_VALID, NL_CB_CUSTOM,
> -                  show_vport_cb, (dp, vport_info))
> +        nl_cb_set(
> +            cb_vport_show,
> +            NL_CB_VALID,
> +            NL_CB_CUSTOM,
> +            show_vport_cb,
> +            (dp, vport_info),
> +        )
>          nl_sk_transaction(msg_vport_get, sk, cb_vport_show)
>      dpctl_show_print(dp_info, vport_info)
>  
>  
>  def mod_cb(msg, add):
> @@ -308,13 +343,20 @@
>              segment = len(hdrval)
>          hdrver = int(hdrval[:segment], 0)
>          if len(hdrval[:segment]):
>              userfeatures = int(hdrval[:segment], 0)
>  
> -    genlmsg_put(msg_dpctl_cmd, 0, NL_AUTO_SEQ,
> -                ovs_families[OVS_DATAPATH_FAMILY], OVS_HDR_LEN,
> -                NLM_F_ACK, cmd, hdrver)
> +    genlmsg_put(
> +        msg_dpctl_cmd,
> +        0,
> +        NL_AUTO_SEQ,
> +        ovs_families[OVS_DATAPATH_FAMILY],
> +        OVS_HDR_LEN,
> +        NLM_F_ACK,
> +        cmd,
> +        hdrver,
> +    )
>  
>      nla_put_u32(msg_dpctl_cmd, OVS_DP_ATTR_UPCALL_PID, 0)
>      nla_put_string(msg_dpctl_cmd, OVS_DP_ATTR_NAME, dp.encode('utf-8'))
>  
>      if setpid:
> @@ -325,12 +367,13 @@
>          for i in range(nproc):
>              if procarray is not None:
>                  procarray += struct.pack("=I", nl_socket_get_local_port(sk))
>              else:
>                  procarray = struct.pack('=I', nl_socket_get_local_port(sk))
> -        nla_put(msg_dpctl_cmd, OVS_DP_ATTR_UPCALL_PID, len(procarray),
> -                procarray)
> +        nla_put(
> +            msg_dpctl_cmd, OVS_DP_ATTR_UPCALL_PID, len(procarray), procarray
> +        )
>      nla_put_u32(msg_dpctl_cmd, OVS_DP_ATTR_USER_FEATURES, userfeatures)
>      cb_dp_mod = nl_cb_alloc(NL_CB_CUSTOM)
>      nl_cb_set(cb_dp_mod, NL_CB_MSG_IN, NL_CB_CUSTOM, mod_cb, add)
>      return nl_sk_transaction(msg_dpctl_cmd, sk, cb_dp_mod)
>  
> @@ -388,19 +431,19 @@
>              else:
>                  dpctl_show(argv[count])
>              return 0
>          elif arg == "add-dp":
>              dpctl_netlink_init()
> -            if len(argv) < 3:   # 3rd arg should be DP name or additional opts
> +            if len(argv) < 3:  # 3rd arg should be DP name or additional opts
>                  help("Missing a DP name")
>                  return -1
>              else:
>                  dpctl_add_dp(argv[count:])
>              return 0
>          elif arg == "del-dp":
>              dpctl_netlink_init()
> -            if len(argv) < 3:   # 3rd arg MUST be DP name
> +            if len(argv) < 3:  # 3rd arg MUST be DP name
>                  help("Missing a DP name")
>                  return -1
>              else:
>                  dpctl_del_dp(argv[count])
>              return 0
> ---


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net 2/2] selftests: add openvswitch selftest suite
  2022-10-20 15:32     ` Aaron Conole
@ 2022-10-20 16:33       ` Ilya Maximets
  2022-10-20 17:17         ` Ilya Maximets
  0 siblings, 1 reply; 9+ messages in thread
From: Ilya Maximets @ 2022-10-20 16:33 UTC (permalink / raw)
  To: Aaron Conole
  Cc: i.maximets, netdev, Pravin B Shelar, Jakub Kicinski,
	David S. Miller, Paolo Abeni, Eric Dumazet, Thomas Graf,
	Kevin Sprague, dev, Eelco Chaudron, Shuah Khan, linux-kernel,
	linux-kselftest

On 10/20/22 17:32, Aaron Conole wrote:
> Hi Ilya,
> 
> Ilya Maximets <i.maximets@ovn.org> writes:
> 
>> On 10/19/22 20:30, Aaron Conole wrote:
>>> Previous commit resolves a WARN splat that can be difficult to reproduce,
>>> but with the ovs-dpctl.py utility, it can be trivial.  Introduce a test
>>> case which creates a DP, and then downgrades the feature set.  This will
>>> include a utility 'ovs-dpctl.py' that can be extended to do additional
>>> work.
>>>
>>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>>> Signed-off-by: Kevin Sprague <ksprague0711@gmail.com>
>>> ---
>>>  MAINTAINERS                                   |   1 +
>>>  tools/testing/selftests/Makefile              |   1 +
>>>  .../selftests/net/openvswitch/Makefile        |  13 +
>>>  .../selftests/net/openvswitch/openvswitch.sh  | 216 +++++++++
>>>  .../selftests/net/openvswitch/ovs-dpctl.py    | 411 ++++++++++++++++++
>>>  5 files changed, 642 insertions(+)
>>>  create mode 100644 tools/testing/selftests/net/openvswitch/Makefile
>>>  create mode 100755 tools/testing/selftests/net/openvswitch/openvswitch.sh
>>>  create mode 100644 tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index abbe88e1c50b..295a6b0fbe26 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -15434,6 +15434,7 @@ S:	Maintained
>>>  W:	http://openvswitch.org
>>>  F:	include/uapi/linux/openvswitch.h
>>>  F:	net/openvswitch/
>>> +F:	tools/testing/selftests/net/openvswitch/
>>>  
>>>  OPERATING PERFORMANCE POINTS (OPP)
>>>  M:	Viresh Kumar <vireshk@kernel.org>
>>
>> ...
>>
>>> +exit ${exitcode}
>>> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>>> b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>>> new file mode 100644
>>> index 000000000000..791d76b7adcd
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>>> @@ -0,0 +1,411 @@
>>> +#!/usr/bin/env python3
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +
>>> +# Controls the openvswitch module.  Part of the kselftest suite, but
>>> +# can be used for some diagnostic purpose as well.
>>> +
>>> +import logging
>>> +import multiprocessing
>>> +import socket
>>> +import struct
>>> +import sys
>>> +
>>> +try:
>>> +    from libnl.attr import NLA_NESTED, NLA_STRING, NLA_U32, NLA_UNSPEC
>>> +    from libnl.attr import nla_get_string, nla_get_u32
>>> +    from libnl.attr import nla_put, nla_put_string, nla_put_u32
>>> +    from libnl.attr import nla_policy
>>> +
>>> +    from libnl.error import errmsg
>>> +
>>> +    from libnl.genl.ctrl import genl_ctrl_resolve
>>> +    from libnl.genl.genl import genl_connect, genlmsg_parse, genlmsg_put
>>> +
>>> +    from libnl.handlers import nl_cb_alloc, nl_cb_set
>>> +    from libnl.handlers import NL_CB_CUSTOM, NL_CB_MSG_IN, NL_CB_VALID
>>> +    from libnl.handlers import NL_OK, NL_STOP
>>> +
>>> +    from libnl.linux_private.netlink import NLM_F_ACK, NLM_F_DUMP
>>> +    from libnl.linux_private.netlink import NLM_F_REQUEST, NLMSG_DONE
>>> +
>>> +    from libnl.msg import NL_AUTO_SEQ, nlmsg_alloc, nlmsg_hdr
>>> +
>>> +    from libnl.nl import NLMSG_ERROR, nl_recvmsgs_default, nl_send_auto
>>> +    from libnl.socket_ import nl_socket_alloc, nl_socket_set_cb
>>> +    from libnl.socket_ import nl_socket_get_local_port
>>> +except ModuleNotFoundError:
>>> +    print("Need to install the python libnl3 library.")
>>
>>
>> Hey, Aaron and Kevin.  Selftests sounds like a very important and
>> long overdue thing to add.  Thanks for working on this!
>>
>> I have some worries about the libnl3 library though.  It doesn't
>> seem to be maintained well.  It it maintained by a single person,
>> it it was at least 3 different single persons over the last 7
>> years via forks.  It didn't get any significant development done
>> since 2015 as well and no commits at all for a last 1.5 years.
>> It is not packaged by any major distributions.
> 
> :-/  On my fedora:
> 
>   11:12:24 aconole@RHTPC1VM0NT ~$ dnf search python3-libnl3
>   Last metadata expiration check: 1 day, 0:25:11 ago on Wed 19 Oct 2022 10:47:21 AM EDT.
>   ===================== Name Exactly Matched: python3-libnl3 =====================
>   python3-libnl3.x86_64 : libnl3 binding for Python 3
> 
> 
> And I can use it:
> 
>   11:18:39 aconole@RHTPC1VM0NT {(6a5c83bdd991...)} ~/git/linux/tools/testing/selftests/net/openvswitch$ sudo python3 ./ovs-dpctl.py show
>   foop
>     Lookups: Hit: 0 Missed: 0 Lost: 0
>     Flows: 0
>     Masks: Hit: 0 Total: 0
>     Cache: Hit: 0
>   Caches:
>     Masks-cache: size: 256
>       Port 0: foop (internal)
>   11:18:43 aconole@RHTPC1VM0NT {(6a5c83bdd991...)} ~/git/linux/tools/testing/selftests/net/openvswitch$ rpm -qa | grep python3-libnl3
>   python3-libnl3-3.5.0-6.fc34.x86_64
>   11:19:01 aconole@RHTPC1VM0NT {(6a5c83bdd991...)} ~/git/linux/tools/testing/selftests/net/openvswitch$ 
> 
> Was there some place you did not find it?

You're right, I missed that somehow.  But this is not an
https://github.com/coolshou/libnl3 project. :)
These are python bindings for the C libnl library:

$ dnf info python3-libnl3
Available Packages
Name         : python3-libnl3
Version      : 3.7.0
Release      : 1.fc36
Architecture : x86_64
Size         : 153 k
Source       : libnl3-3.7.0-1.fc36.src.rpm
Repository   : updates
Summary      : libnl3 binding for Python 3
URL          : http://www.infradead.org/~tgr/libnl/
License      : LGPLv2
Description  : Python 3 bindings for libnl3

> 
>> I'm talking about https://github.com/coolshou/libnl3 .  Please,
>> correct me if that is not the right one.  There are too many
>> libraies with the name libnl out there...  That is also not a great
>> sign.
> 
> Yes, this is the project.

Doensn't look like it...

> We did look at some of the ones you
> mentioned, but didn't find much.
> 
> It is a sparse landscape of projects that provide netlink support in
> python.
> 
>> The C library libnl (https://github.com/thom311/libnl) seems to
>> be well maintained in general.  It has experimental python
>> bindings which are not really supported much.  Python bindings
>> received only 2 actual code-changing commits in the last 7 years.
>> Both of them are just python 2/3 compatibility changes.
>> Maybe that is not that big of a deal since it's not really a
>> real python library, but a wrapper on top of a main C library.
>> However, I didn't find these python bindings to be packaged in
>> distributions.  And they seem to be not available in pip as well.
>> So, building them is kind of a pain.
> 
> Well, the python libnl3 should be installable via pip3.  Ex:
> 
>   11:27:15 aconole@RHTPC1VM0NT ~$ pip3 install libnl3
>   Defaulting to user installation because normal site-packages is not writeable
>   Collecting libnl3
>     Using cached libnl3-0.3.0-py3-none-any.whl (89 kB)
>   Installing collected packages: libnl3
>   Successfully installed libnl3-0.3.0

And this is https://pypi.org/project/libnl3/, which is the
https://github.com/coolshou/libnl3 project.  So, by installing
libnl3 via pip and installing python3-libnl3 from the fedora
you're getting two completely different libraries.

So, which one users should use?

I can't find python bindings for the C libnl (which is the
python3-libnl3 package) in pypi, so it can't be installed
with pip.


> 
> So I guess that is worth something.
> 
> At least on Fedora it is installable from distribution as well.
> 
>> There is another option which is pyroute2.  It positions itself
>> primarily as a netlink library and it does include an
>> pyroute2.netlink module indeed:
>>   https://github.com/svinota/pyroute2/tree/master/pyroute2/netlink
>> See the __init__.py for usage examples.
>>
>> This one looks to me like the most trustworthy.  It is actively
>> used by everyone in the python networking world, e.g. by OpenStack.
>> And it is actively developed and maintained unlike other
>> netlink-related python projects.  It is also packaged in most of the
>> main distributions, so it's easy to install and use.  Many people
>> already have it installed for other purposes.
>>
>> TBH, I didn't compare the functionality, but I'd expect that most
>> of the things we need are implemented.
>>
>> What do you think?
> 
> We can certainly look at switching, but having a quick glance, it seems
> pyroute2 expects to provide the genl commands as well, so they would
> want us to create an ovs module in pyroute2 that includes all of the ovs
> family support.  Of course, we can always do this just in our module,
> but I think it isn't the way pyroute2 project wants to be structured.
> More like a library that provides all the command functionality.

What I was thinking is to import pyroute2.netlink and the
pyroute2.netlink.generic  and go from there.  But I didn't
look too deep on how to actually implement the functionality.

The python bindings for the C libnl (python3-libnl3) sounds
like a fine option since they are actually packaged in
distributions (missed that in my initial reply).  However,
the fact that you can not install them via pip and actually
you will install something but completely different is kind
of weird.  This has to be at least better documented, so
users will know what to install and they will not try to use
pip for that.

> 
>> On the other note, I'm not a real python developer, but the code
>> looks more like a C than a python even for me.  Firstly, I'd say
>> that it would be great to maintain some coding style, e.g. by
>> checking with flake8 and/or black.  See some issues/suggestions
>> provided by these tools below.
> 
> Agreed.  BTW, on the rhel8 system I developed on:
> 
>   [root@wsfd-netdev60 openvswitch]# flake8 ./ovs-dpctl.py 
>   [root@wsfd-netdev60 openvswitch]# 
> 
> So, I guess it is probably that I should have used a different system to
> do the flake8 checks.

Maybe the python version is different...  I was running on f36
with python 3.10.  Also, the list of defaults might be different.
flake8 doesn't use default ignore list if one is explicitly provided.

> 
>> Secondly, we shouldd at least use argparse for argument parsing.
>> It's part of the standard library since python 3.2, so doens't
>> require any special dependencies to be installed.
> 
> Okay - I can switch to argparse.  TBH, I haven't kept up with python
> standard library for some time.

Well, 3.2 was released 11 years ago. :)

> 
>> Some parts of the code can probably be re-written to be more
>> "pythonic" as well, but I won't dive into that for now.  I didn't
>> review the code deep enough for that.
> 
> I have difficulty sometimes understanding what it means to be "Real
> Python (tm)" - I don't plan to change things too much.  I can certainly
> switch to using argparse, but unless you give something you want to
> change, I would not change anything.

I breifly looked through code and though I don't fully
understand what this piece supposed to do:

+        segment = hdrval.find(":")
+        if segment == -1:
+            segment = len(hdrval)
+        hdrver = int(hdrval[:segment], 0)
+        if len(hdrval[:segment]):
+            userfeatures = int(hdrval[:segment], 0)

but I have a strong feeling that this part can benefit
from use of hdrval.split(':').

I won't insist on that too much. :)

Best regards, Ilya Maximets.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net 2/2] selftests: add openvswitch selftest suite
  2022-10-20 16:33       ` Ilya Maximets
@ 2022-10-20 17:17         ` Ilya Maximets
  2022-10-25 12:15           ` Aaron Conole
  0 siblings, 1 reply; 9+ messages in thread
From: Ilya Maximets @ 2022-10-20 17:17 UTC (permalink / raw)
  To: Aaron Conole
  Cc: i.maximets, netdev, Pravin B Shelar, Jakub Kicinski,
	David S. Miller, Paolo Abeni, Eric Dumazet, Thomas Graf,
	Kevin Sprague, dev, Eelco Chaudron, Shuah Khan, linux-kernel,
	linux-kselftest

On 10/20/22 18:33, Ilya Maximets wrote:
> On 10/20/22 17:32, Aaron Conole wrote:
>> Hi Ilya,
>>
>> Ilya Maximets <i.maximets@ovn.org> writes:
>>
>>> On 10/19/22 20:30, Aaron Conole wrote:
>>>> Previous commit resolves a WARN splat that can be difficult to reproduce,
>>>> but with the ovs-dpctl.py utility, it can be trivial.  Introduce a test
>>>> case which creates a DP, and then downgrades the feature set.  This will
>>>> include a utility 'ovs-dpctl.py' that can be extended to do additional
>>>> work.
>>>>
>>>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>>>> Signed-off-by: Kevin Sprague <ksprague0711@gmail.com>
>>>> ---
>>>>  MAINTAINERS                                   |   1 +
>>>>  tools/testing/selftests/Makefile              |   1 +
>>>>  .../selftests/net/openvswitch/Makefile        |  13 +
>>>>  .../selftests/net/openvswitch/openvswitch.sh  | 216 +++++++++
>>>>  .../selftests/net/openvswitch/ovs-dpctl.py    | 411 ++++++++++++++++++
>>>>  5 files changed, 642 insertions(+)
>>>>  create mode 100644 tools/testing/selftests/net/openvswitch/Makefile
>>>>  create mode 100755 tools/testing/selftests/net/openvswitch/openvswitch.sh
>>>>  create mode 100644 tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index abbe88e1c50b..295a6b0fbe26 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -15434,6 +15434,7 @@ S:	Maintained
>>>>  W:	http://openvswitch.org
>>>>  F:	include/uapi/linux/openvswitch.h
>>>>  F:	net/openvswitch/
>>>> +F:	tools/testing/selftests/net/openvswitch/
>>>>  
>>>>  OPERATING PERFORMANCE POINTS (OPP)
>>>>  M:	Viresh Kumar <vireshk@kernel.org>
>>>
>>> ...
>>>
>>>> +exit ${exitcode}
>>>> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>>>> b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>>>> new file mode 100644
>>>> index 000000000000..791d76b7adcd
>>>> --- /dev/null
>>>> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>>>> @@ -0,0 +1,411 @@
>>>> +#!/usr/bin/env python3
>>>> +# SPDX-License-Identifier: GPL-2.0
>>>> +
>>>> +# Controls the openvswitch module.  Part of the kselftest suite, but
>>>> +# can be used for some diagnostic purpose as well.
>>>> +
>>>> +import logging
>>>> +import multiprocessing
>>>> +import socket
>>>> +import struct
>>>> +import sys
>>>> +
>>>> +try:
>>>> +    from libnl.attr import NLA_NESTED, NLA_STRING, NLA_U32, NLA_UNSPEC
>>>> +    from libnl.attr import nla_get_string, nla_get_u32
>>>> +    from libnl.attr import nla_put, nla_put_string, nla_put_u32
>>>> +    from libnl.attr import nla_policy
>>>> +
>>>> +    from libnl.error import errmsg
>>>> +
>>>> +    from libnl.genl.ctrl import genl_ctrl_resolve
>>>> +    from libnl.genl.genl import genl_connect, genlmsg_parse, genlmsg_put
>>>> +
>>>> +    from libnl.handlers import nl_cb_alloc, nl_cb_set
>>>> +    from libnl.handlers import NL_CB_CUSTOM, NL_CB_MSG_IN, NL_CB_VALID
>>>> +    from libnl.handlers import NL_OK, NL_STOP
>>>> +
>>>> +    from libnl.linux_private.netlink import NLM_F_ACK, NLM_F_DUMP
>>>> +    from libnl.linux_private.netlink import NLM_F_REQUEST, NLMSG_DONE
>>>> +
>>>> +    from libnl.msg import NL_AUTO_SEQ, nlmsg_alloc, nlmsg_hdr
>>>> +
>>>> +    from libnl.nl import NLMSG_ERROR, nl_recvmsgs_default, nl_send_auto
>>>> +    from libnl.socket_ import nl_socket_alloc, nl_socket_set_cb
>>>> +    from libnl.socket_ import nl_socket_get_local_port
>>>> +except ModuleNotFoundError:
>>>> +    print("Need to install the python libnl3 library.")
>>>
>>>
>>> Hey, Aaron and Kevin.  Selftests sounds like a very important and
>>> long overdue thing to add.  Thanks for working on this!
>>>
>>> I have some worries about the libnl3 library though.  It doesn't
>>> seem to be maintained well.  It it maintained by a single person,
>>> it it was at least 3 different single persons over the last 7
>>> years via forks.  It didn't get any significant development done
>>> since 2015 as well and no commits at all for a last 1.5 years.
>>> It is not packaged by any major distributions.
>>
>> :-/  On my fedora:
>>
>>   11:12:24 aconole@RHTPC1VM0NT ~$ dnf search python3-libnl3
>>   Last metadata expiration check: 1 day, 0:25:11 ago on Wed 19 Oct 2022 10:47:21 AM EDT.
>>   ===================== Name Exactly Matched: python3-libnl3 =====================
>>   python3-libnl3.x86_64 : libnl3 binding for Python 3
>>
>>
>> And I can use it:
>>
>>   11:18:39 aconole@RHTPC1VM0NT {(6a5c83bdd991...)} ~/git/linux/tools/testing/selftests/net/openvswitch$ sudo python3 ./ovs-dpctl.py show
>>   foop
>>     Lookups: Hit: 0 Missed: 0 Lost: 0
>>     Flows: 0
>>     Masks: Hit: 0 Total: 0
>>     Cache: Hit: 0
>>   Caches:
>>     Masks-cache: size: 256
>>       Port 0: foop (internal)
>>   11:18:43 aconole@RHTPC1VM0NT {(6a5c83bdd991...)} ~/git/linux/tools/testing/selftests/net/openvswitch$ rpm -qa | grep python3-libnl3
>>   python3-libnl3-3.5.0-6.fc34.x86_64
>>   11:19:01 aconole@RHTPC1VM0NT {(6a5c83bdd991...)} ~/git/linux/tools/testing/selftests/net/openvswitch$ 
>>
>> Was there some place you did not find it?
> 
> You're right, I missed that somehow.  But this is not an
> https://github.com/coolshou/libnl3 project. :)
> These are python bindings for the C libnl library:
> 
> $ dnf info python3-libnl3
> Available Packages
> Name         : python3-libnl3
> Version      : 3.7.0
> Release      : 1.fc36
> Architecture : x86_64
> Size         : 153 k
> Source       : libnl3-3.7.0-1.fc36.src.rpm
> Repository   : updates
> Summary      : libnl3 binding for Python 3
> URL          : http://www.infradead.org/~tgr/libnl/
> License      : LGPLv2
> Description  : Python 3 bindings for libnl3

Actually, I can't find an equivalent package for Ubuntu 22.04.
And since pip is not an option (pip install libnl3 is a different
package), there is no way to install it there beside building
from sources.

Am I still missing something?

> 
>>
>>> I'm talking about https://github.com/coolshou/libnl3 .  Please,
>>> correct me if that is not the right one.  There are too many
>>> libraies with the name libnl out there...  That is also not a great
>>> sign.
>>
>> Yes, this is the project.
> 
> Doensn't look like it...
> 
>> We did look at some of the ones you
>> mentioned, but didn't find much.
>>
>> It is a sparse landscape of projects that provide netlink support in
>> python.
>>
>>> The C library libnl (https://github.com/thom311/libnl) seems to
>>> be well maintained in general.  It has experimental python
>>> bindings which are not really supported much.  Python bindings
>>> received only 2 actual code-changing commits in the last 7 years.
>>> Both of them are just python 2/3 compatibility changes.
>>> Maybe that is not that big of a deal since it's not really a
>>> real python library, but a wrapper on top of a main C library.
>>> However, I didn't find these python bindings to be packaged in
>>> distributions.  And they seem to be not available in pip as well.
>>> So, building them is kind of a pain.
>>
>> Well, the python libnl3 should be installable via pip3.  Ex:
>>
>>   11:27:15 aconole@RHTPC1VM0NT ~$ pip3 install libnl3
>>   Defaulting to user installation because normal site-packages is not writeable
>>   Collecting libnl3
>>     Using cached libnl3-0.3.0-py3-none-any.whl (89 kB)
>>   Installing collected packages: libnl3
>>   Successfully installed libnl3-0.3.0
> 
> And this is https://pypi.org/project/libnl3/, which is the
> https://github.com/coolshou/libnl3 project.  So, by installing
> libnl3 via pip and installing python3-libnl3 from the fedora
> you're getting two completely different libraries.
> 
> So, which one users should use?
> 
> I can't find python bindings for the C libnl (which is the
> python3-libnl3 package) in pypi, so it can't be installed
> with pip.
> 
> 
>>
>> So I guess that is worth something.
>>
>> At least on Fedora it is installable from distribution as well.
>>
>>> There is another option which is pyroute2.  It positions itself
>>> primarily as a netlink library and it does include an
>>> pyroute2.netlink module indeed:
>>>   https://github.com/svinota/pyroute2/tree/master/pyroute2/netlink
>>> See the __init__.py for usage examples.
>>>
>>> This one looks to me like the most trustworthy.  It is actively
>>> used by everyone in the python networking world, e.g. by OpenStack.
>>> And it is actively developed and maintained unlike other
>>> netlink-related python projects.  It is also packaged in most of the
>>> main distributions, so it's easy to install and use.  Many people
>>> already have it installed for other purposes.
>>>
>>> TBH, I didn't compare the functionality, but I'd expect that most
>>> of the things we need are implemented.
>>>
>>> What do you think?
>>
>> We can certainly look at switching, but having a quick glance, it seems
>> pyroute2 expects to provide the genl commands as well, so they would
>> want us to create an ovs module in pyroute2 that includes all of the ovs
>> family support.  Of course, we can always do this just in our module,
>> but I think it isn't the way pyroute2 project wants to be structured.
>> More like a library that provides all the command functionality.
> 
> What I was thinking is to import pyroute2.netlink and the
> pyroute2.netlink.generic  and go from there.  But I didn't
> look too deep on how to actually implement the functionality.
> 
> The python bindings for the C libnl (python3-libnl3) sounds
> like a fine option since they are actually packaged in
> distributions (missed that in my initial reply).  However,
> the fact that you can not install them via pip and actually
> you will install something but completely different is kind
> of weird.  This has to be at least better documented, so
> users will know what to install and they will not try to use
> pip for that.
> 
>>
>>> On the other note, I'm not a real python developer, but the code
>>> looks more like a C than a python even for me.  Firstly, I'd say
>>> that it would be great to maintain some coding style, e.g. by
>>> checking with flake8 and/or black.  See some issues/suggestions
>>> provided by these tools below.
>>
>> Agreed.  BTW, on the rhel8 system I developed on:
>>
>>   [root@wsfd-netdev60 openvswitch]# flake8 ./ovs-dpctl.py 
>>   [root@wsfd-netdev60 openvswitch]# 
>>
>> So, I guess it is probably that I should have used a different system to
>> do the flake8 checks.
> 
> Maybe the python version is different...  I was running on f36
> with python 3.10.  Also, the list of defaults might be different.
> flake8 doesn't use default ignore list if one is explicitly provided.
> 
>>
>>> Secondly, we shouldd at least use argparse for argument parsing.
>>> It's part of the standard library since python 3.2, so doens't
>>> require any special dependencies to be installed.
>>
>> Okay - I can switch to argparse.  TBH, I haven't kept up with python
>> standard library for some time.
> 
> Well, 3.2 was released 11 years ago. :)
> 
>>
>>> Some parts of the code can probably be re-written to be more
>>> "pythonic" as well, but I won't dive into that for now.  I didn't
>>> review the code deep enough for that.
>>
>> I have difficulty sometimes understanding what it means to be "Real
>> Python (tm)" - I don't plan to change things too much.  I can certainly
>> switch to using argparse, but unless you give something you want to
>> change, I would not change anything.
> 
> I breifly looked through code and though I don't fully
> understand what this piece supposed to do:
> 
> +        segment = hdrval.find(":")
> +        if segment == -1:
> +            segment = len(hdrval)
> +        hdrver = int(hdrval[:segment], 0)
> +        if len(hdrval[:segment]):
> +            userfeatures = int(hdrval[:segment], 0)
> 
> but I have a strong feeling that this part can benefit
> from use of hdrval.split(':').
> 
> I won't insist on that too much. :)
> 
> Best regards, Ilya Maximets.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net 1/2] openvswitch: switch from WARN to pr_warn
  2022-10-19 18:30 ` [PATCH net 1/2] openvswitch: switch from WARN to pr_warn Aaron Conole
@ 2022-10-20 18:40   ` Ilya Maximets
  0 siblings, 0 replies; 9+ messages in thread
From: Ilya Maximets @ 2022-10-20 18:40 UTC (permalink / raw)
  To: Aaron Conole, netdev
  Cc: i.maximets, Pravin B Shelar, Jakub Kicinski, David S. Miller,
	Paolo Abeni, Eric Dumazet, Thomas Graf, Kevin Sprague, dev,
	Eelco Chaudron, Shuah Khan, linux-kernel, linux-kselftest

On 10/19/22 20:30, Aaron Conole wrote:
> As noted by Paolo Abeni, pr_warn doesn't generate any splat and can still
> preserve the warning to the user that feature downgrade occurred.  We
> likely cannot introduce other kinds of checks / enforcement here because
> syzbot can generate different genl versions to the datapath.
> 
> Reported-by: syzbot+31cde0bef4bbf8ba2d86@syzkaller.appspotmail.com
> Fixes: 44da5ae5fbea ("openvswitch: Drop user features if old user space attempted to create datapath")
> Cc: Thomas Graf <tgraf@suug.ch>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  net/openvswitch/datapath.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index c8a9075ddd0a..155263e73512 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -1616,7 +1616,8 @@ static void ovs_dp_reset_user_features(struct sk_buff *skb,
>  	if (IS_ERR(dp))
>  		return;
>  
> -	WARN(dp->user_features, "Dropping previously announced user features\n");
> +	pr_warn("%s: Dropping previously announced user features\n",
> +		ovs_dp_name(dp));
>  	dp->user_features = 0;
>  }
>  

Works fine.  Thanks!

Acked-by: Ilya Maximets <i.maximets@ovn.org>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net 2/2] selftests: add openvswitch selftest suite
  2022-10-20 17:17         ` Ilya Maximets
@ 2022-10-25 12:15           ` Aaron Conole
  0 siblings, 0 replies; 9+ messages in thread
From: Aaron Conole @ 2022-10-25 12:15 UTC (permalink / raw)
  To: Ilya Maximets
  Cc: netdev, Pravin B Shelar, Jakub Kicinski, David S. Miller,
	Paolo Abeni, Eric Dumazet, Thomas Graf, Kevin Sprague, dev,
	Eelco Chaudron, Shuah Khan, linux-kernel, linux-kselftest

Ilya Maximets <i.maximets@ovn.org> writes:

> On 10/20/22 18:33, Ilya Maximets wrote:
>> On 10/20/22 17:32, Aaron Conole wrote:
>>> Hi Ilya,
>>>
>>> Ilya Maximets <i.maximets@ovn.org> writes:
>>>
>>>> On 10/19/22 20:30, Aaron Conole wrote:
>>>>> Previous commit resolves a WARN splat that can be difficult to reproduce,
>>>>> but with the ovs-dpctl.py utility, it can be trivial.  Introduce a test
>>>>> case which creates a DP, and then downgrades the feature set.  This will
>>>>> include a utility 'ovs-dpctl.py' that can be extended to do additional
>>>>> work.
>>>>>
>>>>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>>>>> Signed-off-by: Kevin Sprague <ksprague0711@gmail.com>
>>>>> ---
>>>>>  MAINTAINERS                                   |   1 +
>>>>>  tools/testing/selftests/Makefile              |   1 +
>>>>>  .../selftests/net/openvswitch/Makefile        |  13 +
>>>>>  .../selftests/net/openvswitch/openvswitch.sh  | 216 +++++++++
>>>>>  .../selftests/net/openvswitch/ovs-dpctl.py    | 411 ++++++++++++++++++
>>>>>  5 files changed, 642 insertions(+)
>>>>>  create mode 100644 tools/testing/selftests/net/openvswitch/Makefile
>>>>>  create mode 100755 tools/testing/selftests/net/openvswitch/openvswitch.sh
>>>>>  create mode 100644 tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>>>>>
>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>> index abbe88e1c50b..295a6b0fbe26 100644
>>>>> --- a/MAINTAINERS
>>>>> +++ b/MAINTAINERS
>>>>> @@ -15434,6 +15434,7 @@ S:	Maintained
>>>>>  W:	http://openvswitch.org
>>>>>  F:	include/uapi/linux/openvswitch.h
>>>>>  F:	net/openvswitch/
>>>>> +F:	tools/testing/selftests/net/openvswitch/
>>>>>  
>>>>>  OPERATING PERFORMANCE POINTS (OPP)
>>>>>  M:	Viresh Kumar <vireshk@kernel.org>
>>>>
>>>> ...
>>>>
>>>>> +exit ${exitcode}
>>>>> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>>>>> b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>>>>> new file mode 100644
>>>>> index 000000000000..791d76b7adcd
>>>>> --- /dev/null
>>>>> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>>>>> @@ -0,0 +1,411 @@
>>>>> +#!/usr/bin/env python3
>>>>> +# SPDX-License-Identifier: GPL-2.0
>>>>> +
>>>>> +# Controls the openvswitch module.  Part of the kselftest suite, but
>>>>> +# can be used for some diagnostic purpose as well.
>>>>> +
>>>>> +import logging
>>>>> +import multiprocessing
>>>>> +import socket
>>>>> +import struct
>>>>> +import sys
>>>>> +
>>>>> +try:
>>>>> +    from libnl.attr import NLA_NESTED, NLA_STRING, NLA_U32, NLA_UNSPEC
>>>>> +    from libnl.attr import nla_get_string, nla_get_u32
>>>>> +    from libnl.attr import nla_put, nla_put_string, nla_put_u32
>>>>> +    from libnl.attr import nla_policy
>>>>> +
>>>>> +    from libnl.error import errmsg
>>>>> +
>>>>> +    from libnl.genl.ctrl import genl_ctrl_resolve
>>>>> +    from libnl.genl.genl import genl_connect, genlmsg_parse, genlmsg_put
>>>>> +
>>>>> +    from libnl.handlers import nl_cb_alloc, nl_cb_set
>>>>> +    from libnl.handlers import NL_CB_CUSTOM, NL_CB_MSG_IN, NL_CB_VALID
>>>>> +    from libnl.handlers import NL_OK, NL_STOP
>>>>> +
>>>>> +    from libnl.linux_private.netlink import NLM_F_ACK, NLM_F_DUMP
>>>>> +    from libnl.linux_private.netlink import NLM_F_REQUEST, NLMSG_DONE
>>>>> +
>>>>> +    from libnl.msg import NL_AUTO_SEQ, nlmsg_alloc, nlmsg_hdr
>>>>> +
>>>>> +    from libnl.nl import NLMSG_ERROR, nl_recvmsgs_default, nl_send_auto
>>>>> +    from libnl.socket_ import nl_socket_alloc, nl_socket_set_cb
>>>>> +    from libnl.socket_ import nl_socket_get_local_port
>>>>> +except ModuleNotFoundError:
>>>>> +    print("Need to install the python libnl3 library.")
>>>>
>>>>
>>>> Hey, Aaron and Kevin.  Selftests sounds like a very important and
>>>> long overdue thing to add.  Thanks for working on this!
>>>>
>>>> I have some worries about the libnl3 library though.  It doesn't
>>>> seem to be maintained well.  It it maintained by a single person,
>>>> it it was at least 3 different single persons over the last 7
>>>> years via forks.  It didn't get any significant development done
>>>> since 2015 as well and no commits at all for a last 1.5 years.
>>>> It is not packaged by any major distributions.
>>>
>>> :-/  On my fedora:
>>>
>>>   11:12:24 aconole@RHTPC1VM0NT ~$ dnf search python3-libnl3
>>>   Last metadata expiration check: 1 day, 0:25:11 ago on Wed 19 Oct 2022 10:47:21 AM EDT.
>>>   ===================== Name Exactly Matched: python3-libnl3 =====================
>>>   python3-libnl3.x86_64 : libnl3 binding for Python 3
>>>
>>>
>>> And I can use it:
>>>
>>>   11:18:39 aconole@RHTPC1VM0NT {(6a5c83bdd991...)} ~/git/linux/tools/testing/selftests/net/openvswitch$ sudo python3 ./ovs-dpctl.py show
>>>   foop
>>>     Lookups: Hit: 0 Missed: 0 Lost: 0
>>>     Flows: 0
>>>     Masks: Hit: 0 Total: 0
>>>     Cache: Hit: 0
>>>   Caches:
>>>     Masks-cache: size: 256
>>>       Port 0: foop (internal)
>>>   11:18:43 aconole@RHTPC1VM0NT {(6a5c83bdd991...)} ~/git/linux/tools/testing/selftests/net/openvswitch$ rpm -qa | grep python3-libnl3
>>>   python3-libnl3-3.5.0-6.fc34.x86_64
>>>   11:19:01 aconole@RHTPC1VM0NT {(6a5c83bdd991...)} ~/git/linux/tools/testing/selftests/net/openvswitch$ 
>>>
>>> Was there some place you did not find it?
>> 
>> You're right, I missed that somehow.  But this is not an
>> https://github.com/coolshou/libnl3 project. :)
>> These are python bindings for the C libnl library:
>> 
>> $ dnf info python3-libnl3
>> Available Packages
>> Name         : python3-libnl3
>> Version      : 3.7.0
>> Release      : 1.fc36
>> Architecture : x86_64
>> Size         : 153 k
>> Source       : libnl3-3.7.0-1.fc36.src.rpm
>> Repository   : updates
>> Summary      : libnl3 binding for Python 3
>> URL          : http://www.infradead.org/~tgr/libnl/
>> License      : LGPLv2
>> Description  : Python 3 bindings for libnl3
>
> Actually, I can't find an equivalent package for Ubuntu 22.04.
> And since pip is not an option (pip install libnl3 is a different
> package), there is no way to install it there beside building
> from sources.
>
> Am I still missing something?

Well, I have switched the latest version to using pyroute2 - hopefully
this will be acceptable :)

>> 
>>>
>>>> I'm talking about https://github.com/coolshou/libnl3 .  Please,
>>>> correct me if that is not the right one.  There are too many
>>>> libraies with the name libnl out there...  That is also not a great
>>>> sign.
>>>
>>> Yes, this is the project.
>> 
>> Doensn't look like it...
>> 
>>> We did look at some of the ones you
>>> mentioned, but didn't find much.
>>>
>>> It is a sparse landscape of projects that provide netlink support in
>>> python.
>>>
>>>> The C library libnl (https://github.com/thom311/libnl) seems to
>>>> be well maintained in general.  It has experimental python
>>>> bindings which are not really supported much.  Python bindings
>>>> received only 2 actual code-changing commits in the last 7 years.
>>>> Both of them are just python 2/3 compatibility changes.
>>>> Maybe that is not that big of a deal since it's not really a
>>>> real python library, but a wrapper on top of a main C library.
>>>> However, I didn't find these python bindings to be packaged in
>>>> distributions.  And they seem to be not available in pip as well.
>>>> So, building them is kind of a pain.
>>>
>>> Well, the python libnl3 should be installable via pip3.  Ex:
>>>
>>>   11:27:15 aconole@RHTPC1VM0NT ~$ pip3 install libnl3
>>>   Defaulting to user installation because normal site-packages is not writeable
>>>   Collecting libnl3
>>>     Using cached libnl3-0.3.0-py3-none-any.whl (89 kB)
>>>   Installing collected packages: libnl3
>>>   Successfully installed libnl3-0.3.0
>> 
>> And this is https://pypi.org/project/libnl3/, which is the
>> https://github.com/coolshou/libnl3 project.  So, by installing
>> libnl3 via pip and installing python3-libnl3 from the fedora
>> you're getting two completely different libraries.
>> 
>> So, which one users should use?
>> 
>> I can't find python bindings for the C libnl (which is the
>> python3-libnl3 package) in pypi, so it can't be installed
>> with pip.
>> 
>> 
>>>
>>> So I guess that is worth something.
>>>
>>> At least on Fedora it is installable from distribution as well.
>>>
>>>> There is another option which is pyroute2.  It positions itself
>>>> primarily as a netlink library and it does include an
>>>> pyroute2.netlink module indeed:
>>>>   https://github.com/svinota/pyroute2/tree/master/pyroute2/netlink
>>>> See the __init__.py for usage examples.
>>>>
>>>> This one looks to me like the most trustworthy.  It is actively
>>>> used by everyone in the python networking world, e.g. by OpenStack.
>>>> And it is actively developed and maintained unlike other
>>>> netlink-related python projects.  It is also packaged in most of the
>>>> main distributions, so it's easy to install and use.  Many people
>>>> already have it installed for other purposes.
>>>>
>>>> TBH, I didn't compare the functionality, but I'd expect that most
>>>> of the things we need are implemented.
>>>>
>>>> What do you think?
>>>
>>> We can certainly look at switching, but having a quick glance, it seems
>>> pyroute2 expects to provide the genl commands as well, so they would
>>> want us to create an ovs module in pyroute2 that includes all of the ovs
>>> family support.  Of course, we can always do this just in our module,
>>> but I think it isn't the way pyroute2 project wants to be structured.
>>> More like a library that provides all the command functionality.
>> 
>> What I was thinking is to import pyroute2.netlink and the
>> pyroute2.netlink.generic  and go from there.  But I didn't
>> look too deep on how to actually implement the functionality.
>> 
>> The python bindings for the C libnl (python3-libnl3) sounds
>> like a fine option since they are actually packaged in
>> distributions (missed that in my initial reply).  However,
>> the fact that you can not install them via pip and actually
>> you will install something but completely different is kind
>> of weird.  This has to be at least better documented, so
>> users will know what to install and they will not try to use
>> pip for that.
>> 
>>>
>>>> On the other note, I'm not a real python developer, but the code
>>>> looks more like a C than a python even for me.  Firstly, I'd say
>>>> that it would be great to maintain some coding style, e.g. by
>>>> checking with flake8 and/or black.  See some issues/suggestions
>>>> provided by these tools below.
>>>
>>> Agreed.  BTW, on the rhel8 system I developed on:
>>>
>>>   [root@wsfd-netdev60 openvswitch]# flake8 ./ovs-dpctl.py 
>>>   [root@wsfd-netdev60 openvswitch]# 
>>>
>>> So, I guess it is probably that I should have used a different system to
>>> do the flake8 checks.
>> 
>> Maybe the python version is different...  I was running on f36
>> with python 3.10.  Also, the list of defaults might be different.
>> flake8 doesn't use default ignore list if one is explicitly provided.
>> 
>>>
>>>> Secondly, we shouldd at least use argparse for argument parsing.
>>>> It's part of the standard library since python 3.2, so doens't
>>>> require any special dependencies to be installed.
>>>
>>> Okay - I can switch to argparse.  TBH, I haven't kept up with python
>>> standard library for some time.
>> 
>> Well, 3.2 was released 11 years ago. :)
>> 
>>>
>>>> Some parts of the code can probably be re-written to be more
>>>> "pythonic" as well, but I won't dive into that for now.  I didn't
>>>> review the code deep enough for that.
>>>
>>> I have difficulty sometimes understanding what it means to be "Real
>>> Python (tm)" - I don't plan to change things too much.  I can certainly
>>> switch to using argparse, but unless you give something you want to
>>> change, I would not change anything.
>> 
>> I breifly looked through code and though I don't fully
>> understand what this piece supposed to do:
>> 
>> +        segment = hdrval.find(":")
>> +        if segment == -1:
>> +            segment = len(hdrval)
>> +        hdrver = int(hdrval[:segment], 0)
>> +        if len(hdrval[:segment]):
>> +            userfeatures = int(hdrval[:segment], 0)
>> 
>> but I have a strong feeling that this part can benefit
>> from use of hdrval.split(':').
>> 
>> I won't insist on that too much. :)
>> 
>> Best regards, Ilya Maximets.


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-10-25 12:15 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-19 18:30 [PATCH net 0/2] openvswitch: syzbot splat fix and introduce selftest Aaron Conole
2022-10-19 18:30 ` [PATCH net 1/2] openvswitch: switch from WARN to pr_warn Aaron Conole
2022-10-20 18:40   ` Ilya Maximets
2022-10-19 18:30 ` [PATCH net 2/2] selftests: add openvswitch selftest suite Aaron Conole
2022-10-20 13:21   ` Ilya Maximets
2022-10-20 15:32     ` Aaron Conole
2022-10-20 16:33       ` Ilya Maximets
2022-10-20 17:17         ` Ilya Maximets
2022-10-25 12:15           ` Aaron Conole

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.