| Message ID | 20210629155116.23464-1-alexander.mikhalitsyn@virtuozzo.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [PATCHv4,iproute2] ip route: ignore ENOENT during save if RT_TABLE_MAIN is being dumped | expand |
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
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, }, { }, };
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
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
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!
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
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
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
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
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);
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(-)