[PATCHv3 iproute2-next] rtnetlink: add new function rtnl_echo_talk()

[PATCHv3 iproute2-next] rtnetlink: add new function rtnl_echo_talk()

[Hangbin Liu]

Add a new function rtnl_echo_talk() that could be used when the
sub-component supports NLM_F_ECHO flag. With this function we can
remove the redundant code added by commit b264b4c6568c7 ("ip: add
NLM_F_ECHO support").

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
v3: no need to use 'else' if we can return directly
v2: only handle echo_request code in helper rtnl_echo_talk()
---
 include/libnetlink.h |  3 +++
 ip/ipaddress.c       | 23 ++---------------------
 ip/iplink.c          | 19 +++----------------
 ip/ipnexthop.c       | 23 ++---------------------
 ip/iproute.c         | 23 ++---------------------
 ip/iprule.c          | 23 ++---------------------
 lib/libnetlink.c     | 22 ++++++++++++++++++++++
 7 files changed, 36 insertions(+), 100 deletions(-)

diff --git a/include/libnetlink.h b/include/libnetlink.h
index a7b0f352..1b8d29bd 100644
--- a/include/libnetlink.h
+++ b/include/libnetlink.h
@@ -171,6 +171,9 @@ int rtnl_dump_filter_errhndlr_nc(struct rtnl_handle *rth,
 #define rtnl_dump_filter_errhndlr(rth, filter, farg, errhndlr, earg) \
 	rtnl_dump_filter_errhndlr_nc(rth, filter, farg, errhndlr, earg, 0)
 
+int rtnl_echo_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n, int json,
+		   int (*print_info)(struct nlmsghdr *n, void *arg))
+	__attribute__((warn_unused_result));
 int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 	      struct nlmsghdr **answer)
 	__attribute__((warn_unused_result));
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 986cfbc3..456545bb 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -2422,11 +2422,6 @@ static int ipaddr_modify(int cmd, int flags, int argc, char **argv)
 	__u32 preferred_lft = INFINITY_LIFE_TIME;
 	__u32 valid_lft = INFINITY_LIFE_TIME;
 	unsigned int ifa_flags = 0;
-	struct nlmsghdr *answer;
-	int ret;
-
-	if (echo_request)
-		req.n.nlmsg_flags |= NLM_F_ECHO | NLM_F_ACK;
 
 	while (argc > 0) {
 		if (strcmp(*argv, "peer") == 0 ||
@@ -2609,23 +2604,9 @@ static int ipaddr_modify(int cmd, int flags, int argc, char **argv)
 	}
 
 	if (echo_request)
-		ret = rtnl_talk(&rth, &req.n, &answer);
-	else
-		ret = rtnl_talk(&rth, &req.n, NULL);
-
-	if (ret < 0)
-		return -2;
+		return rtnl_echo_talk(&rth, &req.n, json, print_addrinfo);
 
-	if (echo_request) {
-		new_json_obj(json);
-		open_json_object(NULL);
-		print_addrinfo(answer, stdout);
-		close_json_object();
-		delete_json_obj();
-		free(answer);
-	}
-
-	return 0;
+	return rtnl_talk(&rth, &req.n, NULL);
 }
 
 int do_ipaddr(int argc, char **argv)
diff --git a/ip/iplink.c b/ip/iplink.c
index ad22f2d7..173f149d 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -1073,16 +1073,12 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
 		.n.nlmsg_type = cmd,
 		.i.ifi_family = preferred_family,
 	};
-	struct nlmsghdr *answer;
 	int ret;
 
 	ret = iplink_parse(argc, argv, &req, &type);
 	if (ret < 0)
 		return ret;
 
-	if (echo_request)
-		req.n.nlmsg_flags |= NLM_F_ECHO | NLM_F_ACK;
-
 	if (type) {
 		struct link_util *lu;
 		struct rtattr *linkinfo;
@@ -1128,21 +1124,12 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
 	}
 
 	if (echo_request)
-		ret = rtnl_talk(&rth, &req.n, &answer);
+		ret = rtnl_echo_talk(&rth, &req.n, json, print_linkinfo);
 	else
 		ret = rtnl_talk(&rth, &req.n, NULL);
 
-	if (ret < 0)
-		return -2;
-
-	if (echo_request) {
-		new_json_obj(json);
-		open_json_object(NULL);
-		print_linkinfo(answer, stdout);
-		close_json_object();
-		delete_json_obj();
-		free(answer);
-	}
+	if (ret)
+		return ret;
 
 	/* remove device from cache; next use can refresh with new data */
 	ll_drop_by_index(req.i.ifi_index);
