diff mbox series

[iproute2,v6,1/3] lib/libnetlink: Add a function rtnl_talk_msg

Message ID 20180104073454.11867-2-chrism@mellanox.com
State Changes Requested, archived
Delegated to: David Ahern
Headers show
Series tc: Add -bs option to batch mode | expand

Commit Message

Chris Mi Jan. 4, 2018, 7:34 a.m. UTC 
rtnl_talk can only send a single message to kernel. Add a new function
rtnl_talk_msg that can send multiple messages to kernel.

Signed-off-by: Chris Mi <chrism@mellanox.com>
---
 include/libnetlink.h |  3 +++
 lib/libnetlink.c     | 66 ++++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 51 insertions(+), 18 deletions(-)

Comments

David Ahern Jan. 5, 2018, 5:50 p.m. UTC | #1 
On 1/4/18 12:34 AM, Chris Mi wrote:
> rtnl_talk can only send a single message to kernel. Add a new function
> rtnl_talk_msg that can send multiple messages to kernel.
> 
> Signed-off-by: Chris Mi <chrism@mellanox.com>
> ---
>  include/libnetlink.h |  3 +++
>  lib/libnetlink.c     | 66 ++++++++++++++++++++++++++++++++++++++--------------
>  2 files changed, 51 insertions(+), 18 deletions(-)
> 

I think you should add an argument to rtnl_talk_msg to return the number
of messages processed. That can be used to refine which line failed. As
batch size increases the current design puts the burden on the user to
scan a lot of lines to find the one that fails:

tc -b tc.batch  -bs 50
RTNETLINK answers: File exists
We have an error talking to the kernel, -1
Command failed tc.batch:2-51

We should be able to tell them exactly which line failed.

Also, it would be better to call this rtnl_talk_iov, take an iov as an
argument and have a common rtnl_talk_msg for existing code and this new one.

As it stands you are having to add:
   struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };

to tc functions when it really only needs to know about iov's.
Chris Mi Jan. 9, 2018, 6:44 a.m. UTC | #2 
> -----Original Message-----

> From: David Ahern [mailto:dsahern@gmail.com]

> Sent: Saturday, January 6, 2018 1:51 AM

> To: Chris Mi <chrism@mellanox.com>; netdev@vger.kernel.org

> Cc: gerlitz.or@gmail.com; stephen@networkplumber.org;

> marcelo.leitner@gmail.com

> Subject: Re: [patch iproute2 v6 1/3] lib/libnetlink: Add a function

> rtnl_talk_msg

> 

> On 1/4/18 12:34 AM, Chris Mi wrote:

> > rtnl_talk can only send a single message to kernel. Add a new function

> > rtnl_talk_msg that can send multiple messages to kernel.

> >

> > Signed-off-by: Chris Mi <chrism@mellanox.com>

> > ---

> >  include/libnetlink.h |  3 +++

> >  lib/libnetlink.c     | 66 ++++++++++++++++++++++++++++++++++++++----

> ----------

> >  2 files changed, 51 insertions(+), 18 deletions(-)

> >

> 

> I think you should add an argument to rtnl_talk_msg to return the number of

> messages processed. That can be used to refine which line failed. As batch

> size increases the current design puts the burden on the user to scan a lot of

> lines to find the one that fails:

> 

> tc -b tc.batch  -bs 50

> RTNETLINK answers: File exists

> We have an error talking to the kernel, -1 Command failed tc.batch:2-51

> 

> We should be able to tell them exactly which line failed.

Done.
> 

> Also, it would be better to call this rtnl_talk_iov, take an iov as an argument

> and have a common rtnl_talk_msg for existing code and this new one.

> 

> As it stands you are having to add:

>    struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };

> 

> to tc functions when it really only needs to know about iov's.

Done.
diff mbox series

Patch

diff --git a/include/libnetlink.h b/include/libnetlink.h
index a4d83b9e..01d98b16 100644
--- a/include/libnetlink.h
+++ b/include/libnetlink.h
@@ -96,6 +96,9 @@  int rtnl_dump_filter_nc(struct rtnl_handle *rth,
 int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 	      struct nlmsghdr **answer)
 	__attribute__((warn_unused_result));
