Re: [PATCH net-next 04/10] tools: ynl: convert rt-link sample to selftest

[Donald Hunter <donald.hunter@gmail.com>]

Jakub Kicinski <kuba@kernel.org> writes:

> Convert rt-link.c to use kselftest_harness.h with FIXTURE/TEST_F.
> Move rt-link from BINS to TEST_GEN_PROGS.
>
> Output:
>
>   TAP version 13
>   1..3
>   # Starting 3 tests from 1 test cases.
>   #  RUN           rt_link.dump ...
>   #   1:          lo: mtu 65536
>   #   2:          sit0: mtu  1480  kind sit
>   #            OK  rt_link.dump
>   ok 1 rt_link.dump
>   #  RUN           rt_link.netkit ...
>   #   4:          nk1: mtu  1500  kind netkit    primary 1  policy blackhole
>   #            OK  rt_link.netkit
>   ok 2 rt_link.netkit
>   #  RUN           rt_link.netkit_err_msg ...
>   #            OK  rt_link.netkit_err_msg
>   ok 3 rt_link.netkit_err_msg
>   # PASSED: 3 / 3 tests passed.
>   # Totals: pass:3 fail:0 xfail:0 xpass:0 skip:0 error:0
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  tools/net/ynl/tests/Makefile  |   2 +-
>  tools/net/ynl/tests/rt-link.c | 192 +++++++++++++++++++---------------
>  tools/net/ynl/tests/config    |   1 +
>  3 files changed, 108 insertions(+), 87 deletions(-)
>
> diff --git a/tools/net/ynl/tests/Makefile b/tools/net/ynl/tests/Makefile
> index df9d37c8b2a4..08d1146d91ce 100644
> --- a/tools/net/ynl/tests/Makefile
> +++ b/tools/net/ynl/tests/Makefile
> @@ -21,13 +21,13 @@ TEST_PROGS := \
>  TEST_GEN_PROGS := \
>  	netdev \
>  	ovs \
> +	rt-link \
>  # end of TEST_GEN_PROGS
>  
>  BINS := \
>  	devlink \
>  	ethtool \
>  	rt-addr \
> -	rt-link \
>  	rt-route \
>  	tc \
>  	tc-filter-add \
> diff --git a/tools/net/ynl/tests/rt-link.c b/tools/net/ynl/tests/rt-link.c
> index acdd4b4a0f74..a55f7523dbe8 100644
> --- a/tools/net/ynl/tests/rt-link.c
> +++ b/tools/net/ynl/tests/rt-link.c
> @@ -7,16 +7,21 @@
>  #include <arpa/inet.h>
>  #include <net/if.h>
>  
> +#include <kselftest_harness.h>
> +
>  #include "rt-link-user.h"
>  
> -static void rt_link_print(struct rt_link_getlink_rsp *r)
> +static void rt_link_print(struct __test_metadata *_metadata,
> +			  struct rt_link_getlink_rsp *r)
>  {
>  	unsigned int i;
>  
> -	printf("%3d: ", r->_hdr.ifi_index);
> +	EXPECT_TRUE((bool)r->_hdr.ifi_index);
> +	ksft_print_msg("%3d: ", r->_hdr.ifi_index);
>  
> +	EXPECT_TRUE((bool)r->_len.ifname);
>  	if (r->_len.ifname)
> -		printf("%16s: ", r->ifname);
> +		printf("%6s: ", r->ifname);
>  
>  	if (r->_present.mtu)
>  		printf("mtu %5d  ", r->mtu);
> @@ -50,7 +55,7 @@ static void rt_link_print(struct rt_link_getlink_rsp *r)
>  	printf("\n");
>  }
>  
> -static int rt_link_create_netkit(struct ynl_sock *ys)
> +static int netkit_create(struct ynl_sock *ys)
>  {
>  	struct rt_link_getlink_ntf *ntf_gl;
>  	struct rt_link_newlink_req *req;
> @@ -58,49 +63,24 @@ static int rt_link_create_netkit(struct ynl_sock *ys)
>  	int ret;
>  
>  	req = rt_link_newlink_req_alloc();
> -	if (!req) {
> -		fprintf(stderr, "Can't alloc req\n");
> +	if (!req)
>  		return -1;
> -	}
>  
> -	/* rtnetlink doesn't provide info about the created object.
> -	 * It expects us to set the ECHO flag and the dig the info out
> -	 * of the notifications...
> -	 */
>  	rt_link_newlink_req_set_nlflags(req, NLM_F_CREATE | NLM_F_ECHO);
> -
>  	rt_link_newlink_req_set_linkinfo_kind(req, "netkit");
> -
> -	/* Test error messages */
> -	rt_link_newlink_req_set_linkinfo_data_netkit_policy(req, 10);
> -	ret = rt_link_newlink(ys, req);
> -	if (ret) {
> -		printf("Testing error message for policy being bad:\n\t%s\n", ys->err.msg);
> -	} else {
> -		fprintf(stderr,	"Warning: unexpected success creating netkit with bad attrs\n");
> -		goto created;
> -	}
> -
>  	rt_link_newlink_req_set_linkinfo_data_netkit_policy(req, NETKIT_DROP);
>  
>  	ret = rt_link_newlink(ys, req);
> -created:
>  	rt_link_newlink_req_free(req);
> -	if (ret) {
> -		fprintf(stderr, "YNL: %s\n", ys->err.msg);
> +	if (ret)
>  		return -1;
> -	}
>  
> -	if (!ynl_has_ntf(ys)) {
> -		fprintf(stderr,
> -			"Warning: interface created but received no notification, won't delete the interface\n");
> +	if (!ynl_has_ntf(ys))
>  		return 0;
> -	}
>  
>  	ntf = ynl_ntf_dequeue(ys);
> -	if (ntf->cmd !=	RTM_NEWLINK) {
> -		fprintf(stderr,
> -			"Warning: unexpected notification type, won't delete the interface\n");
> +	if (!ntf || ntf->cmd != RTM_NEWLINK) {
> +		ynl_ntf_free(ntf);
>  		return 0;
>  	}
>  	ntf_gl = (void *)ntf;
> @@ -110,75 +90,115 @@ static int rt_link_create_netkit(struct ynl_sock *ys)
>  	return ret;
>  }
>  
> -static void rt_link_del(struct ynl_sock *ys, int ifindex)
> +static void netkit_delete(struct __test_metadata *_metadata,
> +			  struct ynl_sock *ys, int ifindex)
>  {
>  	struct rt_link_dellink_req *req;
>  
>  	req = rt_link_dellink_req_alloc();
> -	if (!req) {
> -		fprintf(stderr, "Can't alloc req\n");
> +	if (!req)
>  		return;
> -	}
>  
>  	req->_hdr.ifi_index = ifindex;
> -	if (rt_link_dellink(ys, req))
> -		fprintf(stderr, "YNL: %s\n", ys->err.msg);
> -	else
> -		fprintf(stderr,
> -			"Trying to delete a Netkit interface (ifindex %d)\n",
> -			ifindex);
> -
> +	EXPECT_EQ(0, rt_link_dellink(ys, req));
>  	rt_link_dellink_req_free(req);
>  }
>  
> -int main(int argc, char **argv)
> +FIXTURE(rt_link)
> +{
> +	struct ynl_sock *ys;
> +};
> +
> +FIXTURE_SETUP(rt_link)
> +{
> +	struct ynl_error yerr;
> +
> +	self->ys = ynl_sock_create(&ynl_rt_link_family, &yerr);
> +	ASSERT_NE(NULL, self->ys) {
> +		TH_LOG("failed to create rt-link socket: %s", yerr.msg);
> +	}
> +}
> +
> +FIXTURE_TEARDOWN(rt_link)
> +{
> +	ynl_sock_destroy(self->ys);
> +}
> +
> +TEST_F(rt_link, dump)
>  {
>  	struct rt_link_getlink_req_dump *req;
>  	struct rt_link_getlink_list *rsp;
> -	struct ynl_error yerr;
> -	struct ynl_sock *ys;
> -	int created = 0;
> -
> -	ys = ynl_sock_create(&ynl_rt_link_family, &yerr);
> -	if (!ys) {
> -		fprintf(stderr, "YNL: %s\n", yerr.msg);
> -		return 1;
> -	}
> -
> -	if (argc > 1) {
> -		fprintf(stderr, "Trying to create a Netkit interface\n");
> -		created = rt_link_create_netkit(ys);
> -		if (created < 0)
> -			goto err_destroy;
> -	}
>  
>  	req = rt_link_getlink_req_dump_alloc();
> -	if (!req)
> -		goto err_del_ifc;
> -
> -	rsp = rt_link_getlink_dump(ys, req);
> +	rsp = rt_link_getlink_dump(self->ys, req);
>  	rt_link_getlink_req_dump_free(req);
> -	if (!rsp)
> -		goto err_close;
> +	ASSERT_NE(NULL, rsp) {
> +		TH_LOG("dump failed: %s", self->ys->err.msg);
> +	}
> +	ASSERT_FALSE(ynl_dump_empty(rsp));
>  
> -	if (ynl_dump_empty(rsp))
> -		fprintf(stderr, "Error: no links reported\n");
>  	ynl_dump_foreach(rsp, link)
> -		rt_link_print(link);
> +		rt_link_print(_metadata, link);
> +
>  	rt_link_getlink_list_free(rsp);
> -
> -	if (created)
> -		rt_link_del(ys, created);
> -
> -	ynl_sock_destroy(ys);
> -	return 0;
> -
> -err_close:
> -	fprintf(stderr, "YNL: %s\n", ys->err.msg);
> -err_del_ifc:
> -	if (created)
> -		rt_link_del(ys, created);
> -err_destroy:
> -	ynl_sock_destroy(ys);
> -	return 2;
>  }
> +
> +TEST_F(rt_link, netkit)
> +{
> +	struct rt_link_getlink_req_dump *dreq;
> +	struct rt_link_getlink_list *rsp;
> +	bool found = false;
> +	int netkit_ifindex;
> +
> +	/* Create netkit with valid policy */
> +	netkit_ifindex = netkit_create(self->ys);
> +	ASSERT_GT(netkit_ifindex, 0)
> +		TH_LOG("failed to create netkit: %s", self->ys->err.msg);
> +
> +	/* Verify it appears in a dump */
> +	dreq = rt_link_getlink_req_dump_alloc();

I see there's error handling for *_alloc() in helper functions but
not in tests. Is that intentional?

> +	rsp = rt_link_getlink_dump(self->ys, dreq);
> +	rt_link_getlink_req_dump_free(dreq);
> +	ASSERT_NE(NULL, rsp) {
> +		TH_LOG("dump failed: %s", self->ys->err.msg);
> +	}
> +
> +	ynl_dump_foreach(rsp, link) {
> +		if (link->_hdr.ifi_index == netkit_ifindex) {
> +			rt_link_print(_metadata, link);
> +			found = true;
> +		}
> +	}
> +	rt_link_getlink_list_free(rsp);
> +	EXPECT_TRUE(found);
> +
> +	netkit_delete(_metadata, self->ys, netkit_ifindex);
> +}
> +
> +TEST_F(rt_link, netkit_err_msg)
> +{
> +	struct rt_link_newlink_req *req;
> +	int ret;
> +
> +	/* Test creating netkit with bad policy - should fail */
> +	req = rt_link_newlink_req_alloc();
> +	rt_link_newlink_req_set_nlflags(req, NLM_F_CREATE);
> +	rt_link_newlink_req_set_linkinfo_kind(req, "netkit");
> +	rt_link_newlink_req_set_linkinfo_data_netkit_policy(req, 10);
> +
> +	ret = rt_link_newlink(self->ys, req);
> +	rt_link_newlink_req_free(req);
> +	EXPECT_NE(0, ret) {
> +		TH_LOG("creating netkit with bad policy should fail");
> +	}
> +
> +	/* Expect:
> +	 * Kernel error: 'Provided default xmit policy not supported' (bad attribute: .linkinfo.data(netkit).policy)
> +	 */
> +	EXPECT_NE(NULL, strstr(self->ys->err.msg, "bad attribute: .linkinfo.data(netkit).policy")) {
> +		TH_LOG("expected extack msg not found: %s",
> +		       self->ys->err.msg);
> +	}
> +}
> +
> +TEST_HARNESS_MAIN
> diff --git a/tools/net/ynl/tests/config b/tools/net/ynl/tests/config
> index 357b34611da4..b4c58d86a6c2 100644
> --- a/tools/net/ynl/tests/config
> +++ b/tools/net/ynl/tests/config
> @@ -3,5 +3,6 @@ CONFIG_INET_DIAG=y
>  CONFIG_IPV6=y
>  CONFIG_NET_NS=y
>  CONFIG_NETDEVSIM=m
> +CONFIG_NETKIT=y
>  CONFIG_OPENVSWITCH=m
>  CONFIG_VETH=m