diff --git a/ip/ipnexthop.c b/ip/ipnexthop.c
index 59f8f2fb..c87e847f 100644
--- a/ip/ipnexthop.c
+++ b/ip/ipnexthop.c
@@ -919,12 +919,7 @@ static int ipnh_modify(int cmd, unsigned int flags, int argc, char **argv)
 		.n.nlmsg_type = cmd,
 		.nhm.nh_family = preferred_family,
 	};
-	struct nlmsghdr *answer;
 	__u32 nh_flags = 0;
-	int ret;
-
-	if (echo_request)
-		req.n.nlmsg_flags |= NLM_F_ECHO | NLM_F_ACK;
 
 	while (argc > 0) {
 		if (!strcmp(*argv, "id")) {
@@ -1005,23 +1000,9 @@ static int ipnh_modify(int cmd, unsigned int flags, int argc, char **argv)
 	req.nhm.nh_flags = nh_flags;
 
 	if (echo_request)
-		ret = rtnl_talk(&rth, &req.n, &answer);
-	else
-		ret = rtnl_talk(&rth, &req.n, NULL);
-
-	if (ret < 0)
-		return -2;
+		return rtnl_echo_talk(&rth, &req.n, json, print_nexthop_nocache);
 
-	if (echo_request) {
-		new_json_obj(json);
-		open_json_object(NULL);
-		print_nexthop_nocache(answer, (void *)stdout);
-		close_json_object();
-		delete_json_obj();
-		free(answer);
-	}
-
-	return 0;
+	return rtnl_talk(&rth, &req.n, NULL);
 }
 
 static int ipnh_get_id(__u32 id)
diff --git a/ip/iproute.c b/ip/iproute.c
index 4774aac0..8b2d1fbe 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -1123,7 +1123,6 @@ static int iproute_modify(int cmd, unsigned int flags, int argc, char **argv)
 	};
 	char  mxbuf[256];
 	struct rtattr *mxrta = (void *)mxbuf;
-	struct nlmsghdr *answer;
 	unsigned int mxlock = 0;
 	char  *d = NULL;
 	int gw_ok = 0;
@@ -1134,7 +1133,6 @@ static int iproute_modify(int cmd, unsigned int flags, int argc, char **argv)
 	int raw = 0;
 	int type_ok = 0;
 	__u32 nhid = 0;
-	int ret;
 
 	if (cmd != RTM_DELROUTE) {
 		req.r.rtm_protocol = RTPROT_BOOT;
@@ -1142,9 +1140,6 @@ static int iproute_modify(int cmd, unsigned int flags, int argc, char **argv)
 		req.r.rtm_type = RTN_UNICAST;
 	}
 
-	if (echo_request)
-		req.n.nlmsg_flags |= NLM_F_ECHO | NLM_F_ACK;
-
 	mxrta->rta_type = RTA_METRICS;
 	mxrta->rta_len = RTA_LENGTH(0);
 
@@ -1592,23 +1587,9 @@ static int iproute_modify(int cmd, unsigned int flags, int argc, char **argv)
 		req.r.rtm_type = RTN_UNICAST;
 
 	if (echo_request)
-		ret = rtnl_talk(&rth, &req.n, &answer);
-	else
-		ret = rtnl_talk(&rth, &req.n, NULL);
+		return rtnl_echo_talk(&rth, &req.n, json, print_route);
 
-	if (ret < 0)
-		return -2;
-
-	if (echo_request) {
-		new_json_obj(json);
-		open_json_object(NULL);
-		print_route(answer, (void *)stdout);
-		close_json_object();
-		delete_json_obj();
-		free(answer);
-	}
-
-	return 0;
+	return rtnl_talk(&rth, &req.n, NULL);
 }
 
 static int iproute_flush_cache(void)
diff --git a/ip/iprule.c b/ip/iprule.c
index af77e62c..8f750425 100644
--- a/ip/iprule.c
+++ b/ip/iprule.c
@@ -787,11 +787,6 @@ static int iprule_modify(int cmd, int argc, char **argv)
 		.frh.family = preferred_family,
 		.frh.action = FR_ACT_UNSPEC,
 	};