+int rtnl_talk_msg(struct rtnl_handle *rtnl, struct msghdr *m,
+		  struct nlmsghdr **answer)
+	__attribute__((warn_unused_result));
 int rtnl_talk_extack(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 	      struct nlmsghdr **answer, nl_ext_ack_fn_t errfn)
 	__attribute__((warn_unused_result));
diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 00e6ce0c..49ee1208 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -581,38 +581,40 @@  static void rtnl_talk_error(struct nlmsghdr *h, struct nlmsgerr *err,
 		strerror(-err->error));
 }
 
-static int __rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
-		       struct nlmsghdr **answer,
-		       bool show_rtnl_err, nl_ext_ack_fn_t errfn)
+static int __rtnl_talk_msg(struct rtnl_handle *rtnl, struct msghdr *m,
+			   struct nlmsghdr **answer,
+			   bool show_rtnl_err, nl_ext_ack_fn_t errfn)
 {
-	int status;
-	unsigned int seq;
-	struct nlmsghdr *h;
 	struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
-	struct iovec iov = {
-		.iov_base = n,
-		.iov_len = n->nlmsg_len
-	};
+	int i, status, iovlen = m->msg_iovlen;
+	unsigned int seq = 0;
+	struct nlmsghdr *h;
+	struct iovec iov;
 	struct msghdr msg = {
 		.msg_name = &nladdr,
 		.msg_namelen = sizeof(nladdr),
 		.msg_iov = &iov,
 		.msg_iovlen = 1,
 	};
-	char *buf;
-
-	n->nlmsg_seq = seq = ++rtnl->seq;
 
-	if (answer == NULL)
-		n->nlmsg_flags |= NLM_F_ACK;
+	for (i = 0; i < iovlen; i++) {
+		struct iovec *v;
+		v = &m->msg_iov[i];
+		h = v->iov_base;
+		h->nlmsg_seq = seq = ++rtnl->seq;
+		if (answer == NULL)
+			h->nlmsg_flags |= NLM_F_ACK;
+	}
 
-	status = sendmsg(rtnl->fd, &msg, 0);
+	status = sendmsg(rtnl->fd, m, 0);
 	if (status < 0) {
 		perror("Cannot talk to rtnetlink");
 		return -1;
 	}
 
 	while (1) {
+		char *buf;
+next:
 		status = rtnl_recvmsg(rtnl->fd, &msg, &buf);
 
 		if (status < 0)
@@ -642,7 +644,7 @@  static int __rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 
 			if (nladdr.nl_pid != 0 ||
 			    h->nlmsg_pid != rtnl->local.nl_pid ||
-			    h->nlmsg_seq != seq) {
+			    h->nlmsg_seq > seq || h->nlmsg_seq < seq - iovlen) {
 				/* Don't forget to skip that message. */
 				status -= NLMSG_ALIGN(len);
 				h = (struct nlmsghdr *)((char *)h + NLMSG_ALIGN(len));
@@ -662,7 +664,10 @@  static int __rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 						*answer = (struct nlmsghdr *)buf;
 					else
 						free(buf);
-					return 0;
+					if (h->nlmsg_seq == seq)
+						return 0;
+					else
+						goto next;
 				}
 
 				if (rtnl->proto != NETLINK_SOCK_DIAG &&
@@ -698,12 +703,37 @@  static int __rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 	}
 }
 
+static int __rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
+		       struct nlmsghdr **answer,
+		       bool show_rtnl_err, nl_ext_ack_fn_t errfn)
+{
+	struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
+	struct iovec iov = {
+		.iov_base = n,
+		.iov_len = n->nlmsg_len
+	};
+	struct msghdr msg = {
+		.msg_name = &nladdr,
+		.msg_namelen = sizeof(nladdr),
+		.msg_iov = &iov,
+		.msg_iovlen = 1,
+	};
+
+	return __rtnl_talk_msg(rtnl, &msg, answer, show_rtnl_err, errfn);
+}
+
 int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 	      struct nlmsghdr **answer)
 {
 	return __rtnl_talk(rtnl, n, answer, true, NULL);
 }
 
+int rtnl_talk_msg(struct rtnl_handle *rtnl, struct msghdr *m,
+	      struct nlmsghdr **answer)
+{
+	return __rtnl_talk_msg(rtnl, m, answer, true, NULL);
+}
+
 int rtnl_talk_extack(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 		     struct nlmsghdr **answer,
 		     nl_ext_ack_fn_t errfn)