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

[Chris Mi]

> -Original Message-
> From: David Ahern [mailto:dsah...@gmail.com]
> Sent: Saturday, January 6, 2018 1:51 AM
> To: Chris Mi <chr...@mellanox.com>; netdev@vger.kernel.org
> Cc: gerlitz...@gmail.com; step...@networkplumber.org;
> marcelo.leit...@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 <chr...@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.

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

[David Ahern]

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 
> ---
>  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.

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

[Chris Mi]

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 
---
 include/libnetlink.h |  3 +++
 lib/libnetlink.c | 66 ++--
 2 files changed, 51 insertions(+), 18 deletions(-)

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 = ,
.msg_namelen = sizeof(nladdr),
.msg_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 = >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, , 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, , );
 
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 = ,
+   .msg_namelen = sizeof(nladdr),
+   .msg_iov = ,
+   .msg_iovlen = 1,
+   };
+
+   return __rtnl_talk_msg(rtnl, , answer, show_rtnl_err, errfn);
+}
+
 int rtnl_talk(struct rtnl_handle *rtnl,