-	struct nlmsghdr *answer;
-	int ret;
-
-	if (echo_request)
-		req.n.nlmsg_flags |= NLM_F_ECHO | NLM_F_ACK;
 
 	if (cmd == RTM_NEWRULE) {
 		if (argc == 0) {
@@ -1022,23 +1017,9 @@ static int iprule_modify(int cmd, int argc, char **argv)
 		req.frh.table = RT_TABLE_MAIN;
 
 	if (echo_request)
-		ret = rtnl_talk(&rth, &req.n, &answer);
-	else
-		ret = rtnl_talk(&rth, &req.n, NULL);
-
-	if (ret < 0)
-		return -2;
-
-	if (echo_request) {
-		new_json_obj(json);
-		open_json_object(NULL);
-		print_rule(answer, stdout);
-		close_json_object();
-		delete_json_obj();
-		free(answer);
-	}
+		return rtnl_echo_talk(&rth, &req.n, json, print_rule);
 
-	return 0;
+	return rtnl_talk(&rth, &req.n, NULL);
 }
 
 int do_iprule(int argc, char **argv)
diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index c27627fe..07047bc7 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -1139,6 +1139,28 @@ static int __rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 	return __rtnl_talk_iov(rtnl, &iov, 1, answer, show_rtnl_err, errfn);
 }
 
+int rtnl_echo_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n, int json,
+		   int (*print_info)(struct nlmsghdr *n, void *arg))
+{
+	struct nlmsghdr *answer;
+	int ret;
+
+	n->nlmsg_flags |= NLM_F_ECHO | NLM_F_ACK;
+
+	ret = rtnl_talk(rtnl, n, &answer);
+	if (ret)
+		return ret;
+
+	new_json_obj(json);
+	open_json_object(NULL);
+	print_info(answer, stdout);
+	close_json_object();
+	delete_json_obj();
+	free(answer);
+
+	return 0;
+}
+
 int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 	      struct nlmsghdr **answer)
 {
-- 
2.37.2

Re: [PATCHv3 iproute2-next] rtnetlink: add new function rtnl_echo_talk()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to iproute2/iproute2-next.git (main)
by David Ahern <dsahern@kernel.org>:

On Thu, 29 Sep 2022 16:10:16 +0800 you wrote:
> Add a new function rtnl_echo_talk() that could be used when the
> sub-component supports NLM_F_ECHO flag. With this function we can
> remove the redundant code added by commit b264b4c6568c7 ("ip: add
> NLM_F_ECHO support").
> 
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> 
> [...]

Here is the summary with links:
  - [PATCHv3,iproute2-next] rtnetlink: add new function rtnl_echo_talk()
    https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/commit/?id=6c09257f1bf6

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCHv3 iproute2-next] rtnetlink: add new function rtnl_echo_talk()

[Ido Schimmel]

On Thu, Sep 29, 2022 at 04:10:16PM +0800, Hangbin Liu wrote:
> diff --git a/ip/ipnexthop.c b/ip/ipnexthop.c
> index 59f8f2fb..c87e847f 100644
> --- a/ip/ipnexthop.c
> +++ b/ip/ipnexthop.c
> @@ -919,12 +919,7 @@ static int ipnh_modify(int cmd, unsigned int flags, int argc, char **argv)
>  		.n.nlmsg_type = cmd,
>  		.nhm.nh_family = preferred_family,
>  	};
> -	struct nlmsghdr *answer;
>  	__u32 nh_flags = 0;
> -	int ret;
> -
> -	if (echo_request)
> -		req.n.nlmsg_flags |= NLM_F_ECHO | NLM_F_ACK;
>  
>  	while (argc > 0) {
>  		if (!strcmp(*argv, "id")) {
> @@ -1005,23 +1000,9 @@ static int ipnh_modify(int cmd, unsigned int flags, int argc, char **argv)
>  	req.nhm.nh_flags = nh_flags;
>  
>  	if (echo_request)
> -		ret = rtnl_talk(&rth, &req.n, &answer);
> -	else
> -		ret = rtnl_talk(&rth, &req.n, NULL);
> -
> -	if (ret < 0)
> -		return -2;
> +		return rtnl_echo_talk(&rth, &req.n, json, print_nexthop_nocache);
>  
> -	if (echo_request) {
> -		new_json_obj(json);
> -		open_json_object(NULL);
> -		print_nexthop_nocache(answer, (void *)stdout);
> -		close_json_object();
> -		delete_json_obj();
> -		free(answer);
> -	}
> -
> -	return 0;
> +	return rtnl_talk(&rth, &req.n, NULL);
>  }

Hangbin,

This change breaks the nexthop selftest:
tools/testing/selftests/net/fib_nexthops.sh

Which is specifically checking for "2" as the error code. Example:

# attempt to create nh without a device or gw - fails
run_cmd "$IP nexthop add id 1"
log_test $? 2 "Nexthop with no device or gateway"

I think it's better to restore the original error code than "fixing" all
the tests / applications that rely on it.

The return code of other subcommands was also changed by this patch, but
so far all the failures I have seen are related to "nexthop" subcommand.

Thanks

Re: [PATCHv3 iproute2-next] rtnetlink: add new function rtnl_echo_talk()

[Hangbin Liu]

On Tue, Nov 08, 2022 at 10:40:45AM +0200, Ido Schimmel wrote:
> > +	return rtnl_talk(&rth, &req.n, NULL);
> >  }
> 
> Hangbin,
> 
> This change breaks the nexthop selftest:
> tools/testing/selftests/net/fib_nexthops.sh
> 
> Which is specifically checking for "2" as the error code. Example:

