| Message ID | 0e68f6e4a4fc68fd3f5f164b8d50a95fde6a1f50.1521227930.git.mleitner@redhat.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [RFC,iproute2] libnetlink: allow reading more than one message from extack | expand |
On Fri, 16 Mar 2018 16:23:09 -0300 Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > This patch introduces support for reading more than one message from > extack's and to adjust their level (warning/error) accordingly. > > Yes, there is a FIXME tag in the callback call for now. > > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> Make sense, can hold off until kernel supports warnings. > --- > lib/libnetlink.c | 55 ++++++++++++++++++++++++++++++++++++++++--------------- > 1 file changed, 40 insertions(+), 15 deletions(-) > > diff --git a/lib/libnetlink.c b/lib/libnetlink.c > index 928de1dd16d84b7802c06d0a659f5d73e1bbcb4b..7c4c81e11e02ea857888190eb5e7a9e99d159bb3 100644 > --- a/lib/libnetlink.c > +++ b/lib/libnetlink.c > @@ -43,9 +43,16 @@ static const enum mnl_attr_data_type extack_policy[NLMSGERR_ATTR_MAX + 1] = { > [NLMSGERR_ATTR_OFFS] = MNL_TYPE_U32, > }; > > +#define NETLINK_MAX_EXTACK_MSGS 8 Would rather not have fixed maximums > +struct extack_args { > + const struct nlattr *msg[NETLINK_MAX_EXTACK_MSGS]; > + const struct nlattr *offs; > + int msg_count; > +}; If you put msg[] last in structure, it could be variable length. > static int err_attr_cb(const struct nlattr *attr, void *data) > { > - const struct nlattr **tb = data; > + struct extack_args *tb = data; > uint16_t type; > > if (mnl_attr_type_valid(attr, NLMSGERR_ATTR_MAX) < 0) { > @@ -60,19 +67,23 @@ static int err_attr_cb(const struct nlattr *attr, void *data) > return MNL_CB_ERROR; > } > > - tb[type] = attr; > + if (type == NLMSGERR_ATTR_OFFS) > + tb->offs = attr; > + else if (tb->msg_count < NETLINK_MAX_EXTACK_MSGS) > + tb->msg[tb->msg_count++] = attr; > return MNL_CB_OK; > } > > /* dump netlink extended ack error message */ > int nl_dump_ext_ack(const struct nlmsghdr *nlh, nl_ext_ack_fn_t errfn) > { > - struct nlattr *tb[NLMSGERR_ATTR_MAX + 1] = {}; > + struct extack_args tb = {}; > const struct nlmsgerr *err = mnl_nlmsg_get_payload(nlh); > const struct nlmsghdr *err_nlh = NULL; > unsigned int hlen = sizeof(*err); > - const char *msg = NULL; > + const char *msg[NETLINK_MAX_EXTACK_MSGS] = {}; > uint32_t off = 0; > + int ret, i; > > /* no TLVs, nothing to do here */ > if (!(nlh->nlmsg_flags & NLM_F_ACK_TLVS)) > @@ -82,14 +93,14 @@ int nl_dump_ext_ack(const struct nlmsghdr *nlh, nl_ext_ack_fn_t errfn) > if (!(nlh->nlmsg_flags & NLM_F_CAPPED)) > hlen += mnl_nlmsg_get_payload_len(&err->msg); > > - if (mnl_attr_parse(nlh, hlen, err_attr_cb, tb) != MNL_CB_OK) > + if (mnl_attr_parse(nlh, hlen, err_attr_cb, &tb) != MNL_CB_OK) > return 0; > > - if (tb[NLMSGERR_ATTR_MSG]) > - msg = mnl_attr_get_str(tb[NLMSGERR_ATTR_MSG]); > + for (i = 0; i < NETLINK_MAX_EXTACK_MSGS && tb.msg[i]; i++) > + msg[i] = mnl_attr_get_str(tb.msg[i]); > > - if (tb[NLMSGERR_ATTR_OFFS]) { > - off = mnl_attr_get_u32(tb[NLMSGERR_ATTR_OFFS]); > + if (tb.offs) { > + off = mnl_attr_get_u32(tb.offs); > > if (off > nlh->nlmsg_len) { > fprintf(stderr, > @@ -100,21 +111,35 @@ int nl_dump_ext_ack(const struct nlmsghdr *nlh, nl_ext_ack_fn_t errfn) > } > > if (errfn) > - return errfn(msg, off, err_nlh); > + return errfn(*msg, off, err_nlh); /* FIXME */ > > - if (msg && *msg != '\0') { > + ret = 0; > + for (i = 0; i < NETLINK_MAX_EXTACK_MSGS && msg[i]; i++) { > bool is_err = !!err->error; > + const char *_msg = msg[i]; > + > + /* Message tagging has precedence. > + * KERN_WARNING = ASCII Start Of Header ('\001') + '4' > + */ > + if (!strncmp(_msg, "\0014", 2)) { > + is_err = false; > + _msg += 2; > + } If you are going to have an API that has levels, it must be the same as existing syslog kernel log format and maybe even get some code reuse. > + /* But we can't have Error if it didn't fail. */ > + if (is_err && !err->error) > + is_err = 0; > > fprintf(stderr, "%s: %s", > - is_err ? "Error" : "Warning", msg); > - if (msg[strlen(msg) - 1] != '.') > + is_err ? "Error" : "Warning", _msg); > + if (_msg[strlen(_msg) - 1] != '.') > fprintf(stderr, "."); > fprintf(stderr, "\n"); > > - return is_err ? 1 : 0; > + if (is_err) > + ret = 1; > } > > - return 0; > + return ret; > } > #else > #warning "libmnl required for error support"
On Mon, Mar 19, 2018 at 09:09:50AM -0700, Stephen Hemminger wrote: > On Fri, 16 Mar 2018 16:23:09 -0300 > Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > > > This patch introduces support for reading more than one message from > > extack's and to adjust their level (warning/error) accordingly. > > > > Yes, there is a FIXME tag in the callback call for now. > > > > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > > Make sense, can hold off until kernel supports warnings. Okay. > > > --- > > lib/libnetlink.c | 55 ++++++++++++++++++++++++++++++++++++++++--------------- > > 1 file changed, 40 insertions(+), 15 deletions(-) > > > > diff --git a/lib/libnetlink.c b/lib/libnetlink.c > > index 928de1dd16d84b7802c06d0a659f5d73e1bbcb4b..7c4c81e11e02ea857888190eb5e7a9e99d159bb3 100644 > > --- a/lib/libnetlink.c > > +++ b/lib/libnetlink.c > > @@ -43,9 +43,16 @@ static const enum mnl_attr_data_type extack_policy[NLMSGERR_ATTR_MAX + 1] = { > > [NLMSGERR_ATTR_OFFS] = MNL_TYPE_U32, > > }; > > > > +#define NETLINK_MAX_EXTACK_MSGS 8 > > Would rather not have fixed maximums Makes sense. Then it is more forward-compatible in case we increase that on kernel side later. > > > +struct extack_args { > > + const struct nlattr *msg[NETLINK_MAX_EXTACK_MSGS]; > > + const struct nlattr *offs; > > + int msg_count; > > +}; > > If you put msg[] last in structure, it could be variable length. It will also require a two-times parsing then because we don't have an attribute with the number of messages in there and we have to allocate it with a certain length. > > > static int err_attr_cb(const struct nlattr *attr, void *data) > > { > > - const struct nlattr **tb = data; > > + struct extack_args *tb = data; > > uint16_t type; > > > > if (mnl_attr_type_valid(attr, NLMSGERR_ATTR_MAX) < 0) { > > @@ -60,19 +67,23 @@ static int err_attr_cb(const struct nlattr *attr, void *data) > > return MNL_CB_ERROR; > > } > > > > - tb[type] = attr; > > + if (type == NLMSGERR_ATTR_OFFS) > > + tb->offs = attr; > > + else if (tb->msg_count < NETLINK_MAX_EXTACK_MSGS) > > + tb->msg[tb->msg_count++] = attr; > > return MNL_CB_OK; > > } > > > > /* dump netlink extended ack error message */ > > int nl_dump_ext_ack(const struct nlmsghdr *nlh, nl_ext_ack_fn_t errfn) > > { > > - struct nlattr *tb[NLMSGERR_ATTR_MAX + 1] = {}; > > + struct extack_args tb = {}; > > const struct nlmsgerr *err = mnl_nlmsg_get_payload(nlh); > > const struct nlmsghdr *err_nlh = NULL; > > unsigned int hlen = sizeof(*err); > > - const char *msg = NULL; > > + const char *msg[NETLINK_MAX_EXTACK_MSGS] = {}; > > uint32_t off = 0; > > + int ret, i; > > > > /* no TLVs, nothing to do here */ > > if (!(nlh->nlmsg_flags & NLM_F_ACK_TLVS)) > > @@ -82,14 +93,14 @@ int nl_dump_ext_ack(const struct nlmsghdr *nlh, nl_ext_ack_fn_t errfn) > > if (!(nlh->nlmsg_flags & NLM_F_CAPPED)) > > hlen += mnl_nlmsg_get_payload_len(&err->msg); > > > > - if (mnl_attr_parse(nlh, hlen, err_attr_cb, tb) != MNL_CB_OK) > > + if (mnl_attr_parse(nlh, hlen, err_attr_cb, &tb) != MNL_CB_OK) > > return 0; > > > > - if (tb[NLMSGERR_ATTR_MSG]) > > - msg = mnl_attr_get_str(tb[NLMSGERR_ATTR_MSG]); > > + for (i = 0; i < NETLINK_MAX_EXTACK_MSGS && tb.msg[i]; i++) > > + msg[i] = mnl_attr_get_str(tb.msg[i]); > > > > - if (tb[NLMSGERR_ATTR_OFFS]) { > > - off = mnl_attr_get_u32(tb[NLMSGERR_ATTR_OFFS]); > > + if (tb.offs) { > > + off = mnl_attr_get_u32(tb.offs); > > > > if (off > nlh->nlmsg_len) { > > fprintf(stderr, > > @@ -100,21 +111,35 @@ int nl_dump_ext_ack(const struct nlmsghdr *nlh, nl_ext_ack_fn_t errfn) > > } > > > > if (errfn) > > - return errfn(msg, off, err_nlh); > > + return errfn(*msg, off, err_nlh); /* FIXME */ > > > > - if (msg && *msg != '\0') { > > + ret = 0; > > + for (i = 0; i < NETLINK_MAX_EXTACK_MSGS && msg[i]; i++) { > > bool is_err = !!err->error; > > + const char *_msg = msg[i]; > > + > > + /* Message tagging has precedence. > > + * KERN_WARNING = ASCII Start Of Header ('\001') + '4' > > + */ > > + if (!strncmp(_msg, "\0014", 2)) { > > + is_err = false; > > + _msg += 2; > > + } > > If you are going to have an API that has levels, it must be the same > as existing syslog kernel log format and maybe even get some code reuse. It is the same representation already. The magic "\0014" in there is the same as KERN_WARNING. I'll check how other userspace applications are dealing with this, as I don't see KERN_WARNING being exported to userspace. Thanks, Marcelo > > > + /* But we can't have Error if it didn't fail. */ > > + if (is_err && !err->error) > > + is_err = 0; > > > > fprintf(stderr, "%s: %s", > > - is_err ? "Error" : "Warning", msg); > > - if (msg[strlen(msg) - 1] != '.') > > + is_err ? "Error" : "Warning", _msg); > > + if (_msg[strlen(_msg) - 1] != '.') > > fprintf(stderr, "."); > > fprintf(stderr, "\n"); > > > > - return is_err ? 1 : 0; > > + if (is_err) > > + ret = 1; > > } > > > > - return 0; > > + return ret; > > } > > #else > > #warning "libmnl required for error support" >
diff --git a/lib/libnetlink.c b/lib/libnetlink.c index 928de1dd16d84b7802c06d0a659f5d73e1bbcb4b..7c4c81e11e02ea857888190eb5e7a9e99d159bb3 100644 --- a/lib/libnetlink.c +++ b/lib/libnetlink.c @@ -43,9 +43,16 @@ static const enum mnl_attr_data_type extack_policy[NLMSGERR_ATTR_MAX + 1] = { [NLMSGERR_ATTR_OFFS] = MNL_TYPE_U32, }; +#define NETLINK_MAX_EXTACK_MSGS 8 +struct extack_args { + const struct nlattr *msg[NETLINK_MAX_EXTACK_MSGS]; + const struct nlattr *offs; + int msg_count; +}; + static int err_attr_cb(const struct nlattr *attr, void *data) { - const struct nlattr **tb = data; + struct extack_args *tb = data; uint16_t type; if (mnl_attr_type_valid(attr, NLMSGERR_ATTR_MAX) < 0) { @@ -60,19 +67,23 @@ static int err_attr_cb(const struct nlattr *attr, void *data) return MNL_CB_ERROR; } - tb[type] = attr; + if (type == NLMSGERR_ATTR_OFFS) + tb->offs = attr; + else if (tb->msg_count < NETLINK_MAX_EXTACK_MSGS) + tb->msg[tb->msg_count++] = attr; return MNL_CB_OK; } /* dump netlink extended ack error message */ int nl_dump_ext_ack(const struct nlmsghdr *nlh, nl_ext_ack_fn_t errfn) { - struct nlattr *tb[NLMSGERR_ATTR_MAX + 1] = {}; + struct extack_args tb = {}; const struct nlmsgerr *err = mnl_nlmsg_get_payload(nlh); const struct nlmsghdr *err_nlh = NULL; unsigned int hlen = sizeof(*err); - const char *msg = NULL; + const char *msg[NETLINK_MAX_EXTACK_MSGS] = {}; uint32_t off = 0; + int ret, i; /* no TLVs, nothing to do here */ if (!(nlh->nlmsg_flags & NLM_F_ACK_TLVS)) @@ -82,14 +93,14 @@ int nl_dump_ext_ack(const struct nlmsghdr *nlh, nl_ext_ack_fn_t errfn) if (!(nlh->nlmsg_flags & NLM_F_CAPPED)) hlen += mnl_nlmsg_get_payload_len(&err->msg); - if (mnl_attr_parse(nlh, hlen, err_attr_cb, tb) != MNL_CB_OK) + if (mnl_attr_parse(nlh, hlen, err_attr_cb, &tb) != MNL_CB_OK) return 0; - if (tb[NLMSGERR_ATTR_MSG]) - msg = mnl_attr_get_str(tb[NLMSGERR_ATTR_MSG]); + for (i = 0; i < NETLINK_MAX_EXTACK_MSGS && tb.msg[i]; i++) + msg[i] = mnl_attr_get_str(tb.msg[i]); - if (tb[NLMSGERR_ATTR_OFFS]) { - off = mnl_attr_get_u32(tb[NLMSGERR_ATTR_OFFS]); + if (tb.offs) { + off = mnl_attr_get_u32(tb.offs); if (off > nlh->nlmsg_len) { fprintf(stderr, @@ -100,21 +111,35 @@ int nl_dump_ext_ack(const struct nlmsghdr *nlh, nl_ext_ack_fn_t errfn) } if (errfn) - return errfn(msg, off, err_nlh); + return errfn(*msg, off, err_nlh); /* FIXME */ - if (msg && *msg != '\0') { + ret = 0; + for (i = 0; i < NETLINK_MAX_EXTACK_MSGS && msg[i]; i++) { bool is_err = !!err->error; + const char *_msg = msg[i]; + + /* Message tagging has precedence. + * KERN_WARNING = ASCII Start Of Header ('\001') + '4' + */ + if (!strncmp(_msg, "\0014", 2)) { + is_err = false; + _msg += 2; + } + /* But we can't have Error if it didn't fail. */ + if (is_err && !err->error) + is_err = 0; fprintf(stderr, "%s: %s", - is_err ? "Error" : "Warning", msg); - if (msg[strlen(msg) - 1] != '.') + is_err ? "Error" : "Warning", _msg); + if (_msg[strlen(_msg) - 1] != '.') fprintf(stderr, "."); fprintf(stderr, "\n"); - return is_err ? 1 : 0; + if (is_err) + ret = 1; } - return 0; + return ret; } #else #warning "libmnl required for error support"
This patch introduces support for reading more than one message from extack's and to adjust their level (warning/error) accordingly. Yes, there is a FIXME tag in the callback call for now. Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> --- lib/libnetlink.c | 55 ++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 40 insertions(+), 15 deletions(-)