| Message ID | 673abaddb26351826ca454f46d1271f1f4814c56.1521226621.git.marcelo.leitner@gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | Add support for warnings to extack | expand |
On 3/16/18 1:23 PM, Marcelo Ricardo Leitner wrote: > Currently extack can carry only a single message, which is usually the > error message. > > This imposes a limitation on a more verbose error reporting. For > example, it's not able to carry warning messages together with the error > message, or 2 warning messages. The only means for userspace to separate an error message from info or warnings is the error in nlmsgerr. If it is non-0, any extack message is considered an error else it is a warning. > > One use case is when dealing with tc offloading. If it failed to > offload, and also failed to install on software, it will report only > regarding the error about the software datapath, but the error from the > hardware path would also have been welcomed. > > This patch extends extack so it now can carry up to 8 messages and these > messages may be prefixed similarly to printk/pr_warning, so thus they > can be tagged either was warning or error. > > Fixed number of messages because supporting a dynamic limit seem to be > an overkill for the moment. Remember that this is not meant to be a > trace tool, but an error reporting one. > > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > --- > include/linux/netlink.h | 50 +++++++++++++++++++++++++++++------------------- > net/netlink/af_netlink.c | 12 +++++++----- > 2 files changed, 37 insertions(+), 25 deletions(-) > > diff --git a/include/linux/netlink.h b/include/linux/netlink.h > index f3075d6c7e8229c999ab650537f1e3b11e1f457b..d9780836cf263d4c436d732e9b7a8cde0739ac23 100644 > --- a/include/linux/netlink.h > +++ b/include/linux/netlink.h > @@ -71,43 +71,53 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg) > * @cookie: cookie data to return to userspace (for success) > * @cookie_len: actual cookie data length > */ > +#define NETLINK_MAX_EXTACK_MSGS 8 8 is way too many. If some change fails because of an error, why would a single error message not be enough? If it is a not an error, why is more than 1 warning message not enough? (I forget the details of the tc 'skip_sw' use case) > struct netlink_ext_ack { > - const char *_msg; > + const char *_msg[NETLINK_MAX_EXTACK_MSGS]; > const struct nlattr *bad_attr; > u8 cookie[NETLINK_MAX_COOKIE_LEN]; > u8 cookie_len; > + u8 _msg_count; > }; > > -/* Always use this macro, this allows later putting the > - * message into a separate section or such for things > - * like translation or listing all possible messages. > - * Currently string formatting is not supported (due > - * to the lack of an output buffer.) > +/* Always use these macros, this allows later putting > + * the message into a separate section or such for > + * things like translation or listing all possible > + * messages. Currently string formatting is not > + * supported (due to the lack of an output buffer.) > */ > -#define NL_SET_ERR_MSG(extack, msg) do { \ > - static const char __msg[] = msg; \ > - struct netlink_ext_ack *__extack = (extack); \ > - \ > - if (__extack) \ > - __extack->_msg = __msg; \ > +#define NL_SET_MSG(extack, msg) do { \ > + static const char __msg[] = msg; \ > + struct netlink_ext_ack *__extack = (extack); \ > + \ > + if (__extack && \ > + !WARN_ON(__extack->_msg_count >= NETLINK_MAX_EXTACK_MSGS)) \ > + __extack->_msg[__extack->_msg_count++] = __msg; \ > } while (0) > > +#define NL_SET_ERR_MSG(extack, msg) NL_SET_MSG(extack, msg) > +#define NL_SET_WARN_MSG(extack, msg) NL_SET_MSG(extack, KERN_WARNING msg) > + > #define NL_SET_ERR_MSG_MOD(extack, msg) \ > NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " msg) > +#define NL_SET_WARN_MSG_MOD(extack, msg) \ > + NL_SET_WARN_MSG((extack), KBUILD_MODNAME ": " msg) > + Adding separate macros for error versus warning is confusing since from an extack perspective a message is a message and there is no uapi to separate them.
On Sun, Mar 18, 2018 at 10:11:20AM -0600, David Ahern wrote: > On 3/16/18 1:23 PM, Marcelo Ricardo Leitner wrote: > > Currently extack can carry only a single message, which is usually the > > error message. > > > > This imposes a limitation on a more verbose error reporting. For > > example, it's not able to carry warning messages together with the error > > message, or 2 warning messages. > > > The only means for userspace to separate an error message from info or > warnings is the error in nlmsgerr. If it is non-0, any extack message is > considered an error else it is a warning. I don't see your point here. The proposed patch extends what you said to: - include warnings on error reports - allow more than 1 message With the proposed patch, if nlmsgerr is 0 all messages are considered as warnings. If it's non-zero, some may be marked as warnings. AFAICT it is not far from what you described, and still honouring the main knob, mlmsgerr. > > > > > > One use case is when dealing with tc offloading. If it failed to > > offload, and also failed to install on software, it will report only > > regarding the error about the software datapath, but the error from the > > hardware path would also have been welcomed. > > > > This patch extends extack so it now can carry up to 8 messages and these > > messages may be prefixed similarly to printk/pr_warning, so thus they > > can be tagged either was warning or error. > > > > Fixed number of messages because supporting a dynamic limit seem to be > > an overkill for the moment. Remember that this is not meant to be a > > trace tool, but an error reporting one. > > > > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > > --- > > include/linux/netlink.h | 50 +++++++++++++++++++++++++++++------------------- > > net/netlink/af_netlink.c | 12 +++++++----- > > 2 files changed, 37 insertions(+), 25 deletions(-) > > > > diff --git a/include/linux/netlink.h b/include/linux/netlink.h > > index f3075d6c7e8229c999ab650537f1e3b11e1f457b..d9780836cf263d4c436d732e9b7a8cde0739ac23 100644 > > --- a/include/linux/netlink.h > > +++ b/include/linux/netlink.h > > @@ -71,43 +71,53 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg) > > * @cookie: cookie data to return to userspace (for success) > > * @cookie_len: actual cookie data length > > */ > > +#define NETLINK_MAX_EXTACK_MSGS 8 > > 8 is way too many. If some change fails because of an error, why would a Ok. I'm fine with 4 for now. > single error message not be enough? If it is a not an error, why is more > than 1 warning message not enough? (I forget the details of the tc > 'skip_sw' use case) Because 1 message assumes you have a simple and linear code path being executed and that it aborts on the first error it encounters. You could, for example, report several bad arguments at once instead of having the user to fix & retry until he gets it all right. You can also have situations like: Warning: option foo is useless with option bar specified. (as in: the rule may not work as you intended) Error: failed to allocate a new node. The goal is to be able to deliver more information to the user/software using netlink and be able to log it, for later analysis. If you have a look at mlx5/core/en_tc.c, there are several printk(KERN_WARNING or pr_warn calls that should be getting returned via extack and not to kernel dmesg. That's just one domain and it is not aware if there is already a message stored in extack or if there will be another one later. > > > > struct netlink_ext_ack { > > - const char *_msg; > > + const char *_msg[NETLINK_MAX_EXTACK_MSGS]; > > const struct nlattr *bad_attr; > > u8 cookie[NETLINK_MAX_COOKIE_LEN]; > > u8 cookie_len; > > + u8 _msg_count; > > }; > > > > -/* Always use this macro, this allows later putting the > > - * message into a separate section or such for things > > - * like translation or listing all possible messages. > > - * Currently string formatting is not supported (due > > - * to the lack of an output buffer.) > > +/* Always use these macros, this allows later putting > > + * the message into a separate section or such for > > + * things like translation or listing all possible > > + * messages. Currently string formatting is not > > + * supported (due to the lack of an output buffer.) > > */ > > -#define NL_SET_ERR_MSG(extack, msg) do { \ > > - static const char __msg[] = msg; \ > > - struct netlink_ext_ack *__extack = (extack); \ > > - \ > > - if (__extack) \ > > - __extack->_msg = __msg; \ > > +#define NL_SET_MSG(extack, msg) do { \ > > + static const char __msg[] = msg; \ > > + struct netlink_ext_ack *__extack = (extack); \ > > + \ > > + if (__extack && \ > > + !WARN_ON(__extack->_msg_count >= NETLINK_MAX_EXTACK_MSGS)) \ > > + __extack->_msg[__extack->_msg_count++] = __msg; \ > > } while (0) > > > > +#define NL_SET_ERR_MSG(extack, msg) NL_SET_MSG(extack, msg) > > +#define NL_SET_WARN_MSG(extack, msg) NL_SET_MSG(extack, KERN_WARNING msg) > > + > > #define NL_SET_ERR_MSG_MOD(extack, msg) \ > > NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " msg) > > +#define NL_SET_WARN_MSG_MOD(extack, msg) \ > > + NL_SET_WARN_MSG((extack), KBUILD_MODNAME ": " msg) > > + > > Adding separate macros for error versus warning is confusing since from > an extack perspective a message is a message and there is no uapi to > separate them. Are you saying the markings at beginning of the messages are not possible? If that's the case, we probably can think of something else, as I see value in being able to deliver warnings together with errors. M.
On 3/18/18 12:19 PM, Marcelo Ricardo Leitner wrote: > On Sun, Mar 18, 2018 at 10:11:20AM -0600, David Ahern wrote: >> On 3/16/18 1:23 PM, Marcelo Ricardo Leitner wrote: >>> Currently extack can carry only a single message, which is usually the >>> error message. >>> >>> This imposes a limitation on a more verbose error reporting. For >>> example, it's not able to carry warning messages together with the error >>> message, or 2 warning messages. >> >> >> The only means for userspace to separate an error message from info or >> warnings is the error in nlmsgerr. If it is non-0, any extack message is >> considered an error else it is a warning. > > I don't see your point here. > > The proposed patch extends what you said to: > - include warnings on error reports > - allow more than 1 message > > With the proposed patch, if nlmsgerr is 0 all messages are considered > as warnings. If it's non-zero, some may be marked as warnings. It's the 'some' that I was referring to, but ... >>> +#define NL_SET_ERR_MSG(extack, msg) NL_SET_MSG(extack, msg) >>> +#define NL_SET_WARN_MSG(extack, msg) NL_SET_MSG(extack, KERN_WARNING msg) >>> + >>> #define NL_SET_ERR_MSG_MOD(extack, msg) \ >>> NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " msg) >>> +#define NL_SET_WARN_MSG_MOD(extack, msg) \ >>> + NL_SET_WARN_MSG((extack), KBUILD_MODNAME ": " msg) >>> + >> >> Adding separate macros for error versus warning is confusing since from >> an extack perspective a message is a message and there is no uapi to >> separate them. > > Are you saying the markings at beginning of the messages are not > possible? If that's the case, we probably can think of something else, > as I see value in being able to deliver warnings together with errors. ... I did miss the KERN_WARNIN above. That means that warning messages are prefixed by 0x1 (KERN_SOH) and "4" (warning loglevel). There will be cases missed by iproute2 as current versions won't catch the 2 new characters. Converting code to be able to continue generating error messages yet ultimately fail seems overly complex to me. I get the intent of returning as much info as possible, but most of that feels (e.g., in the mlx5 example you referenced) like someone learning how to do something the first time in which case 1 at a time errors seems reasonable - in fact might drive home some lessons. ;-)
On Sun, Mar 18, 2018 at 10:27:00PM -0600, David Ahern wrote: > On 3/18/18 12:19 PM, Marcelo Ricardo Leitner wrote: > > On Sun, Mar 18, 2018 at 10:11:20AM -0600, David Ahern wrote: > >> On 3/16/18 1:23 PM, Marcelo Ricardo Leitner wrote: > >>> Currently extack can carry only a single message, which is usually the > >>> error message. > >>> > >>> This imposes a limitation on a more verbose error reporting. For > >>> example, it's not able to carry warning messages together with the error > >>> message, or 2 warning messages. > >> > >> > >> The only means for userspace to separate an error message from info or > >> warnings is the error in nlmsgerr. If it is non-0, any extack message is > >> considered an error else it is a warning. > > > > I don't see your point here. > > > > The proposed patch extends what you said to: > > - include warnings on error reports > > - allow more than 1 message > > > > With the proposed patch, if nlmsgerr is 0 all messages are considered > > as warnings. If it's non-zero, some may be marked as warnings. > > It's the 'some' that I was referring to, but ... > > > >>> +#define NL_SET_ERR_MSG(extack, msg) NL_SET_MSG(extack, msg) > >>> +#define NL_SET_WARN_MSG(extack, msg) NL_SET_MSG(extack, KERN_WARNING msg) > >>> + > >>> #define NL_SET_ERR_MSG_MOD(extack, msg) \ > >>> NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " msg) > >>> +#define NL_SET_WARN_MSG_MOD(extack, msg) \ > >>> + NL_SET_WARN_MSG((extack), KBUILD_MODNAME ": " msg) > >>> + > >> > >> Adding separate macros for error versus warning is confusing since from > >> an extack perspective a message is a message and there is no uapi to > >> separate them. > > > > Are you saying the markings at beginning of the messages are not > > possible? If that's the case, we probably can think of something else, > > as I see value in being able to deliver warnings together with errors. > > ... I did miss the KERN_WARNIN above. That means that warning messages > are prefixed by 0x1 (KERN_SOH) and "4" (warning loglevel). There will be > cases missed by iproute2 as current versions won't catch the 2 new > characters. The first one is not printable, so it would print a weird '4' at the beginning of the message. But: only if it didn't have any error message later, because old iproute will display only the last message (and error messages are not tagged). > > Converting code to be able to continue generating error messages yet > ultimately fail seems overly complex to me. I get the intent of > returning as much info as possible, but most of that feels (e.g., in the > mlx5 example you referenced) like someone learning how to do something > the first time in which case 1 at a time errors seems reasonable - in > fact might drive home some lessons. ;-) That is true. Yep, I'm still lacking a real user for it. Maybe with the patchset split it will come up. M.
diff --git a/include/linux/netlink.h b/include/linux/netlink.h index f3075d6c7e8229c999ab650537f1e3b11e1f457b..d9780836cf263d4c436d732e9b7a8cde0739ac23 100644 --- a/include/linux/netlink.h +++ b/include/linux/netlink.h @@ -71,43 +71,53 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg) * @cookie: cookie data to return to userspace (for success) * @cookie_len: actual cookie data length */ +#define NETLINK_MAX_EXTACK_MSGS 8 struct netlink_ext_ack { - const char *_msg; + const char *_msg[NETLINK_MAX_EXTACK_MSGS]; const struct nlattr *bad_attr; u8 cookie[NETLINK_MAX_COOKIE_LEN]; u8 cookie_len; + u8 _msg_count; }; -/* Always use this macro, this allows later putting the - * message into a separate section or such for things - * like translation or listing all possible messages. - * Currently string formatting is not supported (due - * to the lack of an output buffer.) +/* Always use these macros, this allows later putting + * the message into a separate section or such for + * things like translation or listing all possible + * messages. Currently string formatting is not + * supported (due to the lack of an output buffer.) */ -#define NL_SET_ERR_MSG(extack, msg) do { \ - static const char __msg[] = msg; \ - struct netlink_ext_ack *__extack = (extack); \ - \ - if (__extack) \ - __extack->_msg = __msg; \ +#define NL_SET_MSG(extack, msg) do { \ + static const char __msg[] = msg; \ + struct netlink_ext_ack *__extack = (extack); \ + \ + if (__extack && \ + !WARN_ON(__extack->_msg_count >= NETLINK_MAX_EXTACK_MSGS)) \ + __extack->_msg[__extack->_msg_count++] = __msg; \ } while (0) +#define NL_SET_ERR_MSG(extack, msg) NL_SET_MSG(extack, msg) +#define NL_SET_WARN_MSG(extack, msg) NL_SET_MSG(extack, KERN_WARNING msg) + #define NL_SET_ERR_MSG_MOD(extack, msg) \ NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " msg) +#define NL_SET_WARN_MSG_MOD(extack, msg) \ + NL_SET_WARN_MSG((extack), KBUILD_MODNAME ": " msg) + #define NL_SET_BAD_ATTR(extack, attr) do { \ if ((extack)) \ (extack)->bad_attr = (attr); \ } while (0) -#define NL_SET_ERR_MSG_ATTR(extack, attr, msg) do { \ - static const char __msg[] = msg; \ - struct netlink_ext_ack *__extack = (extack); \ - \ - if (__extack) { \ - __extack->_msg = __msg; \ - __extack->bad_attr = (attr); \ - } \ +#define NL_SET_ERR_MSG_ATTR(extack, attr, msg) do { \ + static const char __msg[] = msg; \ + struct netlink_ext_ack *__extack = (extack); \ + \ + if (__extack) { \ + if (!WARN_ON(__extack->_msg_count >= NETLINK_MAX_EXTACK_MSGS)) \ + __extack->_msg[__extack->_msg_count++] = __msg; \ + __extack->bad_attr = (attr); \ + } \ } while (0) extern void netlink_kernel_release(struct sock *sk); diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index 5d10dcfe6411e745e2abcda925fe7b6fabdac0fc..71b4ece5c760979e082b0c644af9d2f222e1b721 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -2350,13 +2350,15 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err, struct netlink_sock *nlk = nlk_sk(NETLINK_CB(in_skb).sk); unsigned int flags = 0; bool nlk_has_extack = nlk->flags & NETLINK_F_EXT_ACK; + int i; /* Error messages get the original request appened, unless the user * requests to cap the error message, and get extra error data if * requested. */ - if (nlk_has_extack && extack && extack->_msg) - tlvlen += nla_total_size(strlen(extack->_msg) + 1); + if (nlk_has_extack && extack) + for (i = 0; i < extack->_msg_count; i++) + tlvlen += nla_total_size(strlen(extack->_msg[i]) + 1); if (err) { if (!(nlk->flags & NETLINK_F_CAP_ACK)) @@ -2389,10 +2391,10 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err, memcpy(&errmsg->msg, nlh, payload > sizeof(*errmsg) ? nlh->nlmsg_len : sizeof(*nlh)); if (nlk_has_extack && extack) { - if (extack->_msg) { + for (i = 0; i < extack->_msg_count; i++) WARN_ON(nla_put_string(skb, NLMSGERR_ATTR_MSG, - extack->_msg)); - } + extack->_msg[i])); + if (err) { if (extack->bad_attr && !WARN_ON((u8 *)extack->bad_attr < in_skb->data ||
Currently extack can carry only a single message, which is usually the error message. This imposes a limitation on a more verbose error reporting. For example, it's not able to carry warning messages together with the error message, or 2 warning messages. One use case is when dealing with tc offloading. If it failed to offload, and also failed to install on software, it will report only regarding the error about the software datapath, but the error from the hardware path would also have been welcomed. This patch extends extack so it now can carry up to 8 messages and these messages may be prefixed similarly to printk/pr_warning, so thus they can be tagged either was warning or error. Fixed number of messages because supporting a dynamic limit seem to be an overkill for the moment. Remember that this is not meant to be a trace tool, but an error reporting one. Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> --- include/linux/netlink.h | 50 +++++++++++++++++++++++++++++------------------- net/netlink/af_netlink.c | 12 +++++++----- 2 files changed, 37 insertions(+), 25 deletions(-)