[PATCH iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[PATCH iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Alexander Mikhalitsyn]

We started to use in-kernel filtering feature which allows to get only needed
tables (see iproute_dump_filter()). From the kernel side it's implemented in
net/ipv4/fib_frontend.c (inet_dump_fib), net/ipv6/ip6_fib.c (inet6_dump_fib).
The problem here is that behaviour of "ip route save" was changed after
c7e6371bc ("ip route: Add protocol, table id and device to dump request").
If filters are used, then kernel returns ENOENT error if requested table is absent,
but in newly created net namespace even RT_TABLE_MAIN table doesn't exist.
It is really allocated, for instance, after issuing "ip l set lo up".

Reproducer is fairly simple:
$ unshare -n ip route save > dump
Error: ipv4: FIB table does not exist.
Dump terminated

Expected result here is to get empty dump file (as it was before this change).

This affects on CRIU [1] because we use ip route save in dump process, to workaround
problem in tests we just put loopback interface up in each net namespace.
Other users also met this problem [2].

Links:
[1] https://github.com/checkpoint-restore/criu/issues/747
[2] https://www.spinics.net/lists/netdev/msg559739.html

Fixes: c7e6371bc ("ip route: Add protocol, table id and device to dump request")

Cc: David Ahern <dsahern@kernel.org>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Andrei Vagin <avagin@gmail.com>
Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
Signed-off-by: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>
---
 ip/iproute.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/ip/iproute.c b/ip/iproute.c
index 5853f026..b70acc00 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -1734,6 +1734,7 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
 	char *od = NULL;
 	unsigned int mark = 0;
 	rtnl_filter_t filter_fn;
+	int ret;
 
 	if (action == IPROUTE_SAVE) {
 		if (save_route_prep())
@@ -1939,7 +1940,11 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
 
 	new_json_obj(json);
 
-	if (rtnl_dump_filter(&rth, filter_fn, stdout) < 0) {
+	ret = rtnl_dump_filter(&rth, filter_fn, stdout);
+
+	/* Let's ignore ENOENT error if we want to dump RT_TABLE_MAIN table */
+	if (ret < 0 &&
+	    !(errno == ENOENT && filter.tb == RT_TABLE_MAIN)) {
 		fprintf(stderr, "Dump terminated\n");
 		return -2;
 	}
-- 
2.31.1

Re: [PATCH iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[David Ahern]

On 6/22/21 9:03 AM, Alexander Mikhalitsyn wrote:
> We started to use in-kernel filtering feature which allows to get only needed
> tables (see iproute_dump_filter()). From the kernel side it's implemented in
> net/ipv4/fib_frontend.c (inet_dump_fib), net/ipv6/ip6_fib.c (inet6_dump_fib).
> The problem here is that behaviour of "ip route save" was changed after
> c7e6371bc ("ip route: Add protocol, table id and device to dump request").
> If filters are used, then kernel returns ENOENT error if requested table is absent,
> but in newly created net namespace even RT_TABLE_MAIN table doesn't exist.
> It is really allocated, for instance, after issuing "ip l set lo up".
> 
> Reproducer is fairly simple:
> $ unshare -n ip route save > dump
> Error: ipv4: FIB table does not exist.
> Dump terminated

The above command on 5.4 kernel with corresponding iproute2 does not
show that error. Is your kernel compiled with CONFIG_IP_MULTIPLE_TABLES
enabled?

> 
> Expected result here is to get empty dump file (as it was before this change).
> 
> This affects on CRIU [1] because we use ip route save in dump process, to workaround
> problem in tests we just put loopback interface up in each net namespace.
> Other users also met this problem [2].
> 
> Links:
> [1] https://github.com/checkpoint-restore/criu/issues/747
> [2] https://www.spinics.net/lists/netdev/msg559739.html
> 
> Fixes: c7e6371bc ("ip route: Add protocol, table id and device to dump request")
> 
> Cc: David Ahern <dsahern@kernel.org>
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Cc: Andrei Vagin <avagin@gmail.com>
> Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
> Signed-off-by: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>
> ---
>  ip/iproute.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/ip/iproute.c b/ip/iproute.c
> index 5853f026..b70acc00 100644
> --- a/ip/iproute.c
> +++ b/ip/iproute.c
> @@ -1734,6 +1734,7 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
>  	char *od = NULL;
>  	unsigned int mark = 0;
>  	rtnl_filter_t filter_fn;
> +	int ret;
>  
>  	if (action == IPROUTE_SAVE) {
>  		if (save_route_prep())
> @@ -1939,7 +1940,11 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
>  
>  	new_json_obj(json);
>  
> -	if (rtnl_dump_filter(&rth, filter_fn, stdout) < 0) {
> +	ret = rtnl_dump_filter(&rth, filter_fn, stdout);
> +
> +	/* Let's ignore ENOENT error if we want to dump RT_TABLE_MAIN table */
> +	if (ret < 0 &&

ret temp variable is not needed; just add the extra checks.

> +	    !(errno == ENOENT && filter.tb == RT_TABLE_MAIN)) {
>  		fprintf(stderr, "Dump terminated\n");
>  		return -2;
>  	}
> 

This looks fine to me, but I want clarification on the kernel config. As
I recall with multiple tables and fib rules tables are created when net
namespace is created.

Re: [PATCH iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Alexander Mikhalitsyn]

On Wed, 23 Jun 2021 09:36:29 -0600
David Ahern <dsahern@gmail.com> wrote:

> On 6/22/21 9:03 AM, Alexander Mikhalitsyn wrote:
> > We started to use in-kernel filtering feature which allows to get only needed
> > tables (see iproute_dump_filter()). From the kernel side it's implemented in
> > net/ipv4/fib_frontend.c (inet_dump_fib), net/ipv6/ip6_fib.c (inet6_dump_fib).
> > The problem here is that behaviour of "ip route save" was changed after
> > c7e6371bc ("ip route: Add protocol, table id and device to dump request").
> > If filters are used, then kernel returns ENOENT error if requested table is absent,
> > but in newly created net namespace even RT_TABLE_MAIN table doesn't exist.
> > It is really allocated, for instance, after issuing "ip l set lo up".
> > 
> > Reproducer is fairly simple:
> > $ unshare -n ip route save > dump
> > Error: ipv4: FIB table does not exist.
> > Dump terminated
> 
> The above command on 5.4 kernel with corresponding iproute2 does not
> show that error. Is your kernel compiled with CONFIG_IP_MULTIPLE_TABLES
> enabled?
> 
Yes it is.
$ grep CONFIG_IP_MULTIPLE_TABLES .config
CONFIG_IP_MULTIPLE_TABLES=y

> > 
> > Expected result here is to get empty dump file (as it was before this change).
> > 
> > This affects on CRIU [1] because we use ip route save in dump process, to workaround
> > problem in tests we just put loopback interface up in each net namespace.
> > Other users also met this problem [2].
> > 
> > Links:
> > [1] https://github.com/checkpoint-restore/criu/issues/747
> > [2] https://www.spinics.net/lists/netdev/msg559739.html
> > 
> > Fixes: c7e6371bc ("ip route: Add protocol, table id and device to dump request")
> > 
> > Cc: David Ahern <dsahern@kernel.org>
> > Cc: Stephen Hemminger <stephen@networkplumber.org>
> > Cc: Andrei Vagin <avagin@gmail.com>
> > Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
> > Signed-off-by: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>
> > ---
> >  ip/iproute.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/ip/iproute.c b/ip/iproute.c
> > index 5853f026..b70acc00 100644
> > --- a/ip/iproute.c
> > +++ b/ip/iproute.c
> > @@ -1734,6 +1734,7 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
> >  	char *od = NULL;
> >  	unsigned int mark = 0;
> >  	rtnl_filter_t filter_fn;
> > +	int ret;
> >  
> >  	if (action == IPROUTE_SAVE) {
> >  		if (save_route_prep())
> > @@ -1939,7 +1940,11 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
> >  
> >  	new_json_obj(json);
> >  
> > -	if (rtnl_dump_filter(&rth, filter_fn, stdout) < 0) {
> > +	ret = rtnl_dump_filter(&rth, filter_fn, stdout);
> > +
> > +	/* Let's ignore ENOENT error if we want to dump RT_TABLE_MAIN table */
> > +	if (ret < 0 &&
> 
> ret temp variable is not needed; just add the extra checks.

Sure, thanks!
I will send v2 if all fine in general with this approach to fix the problem.

> 
> > +	    !(errno == ENOENT && filter.tb == RT_TABLE_MAIN)) {
> >  		fprintf(stderr, "Dump terminated\n");
> >  		return -2;
> >  	}
> > 
> 
> This looks fine to me, but I want clarification on the kernel config. As
> I recall with multiple tables and fib rules tables are created when net
> namespace is created.
I've traced how fib tables allocated (fib_new_table function) during
$ ip l set lo up
and stack looks like that:
ip   740 [003] 99894.075766: probe:fib_new_table: (ffffffffb08ff9a0)
        ffffffffb08ff9a1 fib_new_table+0x1 ([kernel.kallsyms])
        ffffffffb09001d1 fib_magic.isra.24+0xc1 ([kernel.kallsyms])
        ffffffffb0901b3d fib_add_ifaddr+0x16d ([kernel.kallsyms])
        ffffffffb0901be5 fib_netdev_event+0x95 ([kernel.kallsyms])
        ffffffffafed5457 notifier_call_chain+0x47 ([kernel.kallsyms])
        ffffffffb07f249b __dev_notify_flags+0x5b ([kernel.kallsyms])
        ffffffffb07f2c48 dev_change_flags+0x48 ([kernel.kallsyms])
        ffffffffb0805d34 do_setlink+0x314 ([kernel.kallsyms])
        ffffffffb080ad9d __rtnl_newlink+0x53d ([kernel.kallsyms])
        ffffffffb080b143 rtnl_newlink+0x43 ([kernel.kallsyms])
        ffffffffb08048ba rtnetlink_rcv_msg+0x22a ([kernel.kallsyms])
        ffffffffb0848dfc netlink_rcv_skb+0x4c ([kernel.kallsyms])
        ffffffffb084868d netlink_unicast+0x21d ([kernel.kallsyms])
        ffffffffb08488fe netlink_sendmsg+0x22e ([kernel.kallsyms])
        ffffffffb07cad9c sock_sendmsg+0x4c ([kernel.kallsyms])
        ffffffffb07cb0bb ____sys_sendmsg+0x1eb ([kernel.kallsyms])
        ffffffffb07cc74c ___sys_sendmsg+0x7c ([kernel.kallsyms])
        ffffffffb07cc817 __sys_sendmsg+0x57 ([kernel.kallsyms])
        ffffffffafe0279b do_syscall_64+0x5b ([kernel.kallsyms])
        ffffffffb0c000ad entry_SYSCALL_64_after_hwframe+0x65 ([kernel.kallsyms])
            7f556c716d78 __libc_sendmsg+0x18 (/usr/lib64/libc-2.28.so)

During trace of:
$ unshare -n
I see no fib_new_table() calls.

Thank you very much for your attention to the patch.

Regards,
Alex

[PATCHv2 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Alexander Mikhalitsyn]

We started to use in-kernel filtering feature which allows to get only needed
tables (see iproute_dump_filter()). From the kernel side it's implemented in
net/ipv4/fib_frontend.c (inet_dump_fib), net/ipv6/ip6_fib.c (inet6_dump_fib).
The problem here is that behaviour of "ip route save" was changed after
c7e6371bc ("ip route: Add protocol, table id and device to dump request").
If filters are used, then kernel returns ENOENT error if requested table is absent,
but in newly created net namespace even RT_TABLE_MAIN table doesn't exist.
It is really allocated, for instance, after issuing "ip l set lo up".

Reproducer is fairly simple:
$ unshare -n ip route save > dump
Error: ipv4: FIB table does not exist.
Dump terminated

Expected result here is to get empty dump file (as it was before this change).

v2: reworked, so, now it takes into account NLMSGERR_ATTR_MSG
(see nl_dump_ext_ack_done() function). We want to suppress error messages
in stderr about absent FIB table from kernel too.

Fixes: c7e6371bc ("ip route: Add protocol, table id and device to dump request")
Cc: David Ahern <dsahern@gmail.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Andrei Vagin <avagin@gmail.com>
Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
Signed-off-by: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>
---
 include/libnetlink.h |  5 +++++
 ip/iproute.c         |  8 +++++++-
 lib/libnetlink.c     | 31 ++++++++++++++++++++++++++-----
 3 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/include/libnetlink.h b/include/libnetlink.h
index b9073a6a..93c22a09 100644
--- a/include/libnetlink.h
+++ b/include/libnetlink.h
@@ -121,6 +121,11 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
 			void *arg, __u16 nc_flags);
 #define rtnl_dump_filter(rth, filter, arg) \
 	rtnl_dump_filter_nc(rth, filter, arg, 0)
+int rtnl_dump_filter_suppress_rtnl_errmsg_nc(struct rtnl_handle *rth,
+		     rtnl_filter_t filter,
+		     void *arg1, __u16 nc_flags, const int *errnos);
+#define rtnl_dump_filter_suppress_rtnl_errmsg(rth, filter, arg, errnos) \
+	rtnl_dump_filter_suppress_rtnl_errmsg_nc(rth, filter, arg, 0, errnos)
 int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 	      struct nlmsghdr **answer)
 	__attribute__((warn_unused_result));
diff --git a/ip/iproute.c b/ip/iproute.c
index 5853f026..796d6d17 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -1734,6 +1734,8 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
 	char *od = NULL;
 	unsigned int mark = 0;
 	rtnl_filter_t filter_fn;
+	/* last 0 is array trailing */
+	int suppress_rtnl_errnos[2] = { 0, 0 };
 
 	if (action == IPROUTE_SAVE) {
 		if (save_route_prep())
@@ -1939,7 +1941,11 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
 
 	new_json_obj(json);
 
-	if (rtnl_dump_filter(&rth, filter_fn, stdout) < 0) {
+	if (filter.tb == RT_TABLE_MAIN)
+		suppress_rtnl_errnos[0] = ENOENT;
+
+	if (rtnl_dump_filter_suppress_rtnl_errmsg(&rth, filter_fn, stdout,
+						  suppress_rtnl_errnos) < 0) {
 		fprintf(stderr, "Dump terminated\n");
 		return -2;
 	}
diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index c958aa57..310203c2 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -673,7 +673,7 @@ int rtnl_dump_request_n(struct rtnl_handle *rth, struct nlmsghdr *n)
 	return sendmsg(rth->fd, &msg, 0);
 }
 
-static int rtnl_dump_done(struct nlmsghdr *h)
+static int rtnl_dump_done(struct nlmsghdr *h, const int *errnos)
 {
 	int len = *(int *)NLMSG_DATA(h);
 
@@ -683,11 +683,19 @@ static int rtnl_dump_done(struct nlmsghdr *h)
 	}
 
 	if (len < 0) {
+		errno = -len;
+
+		while (errnos && *errnos) {
+			if (errno == *errnos)
+				return 0;
+
+			errnos++;
+		}
+
 		/* check for any messages returned from kernel */
 		if (nl_dump_ext_ack_done(h, len))
 			return len;
 
-		errno = -len;
 		switch (errno) {
 		case ENOENT:
 		case EOPNOTSUPP:
@@ -789,7 +797,8 @@ static int rtnl_recvmsg(int fd, struct msghdr *msg, char **answer)
 }
 
 static int rtnl_dump_filter_l(struct rtnl_handle *rth,
-			      const struct rtnl_dump_filter_arg *arg)
+			      const struct rtnl_dump_filter_arg *arg,
+			      const int *errnos)
 {
 	struct sockaddr_nl nladdr;
 	struct iovec iov;
@@ -834,7 +843,7 @@ static int rtnl_dump_filter_l(struct rtnl_handle *rth,
 					dump_intr = 1;
 
 				if (h->nlmsg_type == NLMSG_DONE) {
-					err = rtnl_dump_done(h);
+					err = rtnl_dump_done(h, errnos);
 					if (err < 0) {
 						free(buf);
 						return -1;
@@ -891,7 +900,19 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
 		{ .filter = NULL,   .arg1 = NULL, .nc_flags = 0, },
 	};
 
-	return rtnl_dump_filter_l(rth, a);
+	return rtnl_dump_filter_l(rth, a, NULL);
+}
+
+int rtnl_dump_filter_suppress_rtnl_errmsg_nc(struct rtnl_handle *rth,
+		     rtnl_filter_t filter,
+		     void *arg1, __u16 nc_flags, const int *errnos)
+{
+	const struct rtnl_dump_filter_arg a[2] = {
+		{ .filter = filter, .arg1 = arg1, .nc_flags = nc_flags, },
+		{ .filter = NULL,   .arg1 = NULL, .nc_flags = 0, },
+	};
+
+	return rtnl_dump_filter_l(rth, a, errnos);
 }
 
 static void rtnl_talk_error(struct nlmsghdr *h, struct nlmsgerr *err,
-- 
2.31.1

Re: [PATCHv2 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Stephen Hemminger]

On Thu, 24 Jun 2021 18:28:12 +0300
Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:

> We started to use in-kernel filtering feature which allows to get only needed
> tables (see iproute_dump_filter()). From the kernel side it's implemented in
> net/ipv4/fib_frontend.c (inet_dump_fib), net/ipv6/ip6_fib.c (inet6_dump_fib).
> The problem here is that behaviour of "ip route save" was changed after
> c7e6371bc ("ip route: Add protocol, table id and device to dump request").
> If filters are used, then kernel returns ENOENT error if requested table is absent,
> but in newly created net namespace even RT_TABLE_MAIN table doesn't exist.
> It is really allocated, for instance, after issuing "ip l set lo up".
> 
> Reproducer is fairly simple:
> $ unshare -n ip route save > dump
> Error: ipv4: FIB table does not exist.
> Dump terminated
> 
> Expected result here is to get empty dump file (as it was before this change).
> 
> v2: reworked, so, now it takes into account NLMSGERR_ATTR_MSG
> (see nl_dump_ext_ack_done() function). We want to suppress error messages
> in stderr about absent FIB table from kernel too.
> 
> Fixes: c7e6371bc ("ip route: Add protocol, table id and device to dump request")
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Cc: Andrei Vagin <avagin@gmail.com>
> Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
> Signed-off-by: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>
> ---
>  include/libnetlink.h |  5 +++++
>  ip/iproute.c         |  8 +++++++-
>  lib/libnetlink.c     | 31 ++++++++++++++++++++++++++-----
>  3 files changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/include/libnetlink.h b/include/libnetlink.h
> index b9073a6a..93c22a09 100644
> --- a/include/libnetlink.h
> +++ b/include/libnetlink.h
> @@ -121,6 +121,11 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
>  			void *arg, __u16 nc_flags);
>  #define rtnl_dump_filter(rth, filter, arg) \
>  	rtnl_dump_filter_nc(rth, filter, arg, 0)
> +int rtnl_dump_filter_suppress_rtnl_errmsg_nc(struct rtnl_handle *rth,
> +		     rtnl_filter_t filter,
> +		     void *arg1, __u16 nc_flags, const int *errnos);
> +#define rtnl_dump_filter_suppress_rtnl_errmsg(rth, filter, arg, errnos) \
> +	rtnl_dump_filter_suppress_rtnl_errmsg_nc(rth, filter, arg, 0, errnos)
>  int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
>  	      struct nlmsghdr **answer)
>  	__attribute__((warn_unused_result));
> diff --git a/ip/iproute.c b/ip/iproute.c
> index 5853f026..796d6d17 100644
> --- a/ip/iproute.c
> +++ b/ip/iproute.c
> @@ -1734,6 +1734,8 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
>  	char *od = NULL;
>  	unsigned int mark = 0;
>  	rtnl_filter_t filter_fn;
> +	/* last 0 is array trailing */
> +	int suppress_rtnl_errnos[2] = { 0, 0 };
>  

The design would be clearer if there were two arguments rather than magic array of size 2.
Also these are being used as boolean.

Re: [PATCHv2 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Alexander Mikhalitsyn]

On Thu, 24 Jun 2021 08:36:47 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

> On Thu, 24 Jun 2021 18:28:12 +0300
> Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:
> 
> > We started to use in-kernel filtering feature which allows to get only needed
> > tables (see iproute_dump_filter()). From the kernel side it's implemented in
> > net/ipv4/fib_frontend.c (inet_dump_fib), net/ipv6/ip6_fib.c (inet6_dump_fib).
> > The problem here is that behaviour of "ip route save" was changed after
> > c7e6371bc ("ip route: Add protocol, table id and device to dump request").
> > If filters are used, then kernel returns ENOENT error if requested table is absent,
> > but in newly created net namespace even RT_TABLE_MAIN table doesn't exist.
> > It is really allocated, for instance, after issuing "ip l set lo up".
> > 
> > Reproducer is fairly simple:
> > $ unshare -n ip route save > dump
> > Error: ipv4: FIB table does not exist.
> > Dump terminated
> > 
> > Expected result here is to get empty dump file (as it was before this change).
> > 
> > v2: reworked, so, now it takes into account NLMSGERR_ATTR_MSG
> > (see nl_dump_ext_ack_done() function). We want to suppress error messages
> > in stderr about absent FIB table from kernel too.
> > 
> > Fixes: c7e6371bc ("ip route: Add protocol, table id and device to dump request")
> > Cc: David Ahern <dsahern@gmail.com>
> > Cc: Stephen Hemminger <stephen@networkplumber.org>
> > Cc: Andrei Vagin <avagin@gmail.com>
> > Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
> > Signed-off-by: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>
> > ---
> >  include/libnetlink.h |  5 +++++
> >  ip/iproute.c         |  8 +++++++-
> >  lib/libnetlink.c     | 31 ++++++++++++++++++++++++++-----
> >  3 files changed, 38 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/libnetlink.h b/include/libnetlink.h
> > index b9073a6a..93c22a09 100644
> > --- a/include/libnetlink.h
> > +++ b/include/libnetlink.h
> > @@ -121,6 +121,11 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
> >  			void *arg, __u16 nc_flags);
> >  #define rtnl_dump_filter(rth, filter, arg) \
> >  	rtnl_dump_filter_nc(rth, filter, arg, 0)
> > +int rtnl_dump_filter_suppress_rtnl_errmsg_nc(struct rtnl_handle *rth,
> > +		     rtnl_filter_t filter,
> > +		     void *arg1, __u16 nc_flags, const int *errnos);
> > +#define rtnl_dump_filter_suppress_rtnl_errmsg(rth, filter, arg, errnos) \
> > +	rtnl_dump_filter_suppress_rtnl_errmsg_nc(rth, filter, arg, 0, errnos)
> >  int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
> >  	      struct nlmsghdr **answer)
> >  	__attribute__((warn_unused_result));
> > diff --git a/ip/iproute.c b/ip/iproute.c
> > index 5853f026..796d6d17 100644
> > --- a/ip/iproute.c
> > +++ b/ip/iproute.c
> > @@ -1734,6 +1734,8 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
> >  	char *od = NULL;
> >  	unsigned int mark = 0;
> >  	rtnl_filter_t filter_fn;
> > +	/* last 0 is array trailing */
> > +	int suppress_rtnl_errnos[2] = { 0, 0 };
> >  
> 
> The design would be clearer if there were two arguments rather than magic array of size 2.
> Also these are being used as boolean.
You mean replace extra (int *) to pair (int *, size_t size)?
For my case I need to filter only ENOENT error, but maybe for some other cases someone may want
to filter out ENOENT and ENOSUPP, for instance then and use { ENOENT, ENOSUPP, 0 } as array.


Regards,
Alex

[PATCHv3 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Alexander Mikhalitsyn]

We started to use in-kernel filtering feature which allows to get only needed
tables (see iproute_dump_filter()). From the kernel side it's implemented in
net/ipv4/fib_frontend.c (inet_dump_fib), net/ipv6/ip6_fib.c (inet6_dump_fib).
The problem here is that behaviour of "ip route save" was changed after
c7e6371bc ("ip route: Add protocol, table id and device to dump request").
If filters are used, then kernel returns ENOENT error if requested table is absent,
but in newly created net namespace even RT_TABLE_MAIN table doesn't exist.
It is really allocated, for instance, after issuing "ip l set lo up".

Reproducer is fairly simple:
$ unshare -n ip route save > dump
Error: ipv4: FIB table does not exist.
Dump terminated

Expected result here is to get empty dump file (as it was before this change).

v2: reworked, so, now it takes into account NLMSGERR_ATTR_MSG
(see nl_dump_ext_ack_done() function). We want to suppress error messages
in stderr about absent FIB table from kernel too.

v3: reworked to make code clearer. Introduced rtnl_suppressed_errors(),
rtnl_suppress_error() helpers. User may suppress up to 3 errors (may be
easily extened by changing SUPPRESS_ERRORS_INIT macro).

Fixes: c7e6371bc ("ip route: Add protocol, table id and device to dump request")
Cc: David Ahern <dsahern@gmail.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Andrei Vagin <avagin@gmail.com>
Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
Signed-off-by: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>
---
 include/libnetlink.h | 37 +++++++++++++++++++++++++++++++++++++
 ip/iproute.c         |  7 ++++++-
 lib/libnetlink.c     | 27 ++++++++++++++++++++++-----
 3 files changed, 65 insertions(+), 6 deletions(-)

diff --git a/include/libnetlink.h b/include/libnetlink.h
index b9073a6a..c41f714a 100644
--- a/include/libnetlink.h
+++ b/include/libnetlink.h
@@ -121,6 +121,43 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
 			void *arg, __u16 nc_flags);
 #define rtnl_dump_filter(rth, filter, arg) \
 	rtnl_dump_filter_nc(rth, filter, arg, 0)
+int rtnl_dump_filter_suppress_rtnl_errmsg_nc(struct rtnl_handle *rth,
+		     rtnl_filter_t filter,
+		     void *arg1, __u16 nc_flags, const int *errnos);
+#define rtnl_dump_filter_suppress_rtnl_errmsg(rth, filter, arg, errnos) \
+	rtnl_dump_filter_suppress_rtnl_errmsg_nc(rth, filter, arg, 0, errnos)
+#define SUPPRESS_ERRORS_INIT { 0, 0, 0, 0 }
+static inline int rtnl_suppressed_error(const int *errnos, int err_no)
+{
+	/* errnos is 0 terminated array or NULL */
+	while (errnos && *errnos) {
+		if (err_no == *errnos)
+			return 1;
+
+		errnos++;
+	}
+
+	return 0;
+}
+static inline void rtnl_suppress_error(int *errnos, int err_no)
+{
+	/* last 0 is trailing for errnos array */
+	int max = sizeof((int[])SUPPRESS_ERRORS_INIT) /
+			sizeof(int) - 1;
+
+	if (errnos == NULL)
+		return;
+
+	for (int i = 0; i < max; i++) {
+		if (errnos[i] == err_no)
+			break;
+
+		if (!errnos[i]) {
+			errnos[i] = err_no;
+			break;
+		}
+	}
+}
 int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 	      struct nlmsghdr **answer)
 	__attribute__((warn_unused_result));
diff --git a/ip/iproute.c b/ip/iproute.c
index 5853f026..532ca724 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -1734,6 +1734,7 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
 	char *od = NULL;
 	unsigned int mark = 0;
 	rtnl_filter_t filter_fn;
+	int suppress_rtnl_errnos[] = SUPPRESS_ERRORS_INIT;
 
 	if (action == IPROUTE_SAVE) {
 		if (save_route_prep())
@@ -1939,7 +1940,11 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
 
 	new_json_obj(json);
 
-	if (rtnl_dump_filter(&rth, filter_fn, stdout) < 0) {
+	if (filter.tb == RT_TABLE_MAIN)
+		rtnl_suppress_error(suppress_rtnl_errnos, ENOENT);
+
+	if (rtnl_dump_filter_suppress_rtnl_errmsg(&rth, filter_fn, stdout,
+						  suppress_rtnl_errnos) < 0) {
 		fprintf(stderr, "Dump terminated\n");
 		return -2;
 	}
diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index c958aa57..5c5a19bb 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -673,7 +673,7 @@ int rtnl_dump_request_n(struct rtnl_handle *rth, struct nlmsghdr *n)
 	return sendmsg(rth->fd, &msg, 0);
 }
 
-static int rtnl_dump_done(struct nlmsghdr *h)
+static int rtnl_dump_done(struct nlmsghdr *h, const int *errnos)
 {
 	int len = *(int *)NLMSG_DATA(h);
 
@@ -683,11 +683,15 @@ static int rtnl_dump_done(struct nlmsghdr *h)
 	}
 
 	if (len < 0) {
+		errno = -len;
+
+		if (rtnl_suppressed_error(errnos, errno))
+			return 0;
+
 		/* check for any messages returned from kernel */
 		if (nl_dump_ext_ack_done(h, len))
 			return len;
 
-		errno = -len;
 		switch (errno) {
 		case ENOENT:
 		case EOPNOTSUPP:
@@ -789,7 +793,8 @@ static int rtnl_recvmsg(int fd, struct msghdr *msg, char **answer)
 }
 
 static int rtnl_dump_filter_l(struct rtnl_handle *rth,
-			      const struct rtnl_dump_filter_arg *arg)
+			      const struct rtnl_dump_filter_arg *arg,
+			      const int *errnos)
 {
 	struct sockaddr_nl nladdr;
 	struct iovec iov;
@@ -834,7 +839,7 @@ static int rtnl_dump_filter_l(struct rtnl_handle *rth,
 					dump_intr = 1;
 
 				if (h->nlmsg_type == NLMSG_DONE) {
-					err = rtnl_dump_done(h);
+					err = rtnl_dump_done(h, errnos);
 					if (err < 0) {
 						free(buf);
 						return -1;
@@ -891,7 +896,19 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
 		{ .filter = NULL,   .arg1 = NULL, .nc_flags = 0, },
 	};
 
-	return rtnl_dump_filter_l(rth, a);
+	return rtnl_dump_filter_l(rth, a, NULL);
+}
+
+int rtnl_dump_filter_suppress_rtnl_errmsg_nc(struct rtnl_handle *rth,
+		     rtnl_filter_t filter,
+		     void *arg1, __u16 nc_flags, const int *errnos)
+{
+	const struct rtnl_dump_filter_arg a[2] = {
+		{ .filter = filter, .arg1 = arg1, .nc_flags = nc_flags, },
+		{ .filter = NULL,   .arg1 = NULL, .nc_flags = 0, },
+	};
+
+	return rtnl_dump_filter_l(rth, a, errnos);
 }
 
 static void rtnl_talk_error(struct nlmsghdr *h, struct nlmsgerr *err,
-- 
2.31.1

Re: [PATCHv2 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Alexander Mikhalitsyn]

On Thu, 24 Jun 2021 08:36:47 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

> On Thu, 24 Jun 2021 18:28:12 +0300
> Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:
> 
> > We started to use in-kernel filtering feature which allows to get only needed
> > tables (see iproute_dump_filter()). From the kernel side it's implemented in
> > net/ipv4/fib_frontend.c (inet_dump_fib), net/ipv6/ip6_fib.c (inet6_dump_fib).
> > The problem here is that behaviour of "ip route save" was changed after
> > c7e6371bc ("ip route: Add protocol, table id and device to dump request").
> > If filters are used, then kernel returns ENOENT error if requested table is absent,
> > but in newly created net namespace even RT_TABLE_MAIN table doesn't exist.
> > It is really allocated, for instance, after issuing "ip l set lo up".
> > 
> > Reproducer is fairly simple:
> > $ unshare -n ip route save > dump
> > Error: ipv4: FIB table does not exist.
> > Dump terminated
> > 
> > Expected result here is to get empty dump file (as it was before this change).
> > 
> > v2: reworked, so, now it takes into account NLMSGERR_ATTR_MSG
> > (see nl_dump_ext_ack_done() function). We want to suppress error messages
> > in stderr about absent FIB table from kernel too.
> > 
> > Fixes: c7e6371bc ("ip route: Add protocol, table id and device to dump request")
> > Cc: David Ahern <dsahern@gmail.com>
> > Cc: Stephen Hemminger <stephen@networkplumber.org>
> > Cc: Andrei Vagin <avagin@gmail.com>
> > Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
> > Signed-off-by: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>
> > ---
> >  include/libnetlink.h |  5 +++++
> >  ip/iproute.c         |  8 +++++++-
> >  lib/libnetlink.c     | 31 ++++++++++++++++++++++++++-----
> >  3 files changed, 38 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/libnetlink.h b/include/libnetlink.h
> > index b9073a6a..93c22a09 100644
> > --- a/include/libnetlink.h
> > +++ b/include/libnetlink.h
> > @@ -121,6 +121,11 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
> >  			void *arg, __u16 nc_flags);
> >  #define rtnl_dump_filter(rth, filter, arg) \
> >  	rtnl_dump_filter_nc(rth, filter, arg, 0)
> > +int rtnl_dump_filter_suppress_rtnl_errmsg_nc(struct rtnl_handle *rth,
> > +		     rtnl_filter_t filter,
> > +		     void *arg1, __u16 nc_flags, const int *errnos);
> > +#define rtnl_dump_filter_suppress_rtnl_errmsg(rth, filter, arg, errnos) \
> > +	rtnl_dump_filter_suppress_rtnl_errmsg_nc(rth, filter, arg, 0, errnos)
> >  int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
> >  	      struct nlmsghdr **answer)
> >  	__attribute__((warn_unused_result));
> > diff --git a/ip/iproute.c b/ip/iproute.c
> > index 5853f026..796d6d17 100644
> > --- a/ip/iproute.c
> > +++ b/ip/iproute.c
> > @@ -1734,6 +1734,8 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
> >  	char *od = NULL;
> >  	unsigned int mark = 0;
> >  	rtnl_filter_t filter_fn;
> > +	/* last 0 is array trailing */
> > +	int suppress_rtnl_errnos[2] = { 0, 0 };
> >  
> 
> The design would be clearer if there were two arguments rather than magic array of size 2.
> Also these are being used as boolean.
I've reworked that to make code cleaner and sent v3.
I decided to keep array on the stack but introduced initializer-macro SUPPRESS_ERRORS_INIT which
can be extended anytime (now it allows to suppress up to 3 errors).

In iplink_vrf.c rtnl_talk_suppress_rtnl_errmsg function is used, which allows to suppress all errors,
but sometime it may be helpful to suppress just a few errors. That's a reason why I didn't implemented
variant which allows to suppress only one errno (ENOENT) for my particular case.

Thanks,
Alex

Re: [PATCHv3 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Stephen Hemminger]

On Fri, 25 Jun 2021 13:44:40 +0300
Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:

> We started to use in-kernel filtering feature which allows to get only needed
> tables (see iproute_dump_filter()). From the kernel side it's implemented in
> net/ipv4/fib_frontend.c (inet_dump_fib), net/ipv6/ip6_fib.c (inet6_dump_fib).
> The problem here is that behaviour of "ip route save" was changed after
> c7e6371bc ("ip route: Add protocol, table id and device to dump request").
> If filters are used, then kernel returns ENOENT error if requested table is absent,
> but in newly created net namespace even RT_TABLE_MAIN table doesn't exist.
> It is really allocated, for instance, after issuing "ip l set lo up".
> 
> Reproducer is fairly simple:
> $ unshare -n ip route save > dump
> Error: ipv4: FIB table does not exist.
> Dump terminated
> 
> Expected result here is to get empty dump file (as it was before this change).
> 
> v2: reworked, so, now it takes into account NLMSGERR_ATTR_MSG
> (see nl_dump_ext_ack_done() function). We want to suppress error messages
> in stderr about absent FIB table from kernel too.
> 
> v3: reworked to make code clearer. Introduced rtnl_suppressed_errors(),
> rtnl_suppress_error() helpers. User may suppress up to 3 errors (may be
> easily extened by changing SUPPRESS_ERRORS_INIT macro).
> 
> Fixes: c7e6371bc ("ip route: Add protocol, table id and device to dump request")
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Cc: Andrei Vagin <avagin@gmail.com>
> Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
> Signed-off-by: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>
> ---
>  include/libnetlink.h | 37 +++++++++++++++++++++++++++++++++++++
>  ip/iproute.c         |  7 ++++++-
>  lib/libnetlink.c     | 27 ++++++++++++++++++++++-----
>  3 files changed, 65 insertions(+), 6 deletions(-)
> 
> diff --git a/include/libnetlink.h b/include/libnetlink.h
> index b9073a6a..c41f714a 100644
> --- a/include/libnetlink.h
> +++ b/include/libnetlink.h
> @@ -121,6 +121,43 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
>  			void *arg, __u16 nc_flags);
>  #define rtnl_dump_filter(rth, filter, arg) \
>  	rtnl_dump_filter_nc(rth, filter, arg, 0)
> +int rtnl_dump_filter_suppress_rtnl_errmsg_nc(struct rtnl_handle *rth,
> +		     rtnl_filter_t filter,
> +		     void *arg1, __u16 nc_flags, const int *errnos);
> +#define rtnl_dump_filter_suppress_rtnl_errmsg(rth, filter, arg, errnos) \
> +	rtnl_dump_filter_suppress_rtnl_errmsg_nc(rth, filter, arg, 0, errnos)

Sorry, this is getting really ugly.

It is almost as bad as looking at Windows source code with the extremely long
function names.

It would be better to refactor the already overload  rtnl_dump_filter into sub
components and different parts could use the sub functions as needed.

> +#define SUPPRESS_ERRORS_INIT { 0, 0, 0, 0 }

Why do you need a special macro just use {} in those places.


> +static inline int rtnl_suppressed_error(const int *errnos, int err_no)

Inline is unnecessary here.

> +{
> +	/* errnos is 0 terminated array or NULL */
> +	while (errnos && *errnos) {
> +		if (err_no == *errnos)
> +			return 1;
> +
> +		errnos++;
> +	}
> +
> +	return 0;
> +}
> +static inline void rtnl_suppress_error(int *errnos, int err_no)
Blank line between functions, Again no inline

> +{
> +	/* last 0 is trailing for errnos array */
> +	int max = sizeof((int[])SUPPRESS_ERRORS_INIT) /
> +			sizeof(int) - 1;
> +
> +	if (errnos == NULL)
> +		return;
> +
> +	for (int i = 0; i < max; i++) {
> +		if (errnos[i] == err_no)
> +			break;
> +
> +		if (!errnos[i]) {
> +			errnos[i] = err_no;
> +			break;
> +		}
> +	}
> +}
>  int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
>  	      struct nlmsghdr **answer)
>  	__attribute__((warn_unused_result));
> diff --git a/ip/iproute.c b/ip/iproute.c
> index 5853f026..532ca724 100644
> --- a/ip/iproute.c
> +++ b/ip/iproute.c
> @@ -1734,6 +1734,7 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
>  	char *od = NULL;
>  	unsigned int mark = 0;
>  	rtnl_filter_t filter_fn;
> +	int suppress_rtnl_errnos[] = SUPPRESS_ERRORS_INIT;
>  
>  	if (action == IPROUTE_SAVE) {
>  		if (save_route_prep())
> @@ -1939,7 +1940,11 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
>  
>  	new_json_obj(json);
>  
> -	if (rtnl_dump_filter(&rth, filter_fn, stdout) < 0) {
> +	if (filter.tb == RT_TABLE_MAIN)
> +		rtnl_suppress_error(suppress_rtnl_errnos, ENOENT);
> +
> +	if (rtnl_dump_filter_suppress_rtnl_errmsg(&rth, filter_fn, stdout,
> +						  suppress_rtnl_errnos) < 0) {
>  		fprintf(stderr, "Dump terminated\n");
>  		return -2;
>  	}
> diff --git a/lib/libnetlink.c b/lib/libnetlink.c
> index c958aa57..5c5a19bb 100644
> --- a/lib/libnetlink.c
> +++ b/lib/libnetlink.c
> @@ -673,7 +673,7 @@ int rtnl_dump_request_n(struct rtnl_handle *rth, struct nlmsghdr *n)
>  	return sendmsg(rth->fd, &msg, 0);
>  }
>  
> -static int rtnl_dump_done(struct nlmsghdr *h)
> +static int rtnl_dump_done(struct nlmsghdr *h, const int *errnos)
>  {
>  	int len = *(int *)NLMSG_DATA(h);
>  
> @@ -683,11 +683,15 @@ static int rtnl_dump_done(struct nlmsghdr *h)
>  	}
>  
>  	if (len < 0) {
> +		errno = -len;
> +
> +		if (rtnl_suppressed_error(errnos, errno))
> +			return 0;
> +
>  		/* check for any messages returned from kernel */
>  		if (nl_dump_ext_ack_done(h, len))
>  			return len;
>  
> -		errno = -len;
>  		switch (errno) {
>  		case ENOENT:
>  		case EOPNOTSUPP:
> @@ -789,7 +793,8 @@ static int rtnl_recvmsg(int fd, struct msghdr *msg, char **answer)
>  }
>  
>  static int rtnl_dump_filter_l(struct rtnl_handle *rth,
> -			      const struct rtnl_dump_filter_arg *arg)
> +			      const struct rtnl_dump_filter_arg *arg,
> +			      const int *errnos)
>  {
>  	struct sockaddr_nl nladdr;
>  	struct iovec iov;
> @@ -834,7 +839,7 @@ static int rtnl_dump_filter_l(struct rtnl_handle *rth,
>  					dump_intr = 1;
>  
>  				if (h->nlmsg_type == NLMSG_DONE) {
> -					err = rtnl_dump_done(h);
> +					err = rtnl_dump_done(h, errnos);
>  					if (err < 0) {
>  						free(buf);
>  						return -1;
> @@ -891,7 +896,19 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
>  		{ .filter = NULL,   .arg1 = NULL, .nc_flags = 0, },
>  	};
>  
> -	return rtnl_dump_filter_l(rth, a);
> +	return rtnl_dump_filter_l(rth, a, NULL);
> +}
> +
> +int rtnl_dump_filter_suppress_rtnl_errmsg_nc(struct rtnl_handle *rth,
> +		     rtnl_filter_t filter,
> +		     void *arg1, __u16 nc_flags, const int *errnos)
> +{
> +	const struct rtnl_dump_filter_arg a[2] = {
> +		{ .filter = filter, .arg1 = arg1, .nc_flags = nc_flags, },
> +		{ .filter = NULL,   .arg1 = NULL, .nc_flags = 0, },
> +	};
> +
> +	return rtnl_dump_filter_l(rth, a, errnos);
>  }
>  
>  static void rtnl_talk_error(struct nlmsghdr *h, struct nlmsgerr *err,

Re: [PATCHv3 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Alexander Mikhalitsyn]

On Sun, 27 Jun 2021 14:54:24 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

> On Fri, 25 Jun 2021 13:44:40 +0300
> Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:
> 
> > We started to use in-kernel filtering feature which allows to get only needed
> > tables (see iproute_dump_filter()). From the kernel side it's implemented in
> > net/ipv4/fib_frontend.c (inet_dump_fib), net/ipv6/ip6_fib.c (inet6_dump_fib).
> > The problem here is that behaviour of "ip route save" was changed after
> > c7e6371bc ("ip route: Add protocol, table id and device to dump request").
> > If filters are used, then kernel returns ENOENT error if requested table is absent,
> > but in newly created net namespace even RT_TABLE_MAIN table doesn't exist.
> > It is really allocated, for instance, after issuing "ip l set lo up".
> > 
> > Reproducer is fairly simple:
> > $ unshare -n ip route save > dump
> > Error: ipv4: FIB table does not exist.
> > Dump terminated
> > 
> > Expected result here is to get empty dump file (as it was before this change).
> > 
> > v2: reworked, so, now it takes into account NLMSGERR_ATTR_MSG
> > (see nl_dump_ext_ack_done() function). We want to suppress error messages
> > in stderr about absent FIB table from kernel too.
> > 
> > v3: reworked to make code clearer. Introduced rtnl_suppressed_errors(),
> > rtnl_suppress_error() helpers. User may suppress up to 3 errors (may be
> > easily extened by changing SUPPRESS_ERRORS_INIT macro).
> > 
> > Fixes: c7e6371bc ("ip route: Add protocol, table id and device to dump request")
> > Cc: David Ahern <dsahern@gmail.com>
> > Cc: Stephen Hemminger <stephen@networkplumber.org>
> > Cc: Andrei Vagin <avagin@gmail.com>
> > Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
> > Signed-off-by: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>
> > ---
> >  include/libnetlink.h | 37 +++++++++++++++++++++++++++++++++++++
> >  ip/iproute.c         |  7 ++++++-
> >  lib/libnetlink.c     | 27 ++++++++++++++++++++++-----
> >  3 files changed, 65 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/libnetlink.h b/include/libnetlink.h
> > index b9073a6a..c41f714a 100644
> > --- a/include/libnetlink.h
> > +++ b/include/libnetlink.h
> > @@ -121,6 +121,43 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
> >  			void *arg, __u16 nc_flags);
> >  #define rtnl_dump_filter(rth, filter, arg) \
> >  	rtnl_dump_filter_nc(rth, filter, arg, 0)
> > +int rtnl_dump_filter_suppress_rtnl_errmsg_nc(struct rtnl_handle *rth,
> > +		     rtnl_filter_t filter,
> > +		     void *arg1, __u16 nc_flags, const int *errnos);
> > +#define rtnl_dump_filter_suppress_rtnl_errmsg(rth, filter, arg, errnos) \
> > +	rtnl_dump_filter_suppress_rtnl_errmsg_nc(rth, filter, arg, 0, errnos)
> 
> Sorry, this is getting really ugly.

Sorry, I apparently overdid it in refactoring ;)

> 
> It is almost as bad as looking at Windows source code with the extremely long
> function names.

Sure, I will choose shorter names.

> 
> It would be better to refactor the already overload  rtnl_dump_filter into sub
> components and different parts could use the sub functions as needed.
> 
> > +#define SUPPRESS_ERRORS_INIT { 0, 0, 0, 0 }
> 
> Why do you need a special macro just use {} in those places.

It seems like I wrongly interpreted you words about "magic array of size 2"
>>The design would be clearer if there were two arguments rather than magic array of size 2.
If I understand you correctly, you wanted to say that it's not fully convenient to read and
see "magic" initializator { 0, 0, 0, 0 } or similar. (why four zeroes? why not 3 or 1?)
So, I've decided to make macro for this initializer like we have macros for linked lists
initializators in kernel, for instance. Another reason is that If someone need to skip more than
3 errors, he can change initializer size and helper function rtnl_suppress_error() will
take new size into account.

What I want to implement:
1. Possibility to skip several errors in rtnl_dump_filter
2. Not use malloc for allocation of "errors" array (because array is really small and
malloc needs free, so it's easier to make errors with memleak in the future)
3. I'm trying not to change original function rtnl_dump_filter() signature because
it's used in other places and that's a stable API.
4. We want to allow programmer to dynamically add skipped errors. It means that
user not specifying array of skipped errors directly like { ENOENT, ENOSUPP, 0 }, but
can write something like:
if (ignore_old_kernel_errors)
    rtnl_suppress_error(errors, ENOSUPP)
if (some_another_reason)
    rtnl_suppress_error(errors, ENOENT)


Maybe some of my points not valid for us and I can throw it?

Thank you for review! ;)

Alex.

> 
> 
> > +static inline int rtnl_suppressed_error(const int *errnos, int err_no)
> 
> Inline is unnecessary here.
> 
> > +{
> > +	/* errnos is 0 terminated array or NULL */
> > +	while (errnos && *errnos) {
> > +		if (err_no == *errnos)
> > +			return 1;
> > +
> > +		errnos++;
> > +	}
> > +
> > +	return 0;
> > +}
> > +static inline void rtnl_suppress_error(int *errnos, int err_no)
> Blank line between functions, Again no inline
> 
> > +{
> > +	/* last 0 is trailing for errnos array */
> > +	int max = sizeof((int[])SUPPRESS_ERRORS_INIT) /
> > +			sizeof(int) - 1;
> > +
> > +	if (errnos == NULL)
> > +		return;
> > +
> > +	for (int i = 0; i < max; i++) {
> > +		if (errnos[i] == err_no)
> > +			break;
> > +
> > +		if (!errnos[i]) {
> > +			errnos[i] = err_no;
> > +			break;
> > +		}
> > +	}
> > +}
> >  int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
> >  	      struct nlmsghdr **answer)
> >  	__attribute__((warn_unused_result));
> > diff --git a/ip/iproute.c b/ip/iproute.c
> > index 5853f026..532ca724 100644
> > --- a/ip/iproute.c
> > +++ b/ip/iproute.c
> > @@ -1734,6 +1734,7 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
> >  	char *od = NULL;
> >  	unsigned int mark = 0;
> >  	rtnl_filter_t filter_fn;
> > +	int suppress_rtnl_errnos[] = SUPPRESS_ERRORS_INIT;
> >  
> >  	if (action == IPROUTE_SAVE) {
> >  		if (save_route_prep())
> > @@ -1939,7 +1940,11 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
> >  
> >  	new_json_obj(json);
> >  
> > -	if (rtnl_dump_filter(&rth, filter_fn, stdout) < 0) {
> > +	if (filter.tb == RT_TABLE_MAIN)
> > +		rtnl_suppress_error(suppress_rtnl_errnos, ENOENT);
> > +
> > +	if (rtnl_dump_filter_suppress_rtnl_errmsg(&rth, filter_fn, stdout,
> > +						  suppress_rtnl_errnos) < 0) {
> >  		fprintf(stderr, "Dump terminated\n");
> >  		return -2;
> >  	}
> > diff --git a/lib/libnetlink.c b/lib/libnetlink.c
> > index c958aa57..5c5a19bb 100644
> > --- a/lib/libnetlink.c
> > +++ b/lib/libnetlink.c
> > @@ -673,7 +673,7 @@ int rtnl_dump_request_n(struct rtnl_handle *rth, struct nlmsghdr *n)
> >  	return sendmsg(rth->fd, &msg, 0);
> >  }
> >  
> > -static int rtnl_dump_done(struct nlmsghdr *h)
> > +static int rtnl_dump_done(struct nlmsghdr *h, const int *errnos)
> >  {
> >  	int len = *(int *)NLMSG_DATA(h);
> >  
> > @@ -683,11 +683,15 @@ static int rtnl_dump_done(struct nlmsghdr *h)
> >  	}
> >  
> >  	if (len < 0) {
> > +		errno = -len;
> > +
> > +		if (rtnl_suppressed_error(errnos, errno))
> > +			return 0;
> > +
> >  		/* check for any messages returned from kernel */
> >  		if (nl_dump_ext_ack_done(h, len))
> >  			return len;
> >  
> > -		errno = -len;
> >  		switch (errno) {
> >  		case ENOENT:
> >  		case EOPNOTSUPP:
> > @@ -789,7 +793,8 @@ static int rtnl_recvmsg(int fd, struct msghdr *msg, char **answer)
> >  }
> >  
> >  static int rtnl_dump_filter_l(struct rtnl_handle *rth,
> > -			      const struct rtnl_dump_filter_arg *arg)
> > +			      const struct rtnl_dump_filter_arg *arg,
> > +			      const int *errnos)
> >  {
> >  	struct sockaddr_nl nladdr;
> >  	struct iovec iov;
> > @@ -834,7 +839,7 @@ static int rtnl_dump_filter_l(struct rtnl_handle *rth,
> >  					dump_intr = 1;
> >  
> >  				if (h->nlmsg_type == NLMSG_DONE) {
> > -					err = rtnl_dump_done(h);
> > +					err = rtnl_dump_done(h, errnos);
> >  					if (err < 0) {
> >  						free(buf);
> >  						return -1;
> > @@ -891,7 +896,19 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
> >  		{ .filter = NULL,   .arg1 = NULL, .nc_flags = 0, },
> >  	};
> >  
> > -	return rtnl_dump_filter_l(rth, a);
> > +	return rtnl_dump_filter_l(rth, a, NULL);
> > +}
> > +
> > +int rtnl_dump_filter_suppress_rtnl_errmsg_nc(struct rtnl_handle *rth,
> > +		     rtnl_filter_t filter,
> > +		     void *arg1, __u16 nc_flags, const int *errnos)
> > +{
> > +	const struct rtnl_dump_filter_arg a[2] = {
> > +		{ .filter = filter, .arg1 = arg1, .nc_flags = nc_flags, },
> > +		{ .filter = NULL,   .arg1 = NULL, .nc_flags = 0, },
> > +	};
> > +
> > +	return rtnl_dump_filter_l(rth, a, errnos);
> >  }
> >  
> >  static void rtnl_talk_error(struct nlmsghdr *h, struct nlmsgerr *err,
> 

Re: [PATCHv3 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Stephen Hemminger]

On Mon, 28 Jun 2021 09:31:32 +0300
Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:

> On Sun, 27 Jun 2021 14:54:24 -0700
> Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> > On Fri, 25 Jun 2021 13:44:40 +0300
> > Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:
> >   
> > > We started to use in-kernel filtering feature which allows to get only needed
> > > tables (see iproute_dump_filter()). From the kernel side it's implemented in
> > > net/ipv4/fib_frontend.c (inet_dump_fib), net/ipv6/ip6_fib.c (inet6_dump_fib).
> > > The problem here is that behaviour of "ip route save" was changed after
> > > c7e6371bc ("ip route: Add protocol, table id and device to dump request").
> > > If filters are used, then kernel returns ENOENT error if requested table is absent,
> > > but in newly created net namespace even RT_TABLE_MAIN table doesn't exist.
> > > It is really allocated, for instance, after issuing "ip l set lo up".
> > > 
> > > Reproducer is fairly simple:
> > > $ unshare -n ip route save > dump
> > > Error: ipv4: FIB table does not exist.
> > > Dump terminated
> > > 
> > > Expected result here is to get empty dump file (as it was before this change).
> > > 
> > > v2: reworked, so, now it takes into account NLMSGERR_ATTR_MSG
> > > (see nl_dump_ext_ack_done() function). We want to suppress error messages
> > > in stderr about absent FIB table from kernel too.
> > > 
> > > v3: reworked to make code clearer. Introduced rtnl_suppressed_errors(),
> > > rtnl_suppress_error() helpers. User may suppress up to 3 errors (may be
> > > easily extened by changing SUPPRESS_ERRORS_INIT macro).
> > > 
> > > Fixes: c7e6371bc ("ip route: Add protocol, table id and device to dump request")
> > > Cc: David Ahern <dsahern@gmail.com>
> > > Cc: Stephen Hemminger <stephen@networkplumber.org>
> > > Cc: Andrei Vagin <avagin@gmail.com>
> > > Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
> > > Signed-off-by: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>
> > > ---
> > >  include/libnetlink.h | 37 +++++++++++++++++++++++++++++++++++++
> > >  ip/iproute.c         |  7 ++++++-
> > >  lib/libnetlink.c     | 27 ++++++++++++++++++++++-----
> > >  3 files changed, 65 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/include/libnetlink.h b/include/libnetlink.h
> > > index b9073a6a..c41f714a 100644
> > > --- a/include/libnetlink.h
> > > +++ b/include/libnetlink.h
> > > @@ -121,6 +121,43 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
> > >  			void *arg, __u16 nc_flags);
> > >  #define rtnl_dump_filter(rth, filter, arg) \
> > >  	rtnl_dump_filter_nc(rth, filter, arg, 0)
> > > +int rtnl_dump_filter_suppress_rtnl_errmsg_nc(struct rtnl_handle *rth,
> > > +		     rtnl_filter_t filter,
> > > +		     void *arg1, __u16 nc_flags, const int *errnos);
> > > +#define rtnl_dump_filter_suppress_rtnl_errmsg(rth, filter, arg, errnos) \
> > > +	rtnl_dump_filter_suppress_rtnl_errmsg_nc(rth, filter, arg, 0, errnos)  
> > 
> > Sorry, this is getting really ugly.  
> 
> Sorry, I apparently overdid it in refactoring ;)
> 
> > 
> > It is almost as bad as looking at Windows source code with the extremely long
> > function names.  
> 
> Sure, I will choose shorter names.
> 
> > 
> > It would be better to refactor the already overload  rtnl_dump_filter into sub
> > components and different parts could use the sub functions as needed.
> >   
> > > +#define SUPPRESS_ERRORS_INIT { 0, 0, 0, 0 }  
> > 
> > Why do you need a special macro just use {} in those places.  
> 
> It seems like I wrongly interpreted you words about "magic array of size 2"
> >>The design would be clearer if there were two arguments rather than magic array of size 2.  
> If I understand you correctly, you wanted to say that it's not fully convenient to read and
> see "magic" initializator { 0, 0, 0, 0 } or similar. (why four zeroes? why not 3 or 1?)
> So, I've decided to make macro for this initializer like we have macros for linked lists
> initializators in kernel, for instance. Another reason is that If someone need to skip more than
> 3 errors, he can change initializer size and helper function rtnl_suppress_error() will
> take new size into account.
> 
> What I want to implement:
> 1. Possibility to skip several errors in rtnl_dump_filter
> 2. Not use malloc for allocation of "errors" array (because array is really small and
> malloc needs free, so it's easier to make errors with memleak in the future)
> 3. I'm trying not to change original function rtnl_dump_filter() signature because
> it's used in other places and that's a stable API.
> 4. We want to allow programmer to dynamically add skipped errors. It means that
> user not specifying array of skipped errors directly like { ENOENT, ENOSUPP, 0 }, but
> can write something like:
> if (ignore_old_kernel_errors)
>     rtnl_suppress_error(errors, ENOSUPP)
> if (some_another_reason)
>     rtnl_suppress_error(errors, ENOENT)
> 
> 
> Maybe some of my points not valid for us and I can throw it?
> 
> Thank you for review! ;)
> 
> Alex.
> 
> > 
> >   
> > > +static inline int rtnl_suppressed_error(const int *errnos, int err_no)  
> > 
> > Inline is unnecessary here.
> >   
> > > +{
> > > +	/* errnos is 0 terminated array or NULL */
> > > +	while (errnos && *errnos) {
> > > +		if (err_no == *errnos)
> > > +			return 1;
> > > +
> > > +		errnos++;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +static inline void rtnl_suppress_error(int *errnos, int err_no)  
> > Blank line between functions, Again no inline
> >   
> > > +{
> > > +	/* last 0 is trailing for errnos array */
> > > +	int max = sizeof((int[])SUPPRESS_ERRORS_INIT) /
> > > +			sizeof(int) - 1;
> > > +
> > > +	if (errnos == NULL)
> > > +		return;
> > > +
> > > +	for (int i = 0; i < max; i++) {
> > > +		if (errnos[i] == err_no)
> > > +			break;
> > > +
> > > +		if (!errnos[i]) {
> > > +			errnos[i] = err_no;
> > > +			break;
> > > +		}
> > > +	}
> > > +}
> > >  int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
> > >  	      struct nlmsghdr **answer)
> > >  	__attribute__((warn_unused_result));
> > > diff --git a/ip/iproute.c b/ip/iproute.c
> > > index 5853f026..532ca724 100644
> > > --- a/ip/iproute.c
> > > +++ b/ip/iproute.c
> > > @@ -1734,6 +1734,7 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
> > >  	char *od = NULL;
> > >  	unsigned int mark = 0;
> > >  	rtnl_filter_t filter_fn;
> > > +	int suppress_rtnl_errnos[] = SUPPRESS_ERRORS_INIT;
> > >  
> > >  	if (action == IPROUTE_SAVE) {
> > >  		if (save_route_prep())
> > > @@ -1939,7 +1940,11 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
> > >  
> > >  	new_json_obj(json);
> > >  
> > > -	if (rtnl_dump_filter(&rth, filter_fn, stdout) < 0) {
> > > +	if (filter.tb == RT_TABLE_MAIN)
> > > +		rtnl_suppress_error(suppress_rtnl_errnos, ENOENT);
> > > +
> > > +	if (rtnl_dump_filter_suppress_rtnl_errmsg(&rth, filter_fn, stdout,
> > > +						  suppress_rtnl_errnos) < 0) {
> > >  		fprintf(stderr, "Dump terminated\n");
> > >  		return -2;
> > >  	}
> > > diff --git a/lib/libnetlink.c b/lib/libnetlink.c
> > > index c958aa57..5c5a19bb 100644
> > > --- a/lib/libnetlink.c
> > > +++ b/lib/libnetlink.c
> > > @@ -673,7 +673,7 @@ int rtnl_dump_request_n(struct rtnl_handle *rth, struct nlmsghdr *n)
> > >  	return sendmsg(rth->fd, &msg, 0);
> > >  }
> > >  
> > > -static int rtnl_dump_done(struct nlmsghdr *h)
> > > +static int rtnl_dump_done(struct nlmsghdr *h, const int *errnos)
> > >  {
> > >  	int len = *(int *)NLMSG_DATA(h);
> > >  
> > > @@ -683,11 +683,15 @@ static int rtnl_dump_done(struct nlmsghdr *h)
> > >  	}
> > >  
> > >  	if (len < 0) {
> > > +		errno = -len;
> > > +
> > > +		if (rtnl_suppressed_error(errnos, errno))
> > > +			return 0;
> > > +
> > >  		/* check for any messages returned from kernel */
> > >  		if (nl_dump_ext_ack_done(h, len))
> > >  			return len;
> > >  
> > > -		errno = -len;
> > >  		switch (errno) {
> > >  		case ENOENT:
> > >  		case EOPNOTSUPP:
> > > @@ -789,7 +793,8 @@ static int rtnl_recvmsg(int fd, struct msghdr *msg, char **answer)
> > >  }
> > >  
> > >  static int rtnl_dump_filter_l(struct rtnl_handle *rth,
> > > -			      const struct rtnl_dump_filter_arg *arg)
> > > +			      const struct rtnl_dump_filter_arg *arg,
> > > +			      const int *errnos)
> > >  {
> > >  	struct sockaddr_nl nladdr;
> > >  	struct iovec iov;
> > > @@ -834,7 +839,7 @@ static int rtnl_dump_filter_l(struct rtnl_handle *rth,
> > >  					dump_intr = 1;
> > >  
> > >  				if (h->nlmsg_type == NLMSG_DONE) {
> > > -					err = rtnl_dump_done(h);
> > > +					err = rtnl_dump_done(h, errnos);
> > >  					if (err < 0) {
> > >  						free(buf);
> > >  						return -1;
> > > @@ -891,7 +896,19 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
> > >  		{ .filter = NULL,   .arg1 = NULL, .nc_flags = 0, },
> > >  	};
> > >  
> > > -	return rtnl_dump_filter_l(rth, a);
> > > +	return rtnl_dump_filter_l(rth, a, NULL);
> > > +}
> > > +
> > > +int rtnl_dump_filter_suppress_rtnl_errmsg_nc(struct rtnl_handle *rth,
> > > +		     rtnl_filter_t filter,
> > > +		     void *arg1, __u16 nc_flags, const int *errnos)
> > > +{
> > > +	const struct rtnl_dump_filter_arg a[2] = {
> > > +		{ .filter = filter, .arg1 = arg1, .nc_flags = nc_flags, },
> > > +		{ .filter = NULL,   .arg1 = NULL, .nc_flags = 0, },
> > > +	};
> > > +
> > > +	return rtnl_dump_filter_l(rth, a, errnos);
> > >  }
> > >  
> > >  static void rtnl_talk_error(struct nlmsghdr *h, struct nlmsgerr *err,  
> >   

Maybe adding an error handler callback to existing dump filter routine.
And add a rtnl_dump_error() function as the default error handler. (ie if NULL is passed)
That way your callback could skip what ever errors it wants and call the rtnl_dump_error
routine for the ones it wants to handle.

Re: [PATCHv3 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Alexander Mikhalitsyn]

On Mon, 28 Jun 2021 10:17:45 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

> On Mon, 28 Jun 2021 09:31:32 +0300
> Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:
> 
> > On Sun, 27 Jun 2021 14:54:24 -0700
> > Stephen Hemminger <stephen@networkplumber.org> wrote:
> > 
> > > On Fri, 25 Jun 2021 13:44:40 +0300
> > > Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:
> > >   
> > > > We started to use in-kernel filtering feature which allows to get only needed
> > > > tables (see iproute_dump_filter()). From the kernel side it's implemented in
> > > > net/ipv4/fib_frontend.c (inet_dump_fib), net/ipv6/ip6_fib.c (inet6_dump_fib).
> > > > The problem here is that behaviour of "ip route save" was changed after
> > > > c7e6371bc ("ip route: Add protocol, table id and device to dump request").
> > > > If filters are used, then kernel returns ENOENT error if requested table is absent,
> > > > but in newly created net namespace even RT_TABLE_MAIN table doesn't exist.
> > > > It is really allocated, for instance, after issuing "ip l set lo up".
> > > > 
> > > > Reproducer is fairly simple:
> > > > $ unshare -n ip route save > dump
> > > > Error: ipv4: FIB table does not exist.
> > > > Dump terminated
> > > > 
> > > > Expected result here is to get empty dump file (as it was before this change).
> > > > 
> > > > v2: reworked, so, now it takes into account NLMSGERR_ATTR_MSG
> > > > (see nl_dump_ext_ack_done() function). We want to suppress error messages
> > > > in stderr about absent FIB table from kernel too.
> > > > 
> > > > v3: reworked to make code clearer. Introduced rtnl_suppressed_errors(),
> > > > rtnl_suppress_error() helpers. User may suppress up to 3 errors (may be
> > > > easily extened by changing SUPPRESS_ERRORS_INIT macro).
> > > > 
> > > > Fixes: c7e6371bc ("ip route: Add protocol, table id and device to dump request")
> > > > Cc: David Ahern <dsahern@gmail.com>
> > > > Cc: Stephen Hemminger <stephen@networkplumber.org>
> > > > Cc: Andrei Vagin <avagin@gmail.com>
> > > > Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
> > > > Signed-off-by: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>
> > > > ---
> > > >  include/libnetlink.h | 37 +++++++++++++++++++++++++++++++++++++
> > > >  ip/iproute.c         |  7 ++++++-
> > > >  lib/libnetlink.c     | 27 ++++++++++++++++++++++-----
> > > >  3 files changed, 65 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/include/libnetlink.h b/include/libnetlink.h
> > > > index b9073a6a..c41f714a 100644
> > > > --- a/include/libnetlink.h
> > > > +++ b/include/libnetlink.h
> > > > @@ -121,6 +121,43 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
> > > >  			void *arg, __u16 nc_flags);
> > > >  #define rtnl_dump_filter(rth, filter, arg) \
> > > >  	rtnl_dump_filter_nc(rth, filter, arg, 0)
> > > > +int rtnl_dump_filter_suppress_rtnl_errmsg_nc(struct rtnl_handle *rth,
> > > > +		     rtnl_filter_t filter,
> > > > +		     void *arg1, __u16 nc_flags, const int *errnos);
> > > > +#define rtnl_dump_filter_suppress_rtnl_errmsg(rth, filter, arg, errnos) \
> > > > +	rtnl_dump_filter_suppress_rtnl_errmsg_nc(rth, filter, arg, 0, errnos)  
> > > 
> > > Sorry, this is getting really ugly.  
> > 
> > Sorry, I apparently overdid it in refactoring ;)
> > 
> > > 
> > > It is almost as bad as looking at Windows source code with the extremely long
> > > function names.  
> > 
> > Sure, I will choose shorter names.
> > 
> > > 
> > > It would be better to refactor the already overload  rtnl_dump_filter into sub
> > > components and different parts could use the sub functions as needed.
> > >   
> > > > +#define SUPPRESS_ERRORS_INIT { 0, 0, 0, 0 }  
> > > 
> > > Why do you need a special macro just use {} in those places.  
> > 
> > It seems like I wrongly interpreted you words about "magic array of size 2"
> > >>The design would be clearer if there were two arguments rather than magic array of size 2.  
> > If I understand you correctly, you wanted to say that it's not fully convenient to read and
> > see "magic" initializator { 0, 0, 0, 0 } or similar. (why four zeroes? why not 3 or 1?)
> > So, I've decided to make macro for this initializer like we have macros for linked lists
> > initializators in kernel, for instance. Another reason is that If someone need to skip more than
> > 3 errors, he can change initializer size and helper function rtnl_suppress_error() will
> > take new size into account.
> > 
> > What I want to implement:
> > 1. Possibility to skip several errors in rtnl_dump_filter
> > 2. Not use malloc for allocation of "errors" array (because array is really small and
> > malloc needs free, so it's easier to make errors with memleak in the future)
> > 3. I'm trying not to change original function rtnl_dump_filter() signature because
> > it's used in other places and that's a stable API.
> > 4. We want to allow programmer to dynamically add skipped errors. It means that
> > user not specifying array of skipped errors directly like { ENOENT, ENOSUPP, 0 }, but
> > can write something like:
> > if (ignore_old_kernel_errors)
> >     rtnl_suppress_error(errors, ENOSUPP)
> > if (some_another_reason)
> >     rtnl_suppress_error(errors, ENOENT)
> > 
> > 
> > Maybe some of my points not valid for us and I can throw it?
> > 
> > Thank you for review! ;)
> > 
> > Alex.
> > 
> > > 
> > >   
> > > > +static inline int rtnl_suppressed_error(const int *errnos, int err_no)  
> > > 
> > > Inline is unnecessary here.
> > >   
> > > > +{
> > > > +	/* errnos is 0 terminated array or NULL */
> > > > +	while (errnos && *errnos) {
> > > > +		if (err_no == *errnos)
> > > > +			return 1;
> > > > +
> > > > +		errnos++;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +static inline void rtnl_suppress_error(int *errnos, int err_no)  
> > > Blank line between functions, Again no inline
> > >   
> > > > +{
> > > > +	/* last 0 is trailing for errnos array */
> > > > +	int max = sizeof((int[])SUPPRESS_ERRORS_INIT) /
> > > > +			sizeof(int) - 1;
> > > > +
> > > > +	if (errnos == NULL)
> > > > +		return;
> > > > +
> > > > +	for (int i = 0; i < max; i++) {
> > > > +		if (errnos[i] == err_no)
> > > > +			break;
> > > > +
> > > > +		if (!errnos[i]) {
> > > > +			errnos[i] = err_no;
> > > > +			break;
> > > > +		}
> > > > +	}
> > > > +}
> > > >  int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
> > > >  	      struct nlmsghdr **answer)
> > > >  	__attribute__((warn_unused_result));
> > > > diff --git a/ip/iproute.c b/ip/iproute.c
> > > > index 5853f026..532ca724 100644
> > > > --- a/ip/iproute.c
> > > > +++ b/ip/iproute.c
> > > > @@ -1734,6 +1734,7 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
> > > >  	char *od = NULL;
> > > >  	unsigned int mark = 0;
> > > >  	rtnl_filter_t filter_fn;
> > > > +	int suppress_rtnl_errnos[] = SUPPRESS_ERRORS_INIT;
> > > >  
> > > >  	if (action == IPROUTE_SAVE) {
> > > >  		if (save_route_prep())
> > > > @@ -1939,7 +1940,11 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
> > > >  
> > > >  	new_json_obj(json);
> > > >  
> > > > -	if (rtnl_dump_filter(&rth, filter_fn, stdout) < 0) {
> > > > +	if (filter.tb == RT_TABLE_MAIN)
> > > > +		rtnl_suppress_error(suppress_rtnl_errnos, ENOENT);
> > > > +
> > > > +	if (rtnl_dump_filter_suppress_rtnl_errmsg(&rth, filter_fn, stdout,
> > > > +						  suppress_rtnl_errnos) < 0) {
> > > >  		fprintf(stderr, "Dump terminated\n");
> > > >  		return -2;
> > > >  	}
> > > > diff --git a/lib/libnetlink.c b/lib/libnetlink.c
> > > > index c958aa57..5c5a19bb 100644
> > > > --- a/lib/libnetlink.c
> > > > +++ b/lib/libnetlink.c
> > > > @@ -673,7 +673,7 @@ int rtnl_dump_request_n(struct rtnl_handle *rth, struct nlmsghdr *n)
> > > >  	return sendmsg(rth->fd, &msg, 0);
> > > >  }
> > > >  
> > > > -static int rtnl_dump_done(struct nlmsghdr *h)
> > > > +static int rtnl_dump_done(struct nlmsghdr *h, const int *errnos)
> > > >  {
> > > >  	int len = *(int *)NLMSG_DATA(h);
> > > >  
> > > > @@ -683,11 +683,15 @@ static int rtnl_dump_done(struct nlmsghdr *h)
> > > >  	}
> > > >  
> > > >  	if (len < 0) {
> > > > +		errno = -len;
> > > > +
> > > > +		if (rtnl_suppressed_error(errnos, errno))
> > > > +			return 0;
> > > > +
> > > >  		/* check for any messages returned from kernel */
> > > >  		if (nl_dump_ext_ack_done(h, len))
> > > >  			return len;
> > > >  
> > > > -		errno = -len;
> > > >  		switch (errno) {
> > > >  		case ENOENT:
> > > >  		case EOPNOTSUPP:
> > > > @@ -789,7 +793,8 @@ static int rtnl_recvmsg(int fd, struct msghdr *msg, char **answer)
> > > >  }
> > > >  
> > > >  static int rtnl_dump_filter_l(struct rtnl_handle *rth,
> > > > -			      const struct rtnl_dump_filter_arg *arg)
> > > > +			      const struct rtnl_dump_filter_arg *arg,
> > > > +			      const int *errnos)
> > > >  {
> > > >  	struct sockaddr_nl nladdr;
> > > >  	struct iovec iov;
> > > > @@ -834,7 +839,7 @@ static int rtnl_dump_filter_l(struct rtnl_handle *rth,
> > > >  					dump_intr = 1;
> > > >  
> > > >  				if (h->nlmsg_type == NLMSG_DONE) {
> > > > -					err = rtnl_dump_done(h);
> > > > +					err = rtnl_dump_done(h, errnos);
> > > >  					if (err < 0) {
> > > >  						free(buf);
> > > >  						return -1;
> > > > @@ -891,7 +896,19 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
> > > >  		{ .filter = NULL,   .arg1 = NULL, .nc_flags = 0, },
> > > >  	};
> > > >  
> > > > -	return rtnl_dump_filter_l(rth, a);
> > > > +	return rtnl_dump_filter_l(rth, a, NULL);
> > > > +}
> > > > +
> > > > +int rtnl_dump_filter_suppress_rtnl_errmsg_nc(struct rtnl_handle *rth,
> > > > +		     rtnl_filter_t filter,
> > > > +		     void *arg1, __u16 nc_flags, const int *errnos)
> > > > +{
> > > > +	const struct rtnl_dump_filter_arg a[2] = {
> > > > +		{ .filter = filter, .arg1 = arg1, .nc_flags = nc_flags, },
> > > > +		{ .filter = NULL,   .arg1 = NULL, .nc_flags = 0, },
> > > > +	};
> > > > +
> > > > +	return rtnl_dump_filter_l(rth, a, errnos);
> > > >  }
> > > >  
> > > >  static void rtnl_talk_error(struct nlmsghdr *h, struct nlmsgerr *err,  
> > >   
> 
> Maybe adding an error handler callback to existing dump filter routine.
> And add a rtnl_dump_error() function as the default error handler. (ie if NULL is passed)
> That way your callback could skip what ever errors it wants and call the rtnl_dump_error
> routine for the ones it wants to handle.
> 

Great. I'll implement that! ;)

Thanks,
Alex

[PATCHv4 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Alexander Mikhalitsyn]

We started to use in-kernel filtering feature which allows to get only needed
tables (see iproute_dump_filter()). From the kernel side it's implemented in
net/ipv4/fib_frontend.c (inet_dump_fib), net/ipv6/ip6_fib.c (inet6_dump_fib).
The problem here is that behaviour of "ip route save" was changed after
c7e6371bc ("ip route: Add protocol, table id and device to dump request").
If filters are used, then kernel returns ENOENT error if requested table is absent,
but in newly created net namespace even RT_TABLE_MAIN table doesn't exist.
It is really allocated, for instance, after issuing "ip l set lo up".

Reproducer is fairly simple:
$ unshare -n ip route save > dump
Error: ipv4: FIB table does not exist.
Dump terminated

Expected result here is to get empty dump file (as it was before this change).

v2: reworked, so, now it takes into account NLMSGERR_ATTR_MSG
(see nl_dump_ext_ack_done() function). We want to suppress error messages
in stderr about absent FIB table from kernel too.

v3: reworked to make code clearer. Introduced rtnl_suppressed_errors(),
rtnl_suppress_error() helpers. User may suppress up to 3 errors (may be
easily extened by changing SUPPRESS_ERRORS_INIT macro).

v4: reworked, rtnl_dump_filter_errhndlr() was introduced. Thanks
to Stephen Hemminger for comments and suggestions

Fixes: c7e6371bc ("ip route: Add protocol, table id and device to dump request")
Cc: David Ahern <dsahern@gmail.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Andrei Vagin <avagin@gmail.com>
Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
Signed-off-by: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>
---
 include/libnetlink.h | 32 +++++++++++++++++++++++++
 ip/iproute.c         | 15 +++++++++++-
 lib/libnetlink.c     | 56 +++++++++++++++++++++++++++++++++++---------
 3 files changed, 91 insertions(+), 12 deletions(-)

diff --git a/include/libnetlink.h b/include/libnetlink.h
index b9073a6a..4545e5e5 100644
--- a/include/libnetlink.h
+++ b/include/libnetlink.h
@@ -104,6 +104,27 @@ struct rtnl_ctrl_data {
 
 typedef int (*rtnl_filter_t)(struct nlmsghdr *n, void *);
 
+/**
+ * rtnl error handler called from
+ * 	rtnl_dump_done()
+ * 	rtnl_dump_error()
+ *
+ * Return value is a bitmask of the following values:
+ * RTNL_LET_NLERR
+ * 	error handled as usual
+ * RTNL_SUPPRESS_NLMSG_DONE_NLERR
+ * 	error in nlmsg_type == NLMSG_DONE will be suppressed
+ * RTNL_SUPPRESS_NLMSG_ERROR_NLERR
+ * 	error in nlmsg_type == NLMSG_ERROR will be suppressed
+ * 	and nlmsg will be skipped
+ * RTNL_SUPPRESS_NLERR - suppress error in both previous cases
+ */
+#define RTNL_LET_NLERR				0x01
+#define RTNL_SUPPRESS_NLMSG_DONE_NLERR		0x02
+#define RTNL_SUPPRESS_NLMSG_ERROR_NLERR		0x04
+#define RTNL_SUPPRESS_NLERR			0x06
+typedef int (*rtnl_err_hndlr_t)(struct nlmsghdr *n, void *);
+
 typedef int (*rtnl_listen_filter_t)(struct rtnl_ctrl_data *,
 				    struct nlmsghdr *n, void *);
 
@@ -113,6 +134,8 @@ typedef int (*nl_ext_ack_fn_t)(const char *errmsg, uint32_t off,
 struct rtnl_dump_filter_arg {
 	rtnl_filter_t filter;
 	void *arg1;
+	rtnl_err_hndlr_t errhndlr;
+	void *arg2;
 	__u16 nc_flags;
 };
 
@@ -121,6 +144,15 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
 			void *arg, __u16 nc_flags);
 #define rtnl_dump_filter(rth, filter, arg) \
 	rtnl_dump_filter_nc(rth, filter, arg, 0)
+int rtnl_dump_filter_errhndlr_nc(struct rtnl_handle *rth,
+				 rtnl_filter_t filter,
+				 void *arg1,
+				 rtnl_err_hndlr_t errhndlr,
+				 void *arg2,
+				 __u16 nc_flags);
+#define rtnl_dump_filter_errhndlr(rth, filter, farg, errhndlr, earg) \
+	rtnl_dump_filter_errhndlr_nc(rth, filter, farg, errhndlr, earg, 0)
+
 int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 	      struct nlmsghdr **answer)
 	__attribute__((warn_unused_result));
diff --git a/ip/iproute.c b/ip/iproute.c
index 5853f026..e45f0bea 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -1727,6 +1727,18 @@ static int iproute_flush(int family, rtnl_filter_t filter_fn)
 	}
 }
 
+static int save_route_errhndlr(struct nlmsghdr *n, void *arg)
+{
+	int err = -*(int *)NLMSG_DATA(n);
+
+	if (n->nlmsg_type == NLMSG_DONE &&
+	    filter.tb == RT_TABLE_MAIN &&
+	    err == ENOENT)
+		return RTNL_SUPPRESS_NLMSG_DONE_NLERR;
+
+	return RTNL_LET_NLERR;
+}
+
 static int iproute_list_flush_or_save(int argc, char **argv, int action)
 {
 	int dump_family = preferred_family;
@@ -1939,7 +1951,8 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
 
 	new_json_obj(json);
 
-	if (rtnl_dump_filter(&rth, filter_fn, stdout) < 0) {
+	if (rtnl_dump_filter_errhndlr(&rth, filter_fn, stdout,
+				      save_route_errhndlr, NULL) < 0) {
 		fprintf(stderr, "Dump terminated\n");
 		return -2;
 	}
diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index c958aa57..80a92e6f 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -673,7 +673,8 @@ int rtnl_dump_request_n(struct rtnl_handle *rth, struct nlmsghdr *n)
 	return sendmsg(rth->fd, &msg, 0);
 }
 
-static int rtnl_dump_done(struct nlmsghdr *h)
+static int rtnl_dump_done(struct nlmsghdr *h,
+			  const struct rtnl_dump_filter_arg *a)
 {
 	int len = *(int *)NLMSG_DATA(h);
 
@@ -683,11 +684,15 @@ static int rtnl_dump_done(struct nlmsghdr *h)
 	}
 
 	if (len < 0) {
+		errno = -len;
+
+		if (a->errhndlr(h, a->arg2) & RTNL_SUPPRESS_NLMSG_DONE_NLERR)
+			return 0;
+
 		/* check for any messages returned from kernel */
 		if (nl_dump_ext_ack_done(h, len))
 			return len;
 
-		errno = -len;
 		switch (errno) {
 		case ENOENT:
 		case EOPNOTSUPP:
@@ -708,8 +713,9 @@ static int rtnl_dump_done(struct nlmsghdr *h)
 	return 0;
 }
 
-static void rtnl_dump_error(const struct rtnl_handle *rth,
-			    struct nlmsghdr *h)
+static int rtnl_dump_error(const struct rtnl_handle *rth,
+			    struct nlmsghdr *h,
+			    const struct rtnl_dump_filter_arg *a)
 {
 
 	if (h->nlmsg_len < NLMSG_LENGTH(sizeof(struct nlmsgerr))) {
@@ -721,11 +727,16 @@ static void rtnl_dump_error(const struct rtnl_handle *rth,
 		if (rth->proto == NETLINK_SOCK_DIAG &&
 		    (errno == ENOENT ||
 		     errno == EOPNOTSUPP))
-			return;
+			return -1;
+
+		if (a->errhndlr(h, a->arg2) & RTNL_SUPPRESS_NLMSG_ERROR_NLERR)
+			return 0;
 
 		if (!(rth->flags & RTNL_HANDLE_F_SUPPRESS_NLERR))
 			perror("RTNETLINK answers");
 	}
+
+	return -1;
 }
 
 static int __rtnl_recvmsg(int fd, struct msghdr *msg, int flags)
@@ -834,7 +845,7 @@ static int rtnl_dump_filter_l(struct rtnl_handle *rth,
 					dump_intr = 1;
 
 				if (h->nlmsg_type == NLMSG_DONE) {
-					err = rtnl_dump_done(h);
+					err = rtnl_dump_done(h, a);
 					if (err < 0) {
 						free(buf);
 						return -1;
@@ -845,9 +856,13 @@ static int rtnl_dump_filter_l(struct rtnl_handle *rth,
 				}
 
 				if (h->nlmsg_type == NLMSG_ERROR) {
-					rtnl_dump_error(rth, h);
-					free(buf);
-					return -1;
+					err = rtnl_dump_error(rth, h, a);
+					if (err < 0) {
+						free(buf);
+						return -1;
+					}
+
+					goto skip_it;
 				}
 
 				if (!rth->dump_fp) {
@@ -887,8 +902,27 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
 		     void *arg1, __u16 nc_flags)
 {
 	const struct rtnl_dump_filter_arg a[2] = {
-		{ .filter = filter, .arg1 = arg1, .nc_flags = nc_flags, },
-		{ .filter = NULL,   .arg1 = NULL, .nc_flags = 0, },
+		{ .filter = filter, .arg1 = arg1,
+		  .errhndlr = NULL, .arg2 = NULL, .nc_flags = nc_flags, },
+		{ .filter = NULL,   .arg1 = NULL,
+		  .errhndlr = NULL, .arg2 = NULL, .nc_flags = 0, },
+	};
+
+	return rtnl_dump_filter_l(rth, a);
+}
+
+int rtnl_dump_filter_errhndlr_nc(struct rtnl_handle *rth,
+		     rtnl_filter_t filter,
+		     void *arg1,
+		     rtnl_err_hndlr_t errhndlr,
+		     void *arg2,
+		     __u16 nc_flags)
+{
+	const struct rtnl_dump_filter_arg a[2] = {
+		{ .filter = filter, .arg1 = arg1,
+		  .errhndlr = errhndlr, .arg2 = arg2, .nc_flags = nc_flags, },
+		{ .filter = NULL,   .arg1 = NULL,
+		  .errhndlr = NULL, .arg2 = NULL, .nc_flags = 0, },
 	};
 
 	return rtnl_dump_filter_l(rth, a);
-- 
2.31.1

Re: [PATCHv4 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Alexander Mikhalitsyn]

On Tue, 29 Jun 2021 18:51:15 +0300
Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:

> We started to use in-kernel filtering feature which allows to get only needed
> tables (see iproute_dump_filter()). From the kernel side it's implemented in
> net/ipv4/fib_frontend.c (inet_dump_fib), net/ipv6/ip6_fib.c (inet6_dump_fib).
> The problem here is that behaviour of "ip route save" was changed after
> c7e6371bc ("ip route: Add protocol, table id and device to dump request").
> If filters are used, then kernel returns ENOENT error if requested table is absent,
> but in newly created net namespace even RT_TABLE_MAIN table doesn't exist.
> It is really allocated, for instance, after issuing "ip l set lo up".
> 
> Reproducer is fairly simple:
> $ unshare -n ip route save > dump
> Error: ipv4: FIB table does not exist.
> Dump terminated
> 
> Expected result here is to get empty dump file (as it was before this change).
> 
> v2: reworked, so, now it takes into account NLMSGERR_ATTR_MSG
> (see nl_dump_ext_ack_done() function). We want to suppress error messages
> in stderr about absent FIB table from kernel too.
> 
> v3: reworked to make code clearer. Introduced rtnl_suppressed_errors(),
> rtnl_suppress_error() helpers. User may suppress up to 3 errors (may be
> easily extened by changing SUPPRESS_ERRORS_INIT macro).
> 
> v4: reworked, rtnl_dump_filter_errhndlr() was introduced. Thanks
> to Stephen Hemminger for comments and suggestions
> 
> Fixes: c7e6371bc ("ip route: Add protocol, table id and device to dump request")
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Cc: Andrei Vagin <avagin@gmail.com>
> Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
> Signed-off-by: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>
> ---
>  include/libnetlink.h | 32 +++++++++++++++++++++++++
>  ip/iproute.c         | 15 +++++++++++-
>  lib/libnetlink.c     | 56 +++++++++++++++++++++++++++++++++++---------
>  3 files changed, 91 insertions(+), 12 deletions(-)
> 
> diff --git a/include/libnetlink.h b/include/libnetlink.h
> index b9073a6a..4545e5e5 100644
> --- a/include/libnetlink.h
> +++ b/include/libnetlink.h
> @@ -104,6 +104,27 @@ struct rtnl_ctrl_data {
>  
>  typedef int (*rtnl_filter_t)(struct nlmsghdr *n, void *);
>  
> +/**
> + * rtnl error handler called from
> + * 	rtnl_dump_done()
> + * 	rtnl_dump_error()
> + *
> + * Return value is a bitmask of the following values:
> + * RTNL_LET_NLERR
> + * 	error handled as usual
> + * RTNL_SUPPRESS_NLMSG_DONE_NLERR
> + * 	error in nlmsg_type == NLMSG_DONE will be suppressed
> + * RTNL_SUPPRESS_NLMSG_ERROR_NLERR
> + * 	error in nlmsg_type == NLMSG_ERROR will be suppressed
> + * 	and nlmsg will be skipped
> + * RTNL_SUPPRESS_NLERR - suppress error in both previous cases
> + */
> +#define RTNL_LET_NLERR				0x01
> +#define RTNL_SUPPRESS_NLMSG_DONE_NLERR		0x02
> +#define RTNL_SUPPRESS_NLMSG_ERROR_NLERR		0x04
> +#define RTNL_SUPPRESS_NLERR			0x06
> +typedef int (*rtnl_err_hndlr_t)(struct nlmsghdr *n, void *);
> +
>  typedef int (*rtnl_listen_filter_t)(struct rtnl_ctrl_data *,
>  				    struct nlmsghdr *n, void *);
>  
> @@ -113,6 +134,8 @@ typedef int (*nl_ext_ack_fn_t)(const char *errmsg, uint32_t off,
>  struct rtnl_dump_filter_arg {
>  	rtnl_filter_t filter;
>  	void *arg1;
> +	rtnl_err_hndlr_t errhndlr;
> +	void *arg2;
>  	__u16 nc_flags;
>  };
>  
> @@ -121,6 +144,15 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
>  			void *arg, __u16 nc_flags);
>  #define rtnl_dump_filter(rth, filter, arg) \
>  	rtnl_dump_filter_nc(rth, filter, arg, 0)
> +int rtnl_dump_filter_errhndlr_nc(struct rtnl_handle *rth,
> +				 rtnl_filter_t filter,
> +				 void *arg1,
> +				 rtnl_err_hndlr_t errhndlr,
> +				 void *arg2,
> +				 __u16 nc_flags);
> +#define rtnl_dump_filter_errhndlr(rth, filter, farg, errhndlr, earg) \
> +	rtnl_dump_filter_errhndlr_nc(rth, filter, farg, errhndlr, earg, 0)
> +
>  int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
>  	      struct nlmsghdr **answer)
>  	__attribute__((warn_unused_result));
> diff --git a/ip/iproute.c b/ip/iproute.c
> index 5853f026..e45f0bea 100644
> --- a/ip/iproute.c
> +++ b/ip/iproute.c
> @@ -1727,6 +1727,18 @@ static int iproute_flush(int family, rtnl_filter_t filter_fn)
>  	}
>  }
>  
> +static int save_route_errhndlr(struct nlmsghdr *n, void *arg)
> +{
> +	int err = -*(int *)NLMSG_DATA(n);
> +
> +	if (n->nlmsg_type == NLMSG_DONE &&
> +	    filter.tb == RT_TABLE_MAIN &&
> +	    err == ENOENT)
> +		return RTNL_SUPPRESS_NLMSG_DONE_NLERR;
> +
> +	return RTNL_LET_NLERR;
> +}
> +
>  static int iproute_list_flush_or_save(int argc, char **argv, int action)
>  {
>  	int dump_family = preferred_family;
> @@ -1939,7 +1951,8 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
>  
>  	new_json_obj(json);
>  
> -	if (rtnl_dump_filter(&rth, filter_fn, stdout) < 0) {
> +	if (rtnl_dump_filter_errhndlr(&rth, filter_fn, stdout,
> +				      save_route_errhndlr, NULL) < 0) {
>  		fprintf(stderr, "Dump terminated\n");
>  		return -2;
>  	}
> diff --git a/lib/libnetlink.c b/lib/libnetlink.c
> index c958aa57..80a92e6f 100644
> --- a/lib/libnetlink.c
> +++ b/lib/libnetlink.c
> @@ -673,7 +673,8 @@ int rtnl_dump_request_n(struct rtnl_handle *rth, struct nlmsghdr *n)
>  	return sendmsg(rth->fd, &msg, 0);
>  }
>  
> -static int rtnl_dump_done(struct nlmsghdr *h)
> +static int rtnl_dump_done(struct nlmsghdr *h,
> +			  const struct rtnl_dump_filter_arg *a)
>  {
>  	int len = *(int *)NLMSG_DATA(h);
>  
> @@ -683,11 +684,15 @@ static int rtnl_dump_done(struct nlmsghdr *h)
>  	}
>  
>  	if (len < 0) {
> +		errno = -len;
> +
> +		if (a->errhndlr(h, a->arg2) & RTNL_SUPPRESS_NLMSG_DONE_NLERR)
> +			return 0;
> +
>  		/* check for any messages returned from kernel */
>  		if (nl_dump_ext_ack_done(h, len))
>  			return len;
>  
> -		errno = -len;
>  		switch (errno) {
>  		case ENOENT:
>  		case EOPNOTSUPP:
> @@ -708,8 +713,9 @@ static int rtnl_dump_done(struct nlmsghdr *h)
>  	return 0;
>  }
>  
> -static void rtnl_dump_error(const struct rtnl_handle *rth,
> -			    struct nlmsghdr *h)
> +static int rtnl_dump_error(const struct rtnl_handle *rth,
> +			    struct nlmsghdr *h,
> +			    const struct rtnl_dump_filter_arg *a)
>  {
>  
>  	if (h->nlmsg_len < NLMSG_LENGTH(sizeof(struct nlmsgerr))) {
> @@ -721,11 +727,16 @@ static void rtnl_dump_error(const struct rtnl_handle *rth,
>  		if (rth->proto == NETLINK_SOCK_DIAG &&
>  		    (errno == ENOENT ||
>  		     errno == EOPNOTSUPP))
> -			return;
> +			return -1;
> +
> +		if (a->errhndlr(h, a->arg2) & RTNL_SUPPRESS_NLMSG_ERROR_NLERR)
> +			return 0;
>  
>  		if (!(rth->flags & RTNL_HANDLE_F_SUPPRESS_NLERR))
>  			perror("RTNETLINK answers");
>  	}
> +
> +	return -1;
>  }
>  
>  static int __rtnl_recvmsg(int fd, struct msghdr *msg, int flags)
> @@ -834,7 +845,7 @@ static int rtnl_dump_filter_l(struct rtnl_handle *rth,
>  					dump_intr = 1;
>  
>  				if (h->nlmsg_type == NLMSG_DONE) {
> -					err = rtnl_dump_done(h);
> +					err = rtnl_dump_done(h, a);
>  					if (err < 0) {
>  						free(buf);
>  						return -1;
> @@ -845,9 +856,13 @@ static int rtnl_dump_filter_l(struct rtnl_handle *rth,
>  				}
>  
>  				if (h->nlmsg_type == NLMSG_ERROR) {
> -					rtnl_dump_error(rth, h);
> -					free(buf);
> -					return -1;
> +					err = rtnl_dump_error(rth, h, a);
> +					if (err < 0) {
> +						free(buf);
> +						return -1;
> +					}
> +
> +					goto skip_it;
>  				}
>  
>  				if (!rth->dump_fp) {
> @@ -887,8 +902,27 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
>  		     void *arg1, __u16 nc_flags)
>  {
>  	const struct rtnl_dump_filter_arg a[2] = {
> -		{ .filter = filter, .arg1 = arg1, .nc_flags = nc_flags, },
> -		{ .filter = NULL,   .arg1 = NULL, .nc_flags = 0, },
> +		{ .filter = filter, .arg1 = arg1,
> +		  .errhndlr = NULL, .arg2 = NULL, .nc_flags = nc_flags, },
> +		{ .filter = NULL,   .arg1 = NULL,
> +		  .errhndlr = NULL, .arg2 = NULL, .nc_flags = 0, },
> +	};
> +
> +	return rtnl_dump_filter_l(rth, a);
> +}
> +
> +int rtnl_dump_filter_errhndlr_nc(struct rtnl_handle *rth,
> +		     rtnl_filter_t filter,
> +		     void *arg1,
> +		     rtnl_err_hndlr_t errhndlr,
> +		     void *arg2,
> +		     __u16 nc_flags)
> +{
> +	const struct rtnl_dump_filter_arg a[2] = {
> +		{ .filter = filter, .arg1 = arg1,
> +		  .errhndlr = errhndlr, .arg2 = arg2, .nc_flags = nc_flags, },
> +		{ .filter = NULL,   .arg1 = NULL,
> +		  .errhndlr = NULL, .arg2 = NULL, .nc_flags = 0, },
>  	};
>  
>  	return rtnl_dump_filter_l(rth, a);
> -- 
> 2.31.1
> 

gentle ping

Re: [PATCHv4 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Stephen Hemminger]

On Tue, 29 Jun 2021 18:51:15 +0300
Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:

> +	const struct rtnl_dump_filter_arg a[2] = {
> +		{ .filter = filter, .arg1 = arg1,
> +		  .errhndlr = errhndlr, .arg2 = arg2, .nc_flags = nc_flags, },
> +		{ .filter = NULL,   .arg1 = NULL,
> +		  .errhndlr = NULL, .arg2 = NULL, .nc_flags = 0, },
>  	};

I am OK with this as is. But you don't need to add initializers for fields
that are 0/NULL (at least in C).

So could be:
	const struct rtnl_dump_filter_arg a[] = {
		{ .filter = filter, .arg1 = arg1,
		  .errhndlr = errhndlr, .arg2 = arg2, .nc_flags = nc_flags, },
		{  },
 	};

Re: [PATCHv4 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Alexander Mikhalitsyn]

On Tue, 6 Jul 2021 08:34:07 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

> On Tue, 29 Jun 2021 18:51:15 +0300
> Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:
> 
> > +	const struct rtnl_dump_filter_arg a[2] = {
> > +		{ .filter = filter, .arg1 = arg1,
> > +		  .errhndlr = errhndlr, .arg2 = arg2, .nc_flags = nc_flags, },
> > +		{ .filter = NULL,   .arg1 = NULL,
> > +		  .errhndlr = NULL, .arg2 = NULL, .nc_flags = 0, },
> >  	};
> 
> I am OK with this as is. But you don't need to add initializers for fields
> that are 0/NULL (at least in C).

Sure, I've made such explicit initializations just because in original
rtnl_dump_filter_nc() we already have them.

Do I need to resend with fixed initializations? ;)

> 
> So could be:
> 	const struct rtnl_dump_filter_arg a[] = {
> 		{ .filter = filter, .arg1 = arg1,
> 		  .errhndlr = errhndlr, .arg2 = arg2, .nc_flags = nc_flags, },
> 		{  },
>  	};

Thanks,
Alex

Re: [PATCHv4 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Stephen Hemminger]

On Tue, 6 Jul 2021 18:44:15 +0300
Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:

> On Tue, 6 Jul 2021 08:34:07 -0700
> Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> > On Tue, 29 Jun 2021 18:51:15 +0300
> > Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:
> >   
> > > +	const struct rtnl_dump_filter_arg a[2] = {
> > > +		{ .filter = filter, .arg1 = arg1,
> > > +		  .errhndlr = errhndlr, .arg2 = arg2, .nc_flags = nc_flags, },
> > > +		{ .filter = NULL,   .arg1 = NULL,
> > > +		  .errhndlr = NULL, .arg2 = NULL, .nc_flags = 0, },
> > >  	};  
> > 
> > I am OK with this as is. But you don't need to add initializers for fields
> > that are 0/NULL (at least in C).  
> 
> Sure, I've made such explicit initializations just because in original
> rtnl_dump_filter_nc() we already have them.
> 
> Do I need to resend with fixed initializations? ;)

Not worth it

Re: [PATCHv4 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Alexander Mikhalitsyn]

On Tue, 6 Jul 2021 09:18:21 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

> On Tue, 6 Jul 2021 18:44:15 +0300
> Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:
> 
> > On Tue, 6 Jul 2021 08:34:07 -0700
> > Stephen Hemminger <stephen@networkplumber.org> wrote:
> > 
> > > On Tue, 29 Jun 2021 18:51:15 +0300
> > > Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:
> > >   
> > > > +	const struct rtnl_dump_filter_arg a[2] = {
> > > > +		{ .filter = filter, .arg1 = arg1,
> > > > +		  .errhndlr = errhndlr, .arg2 = arg2, .nc_flags = nc_flags, },
> > > > +		{ .filter = NULL,   .arg1 = NULL,
> > > > +		  .errhndlr = NULL, .arg2 = NULL, .nc_flags = 0, },
> > > >  	};  
> > > 
> > > I am OK with this as is. But you don't need to add initializers for fields
> > > that are 0/NULL (at least in C).  
> > 
> > Sure, I've made such explicit initializations just because in original
> > rtnl_dump_filter_nc() we already have them.
> > 
> > Do I need to resend with fixed initializations? ;)
> 
> Not worth it

Ok, thanks!

Re: [PATCHv4 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Stephen Hemminger]

On Tue, 6 Jul 2021 20:17:57 +0300
Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:

> On Tue, 6 Jul 2021 09:18:21 -0700
> Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> > On Tue, 6 Jul 2021 18:44:15 +0300
> > Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:
> >   
> > > On Tue, 6 Jul 2021 08:34:07 -0700
> > > Stephen Hemminger <stephen@networkplumber.org> wrote:
> > >   
> > > > On Tue, 29 Jun 2021 18:51:15 +0300
> > > > Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:
> > > >     
> > > > > +	const struct rtnl_dump_filter_arg a[2] = {
> > > > > +		{ .filter = filter, .arg1 = arg1,
> > > > > +		  .errhndlr = errhndlr, .arg2 = arg2, .nc_flags = nc_flags, },
> > > > > +		{ .filter = NULL,   .arg1 = NULL,
> > > > > +		  .errhndlr = NULL, .arg2 = NULL, .nc_flags = 0, },
> > > > >  	};    
> > > > 
> > > > I am OK with this as is. But you don't need to add initializers for fields
> > > > that are 0/NULL (at least in C).    
> > > 
> > > Sure, I've made such explicit initializations just because in original
> > > rtnl_dump_filter_nc() we already have them.
> > > 
> > > Do I need to resend with fixed initializations? ;)  
> > 
> > Not worth it  
> 
> Ok, thanks!
> 

Looks like you need to send v5 anyway.


checkpatch.pl ~/Downloads/PATCHv4-iproute2-ip-route-ignore-ENOENT-during-save-if-RT_TABLE_MAIN-is-being-dumped.patch 
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#62: 
We started to use in-kernel filtering feature which allows to get only needed

WARNING: 'extened' may be misspelled - perhaps 'extended'?
#84: 
easily extened by changing SUPPRESS_ERRORS_INIT macro).
       ^^^^^^^

WARNING: please, no space before tabs
#111: FILE: include/libnetlink.h:109:
+ * ^Irtnl_dump_done()$

WARNING: please, no space before tabs
#112: FILE: include/libnetlink.h:110:
+ * ^Irtnl_dump_error()$

WARNING: please, no space before tabs
#116: FILE: include/libnetlink.h:114:
+ * ^Ierror handled as usual$

WARNING: please, no space before tabs
#118: FILE: include/libnetlink.h:116:
+ * ^Ierror in nlmsg_type == NLMSG_DONE will be suppressed$


WARNING: please, no space before tabs
#120: FILE: include/libnetlink.h:118:
+ * ^Ierror in nlmsg_type == NLMSG_ERROR will be suppressed$

WARNING: please, no space before tabs
#121: FILE: include/libnetlink.h:119:
+ * ^Iand nlmsg will be skipped$

total: 0 errors, 8 warnings, 183 lines checked

[PATCHv5 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Alexander Mikhalitsyn]

We started to use in-kernel filtering feature which allows to get only
needed tables (see iproute_dump_filter()). From the kernel side it's
implemented in net/ipv4/fib_frontend.c (inet_dump_fib), net/ipv6/ip6_fib.c
(inet6_dump_fib). The problem here is that behaviour of "ip route save"
was changed after
c7e6371bc ("ip route: Add protocol, table id and device to dump request").
If filters are used, then kernel returns ENOENT error if requested table
is absent, but in newly created net namespace even RT_TABLE_MAIN table
doesn't exist. It is really allocated, for instance, after issuing
"ip l set lo up".

Reproducer is fairly simple:
$ unshare -n ip route save > dump
Error: ipv4: FIB table does not exist.
Dump terminated

Expected result here is to get empty dump file (as it was before this
change).

v2: reworked, so, now it takes into account NLMSGERR_ATTR_MSG
(see nl_dump_ext_ack_done() function). We want to suppress error messages
in stderr about absent FIB table from kernel too.

v3: reworked to make code clearer. Introduced rtnl_suppressed_errors(),
rtnl_suppress_error() helpers. User may suppress up to 3 errors (may be
easily extended by changing SUPPRESS_ERRORS_INIT macro).

v4: reworked, rtnl_dump_filter_errhndlr() was introduced. Thanks
to Stephen Hemminger for comments and suggestions

v5: space fixes, commit message reformat

Fixes: c7e6371bc ("ip route: Add protocol, table id and device to dump request")
Cc: David Ahern <dsahern@gmail.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Andrei Vagin <avagin@gmail.com>
Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
Signed-off-by: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>
---
 include/libnetlink.h | 32 +++++++++++++++++++++++++
 ip/iproute.c         | 15 +++++++++++-
 lib/libnetlink.c     | 56 +++++++++++++++++++++++++++++++++++---------
 3 files changed, 91 insertions(+), 12 deletions(-)

diff --git a/include/libnetlink.h b/include/libnetlink.h
index b9073a6a..4401551f 100644
--- a/include/libnetlink.h
+++ b/include/libnetlink.h
@@ -104,6 +104,27 @@ struct rtnl_ctrl_data {
 
 typedef int (*rtnl_filter_t)(struct nlmsghdr *n, void *);
 
+/**
+ * rtnl error handler called from
+ *      rtnl_dump_done()
+ *      rtnl_dump_error()
+ *
+ * Return value is a bitmask of the following values:
+ * RTNL_LET_NLERR
+ *      error handled as usual
+ * RTNL_SUPPRESS_NLMSG_DONE_NLERR
+ *      error in nlmsg_type == NLMSG_DONE will be suppressed
+ * RTNL_SUPPRESS_NLMSG_ERROR_NLERR
+ *      error in nlmsg_type == NLMSG_ERROR will be suppressed
+ *      and nlmsg will be skipped
+ * RTNL_SUPPRESS_NLERR - suppress error in both previous cases
+ */
+#define RTNL_LET_NLERR				0x01
+#define RTNL_SUPPRESS_NLMSG_DONE_NLERR		0x02
+#define RTNL_SUPPRESS_NLMSG_ERROR_NLERR		0x04
+#define RTNL_SUPPRESS_NLERR			0x06
+typedef int (*rtnl_err_hndlr_t)(struct nlmsghdr *n, void *);
+
 typedef int (*rtnl_listen_filter_t)(struct rtnl_ctrl_data *,
 				    struct nlmsghdr *n, void *);
 
@@ -113,6 +134,8 @@ typedef int (*nl_ext_ack_fn_t)(const char *errmsg, uint32_t off,
 struct rtnl_dump_filter_arg {
 	rtnl_filter_t filter;
 	void *arg1;
+	rtnl_err_hndlr_t errhndlr;
+	void *arg2;
 	__u16 nc_flags;
 };
 
@@ -121,6 +144,15 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
 			void *arg, __u16 nc_flags);
 #define rtnl_dump_filter(rth, filter, arg) \
 	rtnl_dump_filter_nc(rth, filter, arg, 0)
+int rtnl_dump_filter_errhndlr_nc(struct rtnl_handle *rth,
+				 rtnl_filter_t filter,
+				 void *arg1,
+				 rtnl_err_hndlr_t errhndlr,
+				 void *arg2,
+				 __u16 nc_flags);
+#define rtnl_dump_filter_errhndlr(rth, filter, farg, errhndlr, earg) \
+	rtnl_dump_filter_errhndlr_nc(rth, filter, farg, errhndlr, earg, 0)
+
 int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 	      struct nlmsghdr **answer)
 	__attribute__((warn_unused_result));
diff --git a/ip/iproute.c b/ip/iproute.c
index 5853f026..e45f0bea 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -1727,6 +1727,18 @@ static int iproute_flush(int family, rtnl_filter_t filter_fn)
 	}
 }
 
+static int save_route_errhndlr(struct nlmsghdr *n, void *arg)
+{
+	int err = -*(int *)NLMSG_DATA(n);
+
+	if (n->nlmsg_type == NLMSG_DONE &&
+	    filter.tb == RT_TABLE_MAIN &&
+	    err == ENOENT)
+		return RTNL_SUPPRESS_NLMSG_DONE_NLERR;
+
+	return RTNL_LET_NLERR;
+}
+
 static int iproute_list_flush_or_save(int argc, char **argv, int action)
 {
 	int dump_family = preferred_family;
@@ -1939,7 +1951,8 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
 
 	new_json_obj(json);
 
-	if (rtnl_dump_filter(&rth, filter_fn, stdout) < 0) {
+	if (rtnl_dump_filter_errhndlr(&rth, filter_fn, stdout,
+				      save_route_errhndlr, NULL) < 0) {
 		fprintf(stderr, "Dump terminated\n");
 		return -2;
 	}
diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index c958aa57..80a92e6f 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -673,7 +673,8 @@ int rtnl_dump_request_n(struct rtnl_handle *rth, struct nlmsghdr *n)
 	return sendmsg(rth->fd, &msg, 0);
 }
 
-static int rtnl_dump_done(struct nlmsghdr *h)
+static int rtnl_dump_done(struct nlmsghdr *h,
+			  const struct rtnl_dump_filter_arg *a)
 {
 	int len = *(int *)NLMSG_DATA(h);
 
@@ -683,11 +684,15 @@ static int rtnl_dump_done(struct nlmsghdr *h)
 	}
 
 	if (len < 0) {
+		errno = -len;
+
+		if (a->errhndlr(h, a->arg2) & RTNL_SUPPRESS_NLMSG_DONE_NLERR)
+			return 0;
+
 		/* check for any messages returned from kernel */
 		if (nl_dump_ext_ack_done(h, len))
 			return len;
 
-		errno = -len;
 		switch (errno) {
 		case ENOENT:
 		case EOPNOTSUPP:
@@ -708,8 +713,9 @@ static int rtnl_dump_done(struct nlmsghdr *h)
 	return 0;
 }
 
-static void rtnl_dump_error(const struct rtnl_handle *rth,
-			    struct nlmsghdr *h)
+static int rtnl_dump_error(const struct rtnl_handle *rth,
+			    struct nlmsghdr *h,
+			    const struct rtnl_dump_filter_arg *a)
 {
 
 	if (h->nlmsg_len < NLMSG_LENGTH(sizeof(struct nlmsgerr))) {
@@ -721,11 +727,16 @@ static void rtnl_dump_error(const struct rtnl_handle *rth,
 		if (rth->proto == NETLINK_SOCK_DIAG &&
 		    (errno == ENOENT ||
 		     errno == EOPNOTSUPP))
-			return;
+			return -1;
+
+		if (a->errhndlr(h, a->arg2) & RTNL_SUPPRESS_NLMSG_ERROR_NLERR)
+			return 0;
 
 		if (!(rth->flags & RTNL_HANDLE_F_SUPPRESS_NLERR))
 			perror("RTNETLINK answers");
 	}
+
+	return -1;
 }
 
 static int __rtnl_recvmsg(int fd, struct msghdr *msg, int flags)
@@ -834,7 +845,7 @@ static int rtnl_dump_filter_l(struct rtnl_handle *rth,
 					dump_intr = 1;
 
 				if (h->nlmsg_type == NLMSG_DONE) {
-					err = rtnl_dump_done(h);
+					err = rtnl_dump_done(h, a);
 					if (err < 0) {
 						free(buf);
 						return -1;
@@ -845,9 +856,13 @@ static int rtnl_dump_filter_l(struct rtnl_handle *rth,
 				}
 
 				if (h->nlmsg_type == NLMSG_ERROR) {
-					rtnl_dump_error(rth, h);
-					free(buf);
-					return -1;
+					err = rtnl_dump_error(rth, h, a);
+					if (err < 0) {
+						free(buf);
+						return -1;
+					}
+
+					goto skip_it;
 				}
 
 				if (!rth->dump_fp) {
@@ -887,8 +902,27 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
 		     void *arg1, __u16 nc_flags)
 {
 	const struct rtnl_dump_filter_arg a[2] = {
-		{ .filter = filter, .arg1 = arg1, .nc_flags = nc_flags, },
-		{ .filter = NULL,   .arg1 = NULL, .nc_flags = 0, },
+		{ .filter = filter, .arg1 = arg1,
+		  .errhndlr = NULL, .arg2 = NULL, .nc_flags = nc_flags, },
+		{ .filter = NULL,   .arg1 = NULL,
+		  .errhndlr = NULL, .arg2 = NULL, .nc_flags = 0, },
+	};
+
+	return rtnl_dump_filter_l(rth, a);
+}
+
+int rtnl_dump_filter_errhndlr_nc(struct rtnl_handle *rth,
+		     rtnl_filter_t filter,
+		     void *arg1,
+		     rtnl_err_hndlr_t errhndlr,
+		     void *arg2,
+		     __u16 nc_flags)
+{
+	const struct rtnl_dump_filter_arg a[2] = {
+		{ .filter = filter, .arg1 = arg1,
+		  .errhndlr = errhndlr, .arg2 = arg2, .nc_flags = nc_flags, },
+		{ .filter = NULL,   .arg1 = NULL,
+		  .errhndlr = NULL, .arg2 = NULL, .nc_flags = 0, },
 	};
 
 	return rtnl_dump_filter_l(rth, a);
-- 
2.31.1

[PATCHv5 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Alexander Mikhalitsyn]

We started to use in-kernel filtering feature which allows to get only
needed tables (see iproute_dump_filter()). From the kernel side it's
implemented in net/ipv4/fib_frontend.c (inet_dump_fib), net/ipv6/ip6_fib.c
(inet6_dump_fib). The problem here is that behaviour of "ip route save"
was changed after
c7e6371bc ("ip route: Add protocol, table id and device to dump request").
If filters are used, then kernel returns ENOENT error if requested table
is absent, but in newly created net namespace even RT_TABLE_MAIN table
doesn't exist. It is really allocated, for instance, after issuing
"ip l set lo up".

Reproducer is fairly simple:
$ unshare -n ip route save > dump
Error: ipv4: FIB table does not exist.
Dump terminated

Expected result here is to get empty dump file (as it was before this
change).

v2: reworked, so, now it takes into account NLMSGERR_ATTR_MSG
(see nl_dump_ext_ack_done() function). We want to suppress error messages
in stderr about absent FIB table from kernel too.

v3: reworked to make code clearer. Introduced rtnl_suppressed_errors(),
rtnl_suppress_error() helpers. User may suppress up to 3 errors (may be
easily extended by changing SUPPRESS_ERRORS_INIT macro).

v4: reworked, rtnl_dump_filter_errhndlr() was introduced. Thanks
to Stephen Hemminger for comments and suggestions

v5: space fixes, commit message reformat, empty initializers

Fixes: c7e6371bc ("ip route: Add protocol, table id and device to dump request")
Cc: David Ahern <dsahern@gmail.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Andrei Vagin <avagin@gmail.com>
Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
Signed-off-by: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>
---
 include/libnetlink.h | 32 ++++++++++++++++++++++++++
 ip/iproute.c         | 15 +++++++++++-
 lib/libnetlink.c     | 54 +++++++++++++++++++++++++++++++++++---------
 3 files changed, 89 insertions(+), 12 deletions(-)

diff --git a/include/libnetlink.h b/include/libnetlink.h
index b9073a6a..4401551f 100644
--- a/include/libnetlink.h
+++ b/include/libnetlink.h
@@ -104,6 +104,27 @@ struct rtnl_ctrl_data {
 
 typedef int (*rtnl_filter_t)(struct nlmsghdr *n, void *);
 
+/**
+ * rtnl error handler called from
+ *      rtnl_dump_done()
+ *      rtnl_dump_error()
+ *
+ * Return value is a bitmask of the following values:
+ * RTNL_LET_NLERR
+ *      error handled as usual
+ * RTNL_SUPPRESS_NLMSG_DONE_NLERR
+ *      error in nlmsg_type == NLMSG_DONE will be suppressed
+ * RTNL_SUPPRESS_NLMSG_ERROR_NLERR
+ *      error in nlmsg_type == NLMSG_ERROR will be suppressed
+ *      and nlmsg will be skipped
+ * RTNL_SUPPRESS_NLERR - suppress error in both previous cases
+ */
+#define RTNL_LET_NLERR				0x01
+#define RTNL_SUPPRESS_NLMSG_DONE_NLERR		0x02
+#define RTNL_SUPPRESS_NLMSG_ERROR_NLERR		0x04
+#define RTNL_SUPPRESS_NLERR			0x06
+typedef int (*rtnl_err_hndlr_t)(struct nlmsghdr *n, void *);
+
 typedef int (*rtnl_listen_filter_t)(struct rtnl_ctrl_data *,
 				    struct nlmsghdr *n, void *);
 
@@ -113,6 +134,8 @@ typedef int (*nl_ext_ack_fn_t)(const char *errmsg, uint32_t off,
 struct rtnl_dump_filter_arg {
 	rtnl_filter_t filter;
 	void *arg1;
+	rtnl_err_hndlr_t errhndlr;
+	void *arg2;
 	__u16 nc_flags;
 };
 
@@ -121,6 +144,15 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
 			void *arg, __u16 nc_flags);
 #define rtnl_dump_filter(rth, filter, arg) \
 	rtnl_dump_filter_nc(rth, filter, arg, 0)
+int rtnl_dump_filter_errhndlr_nc(struct rtnl_handle *rth,
+				 rtnl_filter_t filter,
+				 void *arg1,
+				 rtnl_err_hndlr_t errhndlr,
+				 void *arg2,
+				 __u16 nc_flags);
+#define rtnl_dump_filter_errhndlr(rth, filter, farg, errhndlr, earg) \
+	rtnl_dump_filter_errhndlr_nc(rth, filter, farg, errhndlr, earg, 0)
+
 int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 	      struct nlmsghdr **answer)
 	__attribute__((warn_unused_result));
diff --git a/ip/iproute.c b/ip/iproute.c
index 5853f026..e45f0bea 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -1727,6 +1727,18 @@ static int iproute_flush(int family, rtnl_filter_t filter_fn)
 	}
 }
 
+static int save_route_errhndlr(struct nlmsghdr *n, void *arg)
+{
+	int err = -*(int *)NLMSG_DATA(n);
+
+	if (n->nlmsg_type == NLMSG_DONE &&
+	    filter.tb == RT_TABLE_MAIN &&
+	    err == ENOENT)
+		return RTNL_SUPPRESS_NLMSG_DONE_NLERR;
+
+	return RTNL_LET_NLERR;
+}
+
 static int iproute_list_flush_or_save(int argc, char **argv, int action)
 {
 	int dump_family = preferred_family;
@@ -1939,7 +1951,8 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
 
 	new_json_obj(json);
 
-	if (rtnl_dump_filter(&rth, filter_fn, stdout) < 0) {
+	if (rtnl_dump_filter_errhndlr(&rth, filter_fn, stdout,
+				      save_route_errhndlr, NULL) < 0) {
 		fprintf(stderr, "Dump terminated\n");
 		return -2;
 	}
diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index c958aa57..e9b8c3bd 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -673,7 +673,8 @@ int rtnl_dump_request_n(struct rtnl_handle *rth, struct nlmsghdr *n)
 	return sendmsg(rth->fd, &msg, 0);
 }
 
-static int rtnl_dump_done(struct nlmsghdr *h)
+static int rtnl_dump_done(struct nlmsghdr *h,
+			  const struct rtnl_dump_filter_arg *a)
 {
 	int len = *(int *)NLMSG_DATA(h);
 
@@ -683,11 +684,15 @@ static int rtnl_dump_done(struct nlmsghdr *h)
 	}
 
 	if (len < 0) {
+		errno = -len;
+
+		if (a->errhndlr(h, a->arg2) & RTNL_SUPPRESS_NLMSG_DONE_NLERR)
+			return 0;
+
 		/* check for any messages returned from kernel */
 		if (nl_dump_ext_ack_done(h, len))
 			return len;
 
-		errno = -len;
 		switch (errno) {
 		case ENOENT:
 		case EOPNOTSUPP:
@@ -708,8 +713,9 @@ static int rtnl_dump_done(struct nlmsghdr *h)
 	return 0;
 }
 
-static void rtnl_dump_error(const struct rtnl_handle *rth,
-			    struct nlmsghdr *h)
+static int rtnl_dump_error(const struct rtnl_handle *rth,
+			    struct nlmsghdr *h,
+			    const struct rtnl_dump_filter_arg *a)
 {
 
 	if (h->nlmsg_len < NLMSG_LENGTH(sizeof(struct nlmsgerr))) {
@@ -721,11 +727,16 @@ static void rtnl_dump_error(const struct rtnl_handle *rth,
 		if (rth->proto == NETLINK_SOCK_DIAG &&
 		    (errno == ENOENT ||
 		     errno == EOPNOTSUPP))
-			return;
+			return -1;
+
+		if (a->errhndlr(h, a->arg2) & RTNL_SUPPRESS_NLMSG_ERROR_NLERR)
+			return 0;
 
 		if (!(rth->flags & RTNL_HANDLE_F_SUPPRESS_NLERR))
 			perror("RTNETLINK answers");
 	}
+
+	return -1;
 }
 
 static int __rtnl_recvmsg(int fd, struct msghdr *msg, int flags)
@@ -834,7 +845,7 @@ static int rtnl_dump_filter_l(struct rtnl_handle *rth,
 					dump_intr = 1;
 
 				if (h->nlmsg_type == NLMSG_DONE) {
-					err = rtnl_dump_done(h);
+					err = rtnl_dump_done(h, a);
 					if (err < 0) {
 						free(buf);
 						return -1;
@@ -845,9 +856,13 @@ static int rtnl_dump_filter_l(struct rtnl_handle *rth,
 				}
 
 				if (h->nlmsg_type == NLMSG_ERROR) {
-					rtnl_dump_error(rth, h);
-					free(buf);
-					return -1;
+					err = rtnl_dump_error(rth, h, a);
+					if (err < 0) {
+						free(buf);
+						return -1;
+					}
+
+					goto skip_it;
 				}
 
 				if (!rth->dump_fp) {
@@ -887,8 +902,25 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
 		     void *arg1, __u16 nc_flags)
 {
 	const struct rtnl_dump_filter_arg a[2] = {
-		{ .filter = filter, .arg1 = arg1, .nc_flags = nc_flags, },
-		{ .filter = NULL,   .arg1 = NULL, .nc_flags = 0, },
+		{ .filter = filter, .arg1 = arg1,
+		  .errhndlr = NULL, .arg2 = NULL, .nc_flags = nc_flags, },
+		{ },
+	};
+
+	return rtnl_dump_filter_l(rth, a);
+}
+
+int rtnl_dump_filter_errhndlr_nc(struct rtnl_handle *rth,
+		     rtnl_filter_t filter,
+		     void *arg1,
+		     rtnl_err_hndlr_t errhndlr,
+		     void *arg2,
+		     __u16 nc_flags)
+{
+	const struct rtnl_dump_filter_arg a[2] = {
+		{ .filter = filter, .arg1 = arg1,
+		  .errhndlr = errhndlr, .arg2 = arg2, .nc_flags = nc_flags, },
+		{ },
 	};
 
 	return rtnl_dump_filter_l(rth, a);
-- 
2.31.1

Re: [PATCHv4 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Alexander Mikhalitsyn]

On Tue, 6 Jul 2021 17:05:45 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

> On Tue, 6 Jul 2021 20:17:57 +0300
> Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:
> 
> > On Tue, 6 Jul 2021 09:18:21 -0700
> > Stephen Hemminger <stephen@networkplumber.org> wrote:
> > 
> > > On Tue, 6 Jul 2021 18:44:15 +0300
> > > Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:
> > >   
> > > > On Tue, 6 Jul 2021 08:34:07 -0700
> > > > Stephen Hemminger <stephen@networkplumber.org> wrote:
> > > >   
> > > > > On Tue, 29 Jun 2021 18:51:15 +0300
> > > > > Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:
> > > > >     
> > > > > > +	const struct rtnl_dump_filter_arg a[2] = {
> > > > > > +		{ .filter = filter, .arg1 = arg1,
> > > > > > +		  .errhndlr = errhndlr, .arg2 = arg2, .nc_flags = nc_flags, },
> > > > > > +		{ .filter = NULL,   .arg1 = NULL,
> > > > > > +		  .errhndlr = NULL, .arg2 = NULL, .nc_flags = 0, },
> > > > > >  	};    
> > > > > 
> > > > > I am OK with this as is. But you don't need to add initializers for fields
> > > > > that are 0/NULL (at least in C).    
> > > > 
> > > > Sure, I've made such explicit initializations just because in original
> > > > rtnl_dump_filter_nc() we already have them.
> > > > 
> > > > Do I need to resend with fixed initializations? ;)  
> > > 
> > > Not worth it  
> > 
> > Ok, thanks!
> > 
> 
> Looks like you need to send v5 anyway.
> 
> 
> checkpatch.pl ~/Downloads/PATCHv4-iproute2-ip-route-ignore-ENOENT-during-save-if-RT_TABLE_MAIN-is-being-dumped.patch 
> WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
> #62: 
> We started to use in-kernel filtering feature which allows to get only needed
> 
> WARNING: 'extened' may be misspelled - perhaps 'extended'?
> #84: 
> easily extened by changing SUPPRESS_ERRORS_INIT macro).
>        ^^^^^^^
> 
> WARNING: please, no space before tabs
> #111: FILE: include/libnetlink.h:109:
> + * ^Irtnl_dump_done()$
> 
> WARNING: please, no space before tabs
> #112: FILE: include/libnetlink.h:110:
> + * ^Irtnl_dump_error()$
> 
> WARNING: please, no space before tabs
> #116: FILE: include/libnetlink.h:114:
> + * ^Ierror handled as usual$
> 
> WARNING: please, no space before tabs
> #118: FILE: include/libnetlink.h:116:
> + * ^Ierror in nlmsg_type == NLMSG_DONE will be suppressed$
> 
> 
> WARNING: please, no space before tabs
> #120: FILE: include/libnetlink.h:118:
> + * ^Ierror in nlmsg_type == NLMSG_ERROR will be suppressed$
> 
> WARNING: please, no space before tabs
> #121: FILE: include/libnetlink.h:119:
> + * ^Iand nlmsg will be skipped$
> 
> total: 0 errors, 8 warnings, 183 lines checked

Oh, sorry about that. I've sent v5 and checked with checkpatch.pl
I've send two options for v5 - with initializers fix and without :)

Regards,
Alex

Re: [PATCHv4 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Stephen Hemminger]

On Wed, 7 Jul 2021 15:22:36 +0300
Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:

> On Tue, 6 Jul 2021 17:05:45 -0700
> Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> > On Tue, 6 Jul 2021 20:17:57 +0300
> > Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:
> >   
> > > On Tue, 6 Jul 2021 09:18:21 -0700
> > > Stephen Hemminger <stephen@networkplumber.org> wrote:
> > >   
> > > > On Tue, 6 Jul 2021 18:44:15 +0300
> > > > Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:
> > > >     
> > > > > On Tue, 6 Jul 2021 08:34:07 -0700
> > > > > Stephen Hemminger <stephen@networkplumber.org> wrote:
> > > > >     
> > > > > > On Tue, 29 Jun 2021 18:51:15 +0300
> > > > > > Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:
> > > > > >       
> > > > > > > +	const struct rtnl_dump_filter_arg a[2] = {
> > > > > > > +		{ .filter = filter, .arg1 = arg1,
> > > > > > > +		  .errhndlr = errhndlr, .arg2 = arg2, .nc_flags = nc_flags, },
> > > > > > > +		{ .filter = NULL,   .arg1 = NULL,
> > > > > > > +		  .errhndlr = NULL, .arg2 = NULL, .nc_flags = 0, },
> > > > > > >  	};      
> > > > > > 
> > > > > > I am OK with this as is. But you don't need to add initializers for fields
> > > > > > that are 0/NULL (at least in C).      
> > > > > 
> > > > > Sure, I've made such explicit initializations just because in original
> > > > > rtnl_dump_filter_nc() we already have them.
> > > > > 
> > > > > Do I need to resend with fixed initializations? ;)    
> > > > 
> > > > Not worth it    
> > > 
> > > Ok, thanks!
> > >   
> > 
> > Looks like you need to send v5 anyway.
> > 
> > 
> > checkpatch.pl ~/Downloads/PATCHv4-iproute2-ip-route-ignore-ENOENT-during-save-if-RT_TABLE_MAIN-is-being-dumped.patch 
> > WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
> > #62: 
> > We started to use in-kernel filtering feature which allows to get only needed
> > 
> > WARNING: 'extened' may be misspelled - perhaps 'extended'?
> > #84: 
> > easily extened by changing SUPPRESS_ERRORS_INIT macro).
> >        ^^^^^^^
> > 
> > WARNING: please, no space before tabs
> > #111: FILE: include/libnetlink.h:109:
> > + * ^Irtnl_dump_done()$
> > 
> > WARNING: please, no space before tabs
> > #112: FILE: include/libnetlink.h:110:
> > + * ^Irtnl_dump_error()$
> > 
> > WARNING: please, no space before tabs
> > #116: FILE: include/libnetlink.h:114:
> > + * ^Ierror handled as usual$
> > 
> > WARNING: please, no space before tabs
> > #118: FILE: include/libnetlink.h:116:
> > + * ^Ierror in nlmsg_type == NLMSG_DONE will be suppressed$
> > 
> > 
> > WARNING: please, no space before tabs
> > #120: FILE: include/libnetlink.h:118:
> > + * ^Ierror in nlmsg_type == NLMSG_ERROR will be suppressed$
> > 
> > WARNING: please, no space before tabs
> > #121: FILE: include/libnetlink.h:119:
> > + * ^Iand nlmsg will be skipped$
> > 
> > total: 0 errors, 8 warnings, 183 lines checked  
> 
> Oh, sorry about that. I've sent v5 and checked with checkpatch.pl
> I've send two options for v5 - with initializers fix and without :)
> 
> Regards,
> Alex

Choose which ever initializer format you think looks best

Re: [PATCHv4 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Alexander Mikhalitsyn]

On Wed, 7 Jul 2021 07:31:31 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

> On Wed, 7 Jul 2021 15:22:36 +0300
> Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:
> 
> > On Tue, 6 Jul 2021 17:05:45 -0700
> > Stephen Hemminger <stephen@networkplumber.org> wrote:
> > 
> > > On Tue, 6 Jul 2021 20:17:57 +0300
> > > Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:
> > >   
> > > > On Tue, 6 Jul 2021 09:18:21 -0700
> > > > Stephen Hemminger <stephen@networkplumber.org> wrote:
> > > >   
> > > > > On Tue, 6 Jul 2021 18:44:15 +0300
> > > > > Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:
> > > > >     
> > > > > > On Tue, 6 Jul 2021 08:34:07 -0700
> > > > > > Stephen Hemminger <stephen@networkplumber.org> wrote:
> > > > > >     
> > > > > > > On Tue, 29 Jun 2021 18:51:15 +0300
> > > > > > > Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:
> > > > > > >       
> > > > > > > > +	const struct rtnl_dump_filter_arg a[2] = {
> > > > > > > > +		{ .filter = filter, .arg1 = arg1,
> > > > > > > > +		  .errhndlr = errhndlr, .arg2 = arg2, .nc_flags = nc_flags, },
> > > > > > > > +		{ .filter = NULL,   .arg1 = NULL,
> > > > > > > > +		  .errhndlr = NULL, .arg2 = NULL, .nc_flags = 0, },
> > > > > > > >  	};      
> > > > > > > 
> > > > > > > I am OK with this as is. But you don't need to add initializers for fields
> > > > > > > that are 0/NULL (at least in C).      
> > > > > > 
> > > > > > Sure, I've made such explicit initializations just because in original
> > > > > > rtnl_dump_filter_nc() we already have them.
> > > > > > 
> > > > > > Do I need to resend with fixed initializations? ;)    
> > > > > 
> > > > > Not worth it    
> > > > 
> > > > Ok, thanks!
> > > >   
> > > 
> > > Looks like you need to send v5 anyway.
> > > 
> > > 
> > > checkpatch.pl ~/Downloads/PATCHv4-iproute2-ip-route-ignore-ENOENT-during-save-if-RT_TABLE_MAIN-is-being-dumped.patch 
> > > WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
> > > #62: 
> > > We started to use in-kernel filtering feature which allows to get only needed
> > > 
> > > WARNING: 'extened' may be misspelled - perhaps 'extended'?
> > > #84: 
> > > easily extened by changing SUPPRESS_ERRORS_INIT macro).
> > >        ^^^^^^^
> > > 
> > > WARNING: please, no space before tabs
> > > #111: FILE: include/libnetlink.h:109:
> > > + * ^Irtnl_dump_done()$
> > > 
> > > WARNING: please, no space before tabs
> > > #112: FILE: include/libnetlink.h:110:
> > > + * ^Irtnl_dump_error()$
> > > 
> > > WARNING: please, no space before tabs
> > > #116: FILE: include/libnetlink.h:114:
> > > + * ^Ierror handled as usual$
> > > 
> > > WARNING: please, no space before tabs
> > > #118: FILE: include/libnetlink.h:116:
> > > + * ^Ierror in nlmsg_type == NLMSG_DONE will be suppressed$
> > > 
> > > 
> > > WARNING: please, no space before tabs
> > > #120: FILE: include/libnetlink.h:118:
> > > + * ^Ierror in nlmsg_type == NLMSG_ERROR will be suppressed$
> > > 
> > > WARNING: please, no space before tabs
> > > #121: FILE: include/libnetlink.h:119:
> > > + * ^Iand nlmsg will be skipped$
> > > 
> > > total: 0 errors, 8 warnings, 183 lines checked  
> > 
> > Oh, sorry about that. I've sent v5 and checked with checkpatch.pl
> > I've send two options for v5 - with initializers fix and without :)
> > 
> > Regards,
> > Alex
> 
> Choose which ever initializer format you think looks best

Ha-ha! :) Let's take with empty initializers ;)

Thanks,
Alex

Re: [PATCHv5 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Stephen Hemminger]

On Wed,  7 Jul 2021 15:22:01 +0300
Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:

> We started to use in-kernel filtering feature which allows to get only
> needed tables (see iproute_dump_filter()). From the kernel side it's
> implemented in net/ipv4/fib_frontend.c (inet_dump_fib), net/ipv6/ip6_fib.c
> (inet6_dump_fib). The problem here is that behaviour of "ip route save"
> was changed after
> c7e6371bc ("ip route: Add protocol, table id and device to dump request").
> If filters are used, then kernel returns ENOENT error if requested table
> is absent, but in newly created net namespace even RT_TABLE_MAIN table
> doesn't exist. It is really allocated, for instance, after issuing
> "ip l set lo up".
> 
> Reproducer is fairly simple:
> $ unshare -n ip route save > dump
> Error: ipv4: FIB table does not exist.
> Dump terminated
> 
> Expected result here is to get empty dump file (as it was before this
> change).
> 
> v2: reworked, so, now it takes into account NLMSGERR_ATTR_MSG
> (see nl_dump_ext_ack_done() function). We want to suppress error messages
> in stderr about absent FIB table from kernel too.
> 
> v3: reworked to make code clearer. Introduced rtnl_suppressed_errors(),
> rtnl_suppress_error() helpers. User may suppress up to 3 errors (may be
> easily extended by changing SUPPRESS_ERRORS_INIT macro).
> 
> v4: reworked, rtnl_dump_filter_errhndlr() was introduced. Thanks
> to Stephen Hemminger for comments and suggestions
> 
> v5: space fixes, commit message reformat, empty initializers
> 
> Fixes: c7e6371bc ("ip route: Add protocol, table id and device to dump request")
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Cc: Andrei Vagin <avagin@gmail.com>
> Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
> Signed-off-by: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>

Applied this version

Re: [PATCHv5 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Alexander Mikhalitsyn]

On Wed, 7 Jul 2021 07:35:05 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

> On Wed,  7 Jul 2021 15:22:01 +0300
> Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> wrote:
> 
> > We started to use in-kernel filtering feature which allows to get only
> > needed tables (see iproute_dump_filter()). From the kernel side it's
> > implemented in net/ipv4/fib_frontend.c (inet_dump_fib), net/ipv6/ip6_fib.c
> > (inet6_dump_fib). The problem here is that behaviour of "ip route save"
> > was changed after
> > c7e6371bc ("ip route: Add protocol, table id and device to dump request").
> > If filters are used, then kernel returns ENOENT error if requested table
> > is absent, but in newly created net namespace even RT_TABLE_MAIN table
> > doesn't exist. It is really allocated, for instance, after issuing
> > "ip l set lo up".
> > 
> > Reproducer is fairly simple:
> > $ unshare -n ip route save > dump
> > Error: ipv4: FIB table does not exist.
> > Dump terminated
> > 
> > Expected result here is to get empty dump file (as it was before this
> > change).
> > 
> > v2: reworked, so, now it takes into account NLMSGERR_ATTR_MSG
> > (see nl_dump_ext_ack_done() function). We want to suppress error messages
> > in stderr about absent FIB table from kernel too.
> > 
> > v3: reworked to make code clearer. Introduced rtnl_suppressed_errors(),
> > rtnl_suppress_error() helpers. User may suppress up to 3 errors (may be
> > easily extended by changing SUPPRESS_ERRORS_INIT macro).
> > 
> > v4: reworked, rtnl_dump_filter_errhndlr() was introduced. Thanks
> > to Stephen Hemminger for comments and suggestions
> > 
> > v5: space fixes, commit message reformat, empty initializers
> > 
> > Fixes: c7e6371bc ("ip route: Add protocol, table id and device to dump request")
> > Cc: David Ahern <dsahern@gmail.com>
> > Cc: Stephen Hemminger <stephen@networkplumber.org>
> > Cc: Andrei Vagin <avagin@gmail.com>
> > Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
> > Signed-off-by: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>
> 
> Applied this version

Stephen,

Thank you for your review and suggestions!

Alex

Re: [PATCHv5 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to iproute2/iproute2.git (refs/heads/main):

On Wed,  7 Jul 2021 15:22:01 +0300 you wrote:
> We started to use in-kernel filtering feature which allows to get only
> needed tables (see iproute_dump_filter()). From the kernel side it's
> implemented in net/ipv4/fib_frontend.c (inet_dump_fib), net/ipv6/ip6_fib.c
> (inet6_dump_fib). The problem here is that behaviour of "ip route save"
> was changed after
> c7e6371bc ("ip route: Add protocol, table id and device to dump request").
> If filters are used, then kernel returns ENOENT error if requested table
> is absent, but in newly created net namespace even RT_TABLE_MAIN table
> doesn't exist. It is really allocated, for instance, after issuing
> "ip l set lo up".
> 
> [...]

Here is the summary with links:
  - [PATCHv5,iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=459ce6e3d792

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

Re: [PATCHv5 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Roi Dayan]

On 2021-07-07 5:50 PM, patchwork-bot+netdevbpf@kernel.org wrote:
> Hello:
> 
> This patch was applied to iproute2/iproute2.git (refs/heads/main):
> 
> On Wed,  7 Jul 2021 15:22:01 +0300 you wrote:
>> We started to use in-kernel filtering feature which allows to get only
>> needed tables (see iproute_dump_filter()). From the kernel side it's
>> implemented in net/ipv4/fib_frontend.c (inet_dump_fib), net/ipv6/ip6_fib.c
>> (inet6_dump_fib). The problem here is that behaviour of "ip route save"
>> was changed after
>> c7e6371bc ("ip route: Add protocol, table id and device to dump request").
>> If filters are used, then kernel returns ENOENT error if requested table
>> is absent, but in newly created net namespace even RT_TABLE_MAIN table
>> doesn't exist. It is really allocated, for instance, after issuing
>> "ip l set lo up".
>>
>> [...]
> 
> Here is the summary with links:
>    - [PATCHv5,iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped
>      https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=459ce6e3d792
> 
> You are awesome, thank you!
> --
> Deet-doot-dot, I am a bot.
> https://korg.docs.kernel.org/patchwork/pwbot.html
> 
> 


Hi,

I didn't get a chance to check but after this commit the utility ss
is crashing from libnetlink.c

(gdb) bt
#0  0x0000000000000000 in ?? ()
#1  0x0000000000418980 in rtnl_dump_done (a=0x7fffffffdc00, h=0x449c80) 
at libnetlink.c:734
#2  rtnl_dump_filter_l (rth=rth@entry=0x7fffffffdcf0, 
arg=arg@entry=0x7fffffffdc00) at libnetlink.c:893
#3  0x00000000004197a2 in rtnl_dump_filter_nc 
(rth=rth@entry=0x7fffffffdcf0, filter=filter@entry=0x40cdf0 
<show_one_inet_sock>,
     arg1=arg1@entry=0x7fffffffdca0, nc_flags=nc_flags@entry=0) at 
libnetlink.c:957
#4  0x0000000000406dc7 in inet_show_netlink (dump_fp=dump_fp@entry=0x0, 
protocol=protocol@entry=255, f=0x42e900 <current_filter>) at ss.c:3638
#5  0x0000000000404db3 in raw_show (f=0x42e900 <current_filter>) at 
ss.c:3939


if I revert this commit I can run the ss utility ok.

Thanks,
Roi

Re: [PATCHv5 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Roi Dayan]

On 2021-07-11 1:59 PM, Roi Dayan wrote:
> 
> 
> On 2021-07-07 5:50 PM, patchwork-bot+netdevbpf@kernel.org wrote:
>> Hello:
>>
>> This patch was applied to iproute2/iproute2.git (refs/heads/main):
>>
>> On Wed,  7 Jul 2021 15:22:01 +0300 you wrote:
>>> We started to use in-kernel filtering feature which allows to get only
>>> needed tables (see iproute_dump_filter()). From the kernel side it's
>>> implemented in net/ipv4/fib_frontend.c (inet_dump_fib), 
>>> net/ipv6/ip6_fib.c
>>> (inet6_dump_fib). The problem here is that behaviour of "ip route save"
>>> was changed after
>>> c7e6371bc ("ip route: Add protocol, table id and device to dump 
>>> request").
>>> If filters are used, then kernel returns ENOENT error if requested table
>>> is absent, but in newly created net namespace even RT_TABLE_MAIN table
>>> doesn't exist. It is really allocated, for instance, after issuing
>>> "ip l set lo up".
>>>
>>> [...]
>>
>> Here is the summary with links:
>>    - [PATCHv5,iproute2] ip route: ignore ENOENT during save if 
>> RT_TABLE_MAIN is being dumped
>>      
>> https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=459ce6e3d792 
>>
>>
>> You are awesome, thank you!
>> -- 
>> Deet-doot-dot, I am a bot.
>> https://korg.docs.kernel.org/patchwork/pwbot.html
>>
>>
> 
> 
> Hi,
> 
> I didn't get a chance to check but after this commit the utility ss
> is crashing from libnetlink.c
> 
> (gdb) bt
> #0  0x0000000000000000 in ?? ()
> #1  0x0000000000418980 in rtnl_dump_done (a=0x7fffffffdc00, h=0x449c80) 
> at libnetlink.c:734
> #2  rtnl_dump_filter_l (rth=rth@entry=0x7fffffffdcf0, 
> arg=arg@entry=0x7fffffffdc00) at libnetlink.c:893
> #3  0x00000000004197a2 in rtnl_dump_filter_nc 
> (rth=rth@entry=0x7fffffffdcf0, filter=filter@entry=0x40cdf0 
> <show_one_inet_sock>,
>      arg1=arg1@entry=0x7fffffffdca0, nc_flags=nc_flags@entry=0) at 
> libnetlink.c:957
> #4  0x0000000000406dc7 in inet_show_netlink (dump_fp=dump_fp@entry=0x0, 
> protocol=protocol@entry=255, f=0x42e900 <current_filter>) at ss.c:3638
> #5  0x0000000000404db3 in raw_show (f=0x42e900 <current_filter>) at 
> ss.c:3939
> 
> 
> if I revert this commit I can run the ss utility ok.
> 
> Thanks,
> Roi

I found for me ss crashed using a->errhndlr but it could be null
in rtnl_dump_done()

I see a second usage of a->errhndlr in rtnl_dump_error()

Re: [PATCHv5 iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped

[Alexander Mihalicyn]

Thanks, Roi.

I will send a patch soon.

On Sun, Jul 11, 2021 at 2:09 PM Roi Dayan <roid@nvidia.com> wrote:
>
>
>
> On 2021-07-11 1:59 PM, Roi Dayan wrote:
> >
> >
> > On 2021-07-07 5:50 PM, patchwork-bot+netdevbpf@kernel.org wrote:
> >> Hello:
> >>
> >> This patch was applied to iproute2/iproute2.git (refs/heads/main):
> >>
> >> On Wed,  7 Jul 2021 15:22:01 +0300 you wrote:
> >>> We started to use in-kernel filtering feature which allows to get only
> >>> needed tables (see iproute_dump_filter()). From the kernel side it's
> >>> implemented in net/ipv4/fib_frontend.c (inet_dump_fib),
> >>> net/ipv6/ip6_fib.c
> >>> (inet6_dump_fib). The problem here is that behaviour of "ip route save"
> >>> was changed after
> >>> c7e6371bc ("ip route: Add protocol, table id and device to dump
> >>> request").
> >>> If filters are used, then kernel returns ENOENT error if requested table
> >>> is absent, but in newly created net namespace even RT_TABLE_MAIN table
> >>> doesn't exist. It is really allocated, for instance, after issuing
> >>> "ip l set lo up".
> >>>
> >>> [...]
> >>
> >> Here is the summary with links:
> >>    - [PATCHv5,iproute2] ip route: ignore ENOENT during save if
> >> RT_TABLE_MAIN is being dumped
> >>
> >> https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=459ce6e3d792
> >>
> >>
> >> You are awesome, thank you!
> >> --
> >> Deet-doot-dot, I am a bot.
> >> https://korg.docs.kernel.org/patchwork/pwbot.html
> >>
> >>
> >
> >
> > Hi,
> >
> > I didn't get a chance to check but after this commit the utility ss
> > is crashing from libnetlink.c
> >
> > (gdb) bt
> > #0  0x0000000000000000 in ?? ()
> > #1  0x0000000000418980 in rtnl_dump_done (a=0x7fffffffdc00, h=0x449c80)
> > at libnetlink.c:734
> > #2  rtnl_dump_filter_l (rth=rth@entry=0x7fffffffdcf0,
> > arg=arg@entry=0x7fffffffdc00) at libnetlink.c:893
> > #3  0x00000000004197a2 in rtnl_dump_filter_nc
> > (rth=rth@entry=0x7fffffffdcf0, filter=filter@entry=0x40cdf0
> > <show_one_inet_sock>,
> >      arg1=arg1@entry=0x7fffffffdca0, nc_flags=nc_flags@entry=0) at
> > libnetlink.c:957
> > #4  0x0000000000406dc7 in inet_show_netlink (dump_fp=dump_fp@entry=0x0,
> > protocol=protocol@entry=255, f=0x42e900 <current_filter>) at ss.c:3638
> > #5  0x0000000000404db3 in raw_show (f=0x42e900 <current_filter>) at
> > ss.c:3939
> >
> >
> > if I revert this commit I can run the ss utility ok.
> >
> > Thanks,
> > Roi
>
> I found for me ss crashed using a->errhndlr but it could be null
> in rtnl_dump_done()
>
> I see a second usage of a->errhndlr in rtnl_dump_error()
>
>