Hi Ido,

Thanks for the report.

> 
> # attempt to create nh without a device or gw - fails
> run_cmd "$IP nexthop add id 1"
> log_test $? 2 "Nexthop with no device or gateway"
> 
> I think it's better to restore the original error code than "fixing" all
> the tests / applications that rely on it.

I can fix this either in iproute2 or in the selftests.
I'd perfer ask David's opinion.

> 
> The return code of other subcommands was also changed by this patch, but
> so far all the failures I have seen are related to "nexthop" subcommand.

I grep "log_test \$? 2" in selftest/net folder and found the following tests
would use it

fib_tests.sh
test_vxlan_vnifiltering.sh
fcnal-test.sh
fib_nexthops.sh

Thanks
Hangbin

Re: [PATCHv3 iproute2-next] rtnetlink: add new function rtnl_echo_talk()

[Ido Schimmel]

On Tue, Nov 08, 2022 at 05:09:05PM +0800, Hangbin Liu wrote:
> On Tue, Nov 08, 2022 at 10:40:45AM +0200, Ido Schimmel wrote:
> > > +	return rtnl_talk(&rth, &req.n, NULL);
> > >  }
> > 
> > Hangbin,
> > 
> > This change breaks the nexthop selftest:
> > tools/testing/selftests/net/fib_nexthops.sh
> > 
> > Which is specifically checking for "2" as the error code. Example:
> 
> Hi Ido,
> 
> Thanks for the report.
> 
> > 
> > # attempt to create nh without a device or gw - fails
> > run_cmd "$IP nexthop add id 1"
> > log_test $? 2 "Nexthop with no device or gateway"
> > 
> > I think it's better to restore the original error code than "fixing" all
> > the tests / applications that rely on it.
> 
> I can fix this either in iproute2 or in the selftests.
> I'd perfer ask David's opinion.

Sure, but note that:

1. Other than the 4 selftests that we know about and can easily patch,
there might be a lot of other applications that invoke iproute2 and
expect this return code. It is used by iproute2 since at least 2004.

2. There is already precedence for restoring the original code. See
commit d58ba4ba2a53 ("ip: return correct exit code on route failure").

> 
> > 
> > The return code of other subcommands was also changed by this patch, but
> > so far all the failures I have seen are related to "nexthop" subcommand.
> 
> I grep "log_test \$? 2" in selftest/net folder and found the following tests
> would use it
> 
> fib_tests.sh
> test_vxlan_vnifiltering.sh
> fcnal-test.sh
> fib_nexthops.sh
> 
> Thanks
> Hangbin

Re: [PATCHv3 iproute2-next] rtnetlink: add new function rtnl_echo_talk()

[Hangbin Liu]

On Tue, Nov 08, 2022 at 11:40:06AM +0200, Ido Schimmel wrote:
> > I can fix this either in iproute2 or in the selftests.
> > I'd perfer ask David's opinion.
> 
> Sure, but note that:
> 
> 1. Other than the 4 selftests that we know about and can easily patch,
> there might be a lot of other applications that invoke iproute2 and
> expect this return code. It is used by iproute2 since at least 2004.
> 
> 2. There is already precedence for restoring the original code. See
> commit d58ba4ba2a53 ("ip: return correct exit code on route failure").

OK, I will post a iproute fix first. If David or others has comments.
Please just NACK the new patch.

Thanks
Hangbin