Re: [PATCH net-next v4 1/5] netlink: extended ACK reporting
Tue, Apr 11, 2017 at 10:29:52AM CEST, johan...@sipsolutions.net wrote: >On Tue, 2017-04-11 at 10:13 +0200, Jiri Pirko wrote: >> Tue, Apr 11, 2017 at 09:29:18AM CEST, johan...@sipsolutions.net >> wrote: >> > On Tue, 2017-04-11 at 09:19 +0200, Jiri Pirko wrote: >> > > >> > > > + NUM_NLMSGERR_ATTRS, >> > > >> > > According to the rest of the uapi, this should be rather named: >> > > __NLMSGERR_ATTR_MAX >> > >> > nl80211 uses NUM_ so I guess that's a matter of convention, but I >> > can >> > change that I guess. >> >> Please do. > >No further comments on any other parts of the patch(es)? :) I didn't found anything aside the narrow descriptions- you have it like that in 4 out of 5 patches :) > >I don't want to be resending all the time, so I guess I'll wait. For >now, all code is in the nl-err branch in mac80211-next. > >johannes
Re: [PATCH net-next v4 1/5] netlink: extended ACK reporting
On Tue, 2017-04-11 at 10:13 +0200, Jiri Pirko wrote: > Tue, Apr 11, 2017 at 09:29:18AM CEST, johan...@sipsolutions.net > wrote: > > On Tue, 2017-04-11 at 09:19 +0200, Jiri Pirko wrote: > > > > > > > + NUM_NLMSGERR_ATTRS, > > > > > > According to the rest of the uapi, this should be rather named: > > > __NLMSGERR_ATTR_MAX > > > > nl80211 uses NUM_ so I guess that's a matter of convention, but I > > can > > change that I guess. > > Please do. No further comments on any other parts of the patch(es)? :) I don't want to be resending all the time, so I guess I'll wait. For now, all code is in the nl-err branch in mac80211-next. johannes
Re: [PATCH net-next v4 1/5] netlink: extended ACK reporting
Tue, Apr 11, 2017 at 09:29:18AM CEST, johan...@sipsolutions.net wrote: >On Tue, 2017-04-11 at 09:19 +0200, Jiri Pirko wrote: >> >> > + NUM_NLMSGERR_ATTRS, >> >> According to the rest of the uapi, this should be rather named: >> __NLMSGERR_ATTR_MAX > >nl80211 uses NUM_ so I guess that's a matter of convention, but I can >change that I guess. Please do. > >> >if (err || (nlh->nlmsg_flags & NLM_F_ACK)) >> > - netlink_ack(skb, nlh, err); >> > + netlink_ack(skb, nlh, err, NULL); >> >> Wouldn't it make sense to leave netlink_ack as is and add >> netlink_ack_ext for those who need to pass non-null? > >I thought about it, but didn't really see much point. The churn isn't >super big (a dozen callers or so), and I thought it makes sense to >point out to the users that there's something here. Makes sense.
Re: [PATCH net-next v4 1/5] netlink: extended ACK reporting
On Tue, 2017-04-11 at 09:19 +0200, Jiri Pirko wrote: > > > + NUM_NLMSGERR_ATTRS, > > According to the rest of the uapi, this should be rather named: > __NLMSGERR_ATTR_MAX nl80211 uses NUM_ so I guess that's a matter of convention, but I can change that I guess. > > if (err || (nlh->nlmsg_flags & NLM_F_ACK)) > > - netlink_ack(skb, nlh, err); > > + netlink_ack(skb, nlh, err, NULL); > > Wouldn't it make sense to leave netlink_ack as is and add > netlink_ack_ext for those who need to pass non-null? I thought about it, but didn't really see much point. The churn isn't super big (a dozen callers or so), and I thought it makes sense to point out to the users that there's something here. johannes
Re: [PATCH net-next v4 1/5] netlink: extended ACK reporting
Tue, Apr 11, 2017 at 08:56:56AM CEST, johan...@sipsolutions.net wrote:
>From: Johannes Berg
>
>Add the base infrastructure and UAPI for netlink
>extended ACK reporting. All "manual" calls to
>netlink_ack() pass NULL for now and thus don't
>get extended ACK reporting.
>
>Big thanks goes to Pablo Neira Ayuso for not only
>bringing up the whole topic at netconf (again) but
>also coming up with the nlattr passing trick and
>various other ideads.
>
>Signed-off-by: Johannes Berg
>---
[...]
>+/**
>+ * enum nlmsgerr_attrs - nlmsgerr attributes
>+ * @NLMSGERR_ATTR_UNUSED: unused
>+ * @NLMSGERR_ATTR_MSG: error message string (string)
>+ * @NLMSGERR_ATTR_OFFS: offset of the invalid attribute in the original
>+ * message, counting from the beginning of the header (u32)
>+ * @NUM_NLMSGERR_ATTRS: number of attributes
>+ * @NLMSGERR_ATTR_MAX: highest attribute number
>+ */
>+enum nlmsgerr_attrs {
>+ NLMSGERR_ATTR_UNUSED,
>+ NLMSGERR_ATTR_MSG,
>+ NLMSGERR_ATTR_OFFS,
>+
>+ NUM_NLMSGERR_ATTRS,
According to the rest of the uapi, this should be rather named:
__NLMSGERR_ATTR_MAX
>+ NLMSGERR_ATTR_MAX = NUM_NLMSGERR_ATTRS - 1
> };
>
> #define NETLINK_ADD_MEMBERSHIP1
>@@ -115,6 +146,7 @@ struct nlmsgerr {
> #define NETLINK_LISTEN_ALL_NSID 8
> #define NETLINK_LIST_MEMBERSHIPS 9
> #define NETLINK_CAP_ACK 10
>+#define NETLINK_EXT_ACK 11
>
> struct nl_pktinfo {
> __u32 group;
>diff --git a/kernel/audit.c b/kernel/audit.c
>index 2f4964cfde0b..d54bf5932374 100644
>--- a/kernel/audit.c
>+++ b/kernel/audit.c
>@@ -1402,7 +1402,7 @@ static void audit_receive_skb(struct sk_buff *skb)
> err = audit_receive_msg(skb, nlh);
> /* if err or if this message says it wants a response */
> if (err || (nlh->nlmsg_flags & NLM_F_ACK))
>- netlink_ack(skb, nlh, err);
>+ netlink_ack(skb, nlh, err, NULL);
Wouldn't it make sense to leave netlink_ack as is and add
netlink_ack_ext for those who need to pass non-null?
>
> nlh = nlmsg_next(nlh, &len);
> }
Re: [PATCH net-next v4 1/5] netlink: extended ACK reporting
Tue, Apr 11, 2017 at 08:56:56AM CEST, johan...@sipsolutions.net wrote:
>From: Johannes Berg
>
>Add the base infrastructure and UAPI for netlink
>extended ACK reporting. All "manual" calls to
>netlink_ack() pass NULL for now and thus don't
>get extended ACK reporting.
Johannes, I asked already why you are not using 80 cols. This
narrow descriptions look odd.
>
>Big thanks goes to Pablo Neira Ayuso for not only
>bringing up the whole topic at netconf (again) but
>also coming up with the nlattr passing trick and
>various other ideads.
>
>Signed-off-by: Johannes Berg
>---
> crypto/crypto_user.c | 3 +-
> drivers/infiniband/core/netlink.c | 5 +--
> drivers/scsi/scsi_netlink.c | 2 +-
> include/linux/netlink.h | 26 +-
> include/net/netlink.h | 3 +-
> include/uapi/linux/netlink.h | 32 ++
> kernel/audit.c| 2 +-
> net/core/rtnetlink.c | 3 +-
> net/core/sock_diag.c | 3 +-
> net/decnet/netfilter/dn_rtmsg.c | 2 +-
> net/hsr/hsr_netlink.c | 4 +--
> net/netfilter/ipset/ip_set_core.c | 2 +-
> net/netfilter/nfnetlink.c | 22 ++--
> net/netlink/af_netlink.c | 71 ++-
> net/netlink/af_netlink.h | 1 +
> net/netlink/genetlink.c | 3 +-
> net/xfrm/xfrm_user.c | 3 +-
> 17 files changed, 153 insertions(+), 34 deletions(-)
>
>diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
>index a90404a0c5ff..4a44830741c1 100644
>--- a/crypto/crypto_user.c
>+++ b/crypto/crypto_user.c
>@@ -483,7 +483,8 @@ static const struct crypto_link {
> [CRYPTO_MSG_DELRNG - CRYPTO_MSG_BASE] = { .doit = crypto_del_rng },
> };
>
>-static int crypto_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>+static int crypto_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
>+ struct netlink_ext_ack *extack)
> {
> struct nlattr *attrs[CRYPTOCFGA_MAX+1];
> const struct crypto_link *link;
>diff --git a/drivers/infiniband/core/netlink.c
>b/drivers/infiniband/core/netlink.c
>index 10469b0088b5..b784055423c8 100644
>--- a/drivers/infiniband/core/netlink.c
>+++ b/drivers/infiniband/core/netlink.c
>@@ -146,7 +146,8 @@ int ibnl_put_attr(struct sk_buff *skb, struct nlmsghdr
>*nlh,
> }
> EXPORT_SYMBOL(ibnl_put_attr);
>
>-static int ibnl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>+static int ibnl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
>+ struct netlink_ext_ack *extack)
> {
> struct ibnl_client *client;
> int type = nlh->nlmsg_type;
>@@ -209,7 +210,7 @@ static void ibnl_rcv_reply_skb(struct sk_buff *skb)
> if (nlh->nlmsg_flags & NLM_F_REQUEST)
> return;
>
>- ibnl_rcv_msg(skb, nlh);
>+ ibnl_rcv_msg(skb, nlh, NULL);
>
> msglen = NLMSG_ALIGN(nlh->nlmsg_len);
> if (msglen > skb->len)
>diff --git a/drivers/scsi/scsi_netlink.c b/drivers/scsi/scsi_netlink.c
>index 109802f776ed..50e624fb8307 100644
>--- a/drivers/scsi/scsi_netlink.c
>+++ b/drivers/scsi/scsi_netlink.c
>@@ -111,7 +111,7 @@ scsi_nl_rcv_msg(struct sk_buff *skb)
>
> next_msg:
> if ((err) || (nlh->nlmsg_flags & NLM_F_ACK))
>- netlink_ack(skb, nlh, err);
>+ netlink_ack(skb, nlh, err, NULL);
>
> skb_pull(skb, rlen);
> }
>diff --git a/include/linux/netlink.h b/include/linux/netlink.h
>index da14ab61f363..60e7137f840d 100644
>--- a/include/linux/netlink.h
>+++ b/include/linux/netlink.h
>@@ -62,11 +62,35 @@ netlink_kernel_create(struct net *net, int unit, struct
>netlink_kernel_cfg *cfg)
> return __netlink_kernel_create(net, unit, THIS_MODULE, cfg);
> }
>
>+/**
>+ * struct netlink_ext_ack - netlink extended ACK report struct
>+ * @_msg: message string to report - don't access directly, use
>+ *%NL_SET_ERR_MSG
>+ * @bad_attr: attribute with error
>+ */
>+struct netlink_ext_ack {
>+ const char *_msg;
>+ const struct nlattr *bad_attr;
>+};
>+
>+/* 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.)
>+ */
>+#define NL_SET_ERR_MSG(extack, msg) do { \
>+ static const char _msg[] = (msg); \
>+ \
>+ (extack)->_msg = _msg; \
>+} while (0)
>+
> extern void netlink_kernel_release(struct sock *sk);
> extern int __netlink_change_ngroups(struct sock *sk, unsigned int groups);
> extern int netlink_change_ngroups(struct sock *sk, unsigned int groups);
> extern void __netlink_clear_multicast_users(struct sock *sk, unsigned int
> group);
>-extern void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int
[PATCH net-next v4 1/5] netlink: extended ACK reporting
From: Johannes Berg
Add the base infrastructure and UAPI for netlink
extended ACK reporting. All "manual" calls to
netlink_ack() pass NULL for now and thus don't
get extended ACK reporting.
Big thanks goes to Pablo Neira Ayuso for not only
bringing up the whole topic at netconf (again) but
also coming up with the nlattr passing trick and
various other ideads.
Signed-off-by: Johannes Berg
---
crypto/crypto_user.c | 3 +-
drivers/infiniband/core/netlink.c | 5 +--
drivers/scsi/scsi_netlink.c | 2 +-
include/linux/netlink.h | 26 +-
include/net/netlink.h | 3 +-
include/uapi/linux/netlink.h | 32 ++
kernel/audit.c| 2 +-
net/core/rtnetlink.c | 3 +-
net/core/sock_diag.c | 3 +-
net/decnet/netfilter/dn_rtmsg.c | 2 +-
net/hsr/hsr_netlink.c | 4 +--
net/netfilter/ipset/ip_set_core.c | 2 +-
net/netfilter/nfnetlink.c | 22 ++--
net/netlink/af_netlink.c | 71 ++-
net/netlink/af_netlink.h | 1 +
net/netlink/genetlink.c | 3 +-
net/xfrm/xfrm_user.c | 3 +-
17 files changed, 153 insertions(+), 34 deletions(-)
diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
index a90404a0c5ff..4a44830741c1 100644
--- a/crypto/crypto_user.c
+++ b/crypto/crypto_user.c
@@ -483,7 +483,8 @@ static const struct crypto_link {
[CRYPTO_MSG_DELRNG - CRYPTO_MSG_BASE] = { .doit = crypto_del_rng },
};
-static int crypto_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
+static int crypto_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
+ struct netlink_ext_ack *extack)
{
struct nlattr *attrs[CRYPTOCFGA_MAX+1];
const struct crypto_link *link;
diff --git a/drivers/infiniband/core/netlink.c
b/drivers/infiniband/core/netlink.c
index 10469b0088b5..b784055423c8 100644
--- a/drivers/infiniband/core/netlink.c
+++ b/drivers/infiniband/core/netlink.c
@@ -146,7 +146,8 @@ int ibnl_put_attr(struct sk_buff *skb, struct nlmsghdr *nlh,
}
EXPORT_SYMBOL(ibnl_put_attr);
-static int ibnl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
+static int ibnl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
+ struct netlink_ext_ack *extack)
{
struct ibnl_client *client;
int type = nlh->nlmsg_type;
@@ -209,7 +210,7 @@ static void ibnl_rcv_reply_skb(struct sk_buff *skb)
if (nlh->nlmsg_flags & NLM_F_REQUEST)
return;
- ibnl_rcv_msg(skb, nlh);
+ ibnl_rcv_msg(skb, nlh, NULL);
msglen = NLMSG_ALIGN(nlh->nlmsg_len);
if (msglen > skb->len)
diff --git a/drivers/scsi/scsi_netlink.c b/drivers/scsi/scsi_netlink.c
index 109802f776ed..50e624fb8307 100644
--- a/drivers/scsi/scsi_netlink.c
+++ b/drivers/scsi/scsi_netlink.c
@@ -111,7 +111,7 @@ scsi_nl_rcv_msg(struct sk_buff *skb)
next_msg:
if ((err) || (nlh->nlmsg_flags & NLM_F_ACK))
- netlink_ack(skb, nlh, err);
+ netlink_ack(skb, nlh, err, NULL);
skb_pull(skb, rlen);
}
diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index da14ab61f363..60e7137f840d 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -62,11 +62,35 @@ netlink_kernel_create(struct net *net, int unit, struct
netlink_kernel_cfg *cfg)
return __netlink_kernel_create(net, unit, THIS_MODULE, cfg);
}
+/**
+ * struct netlink_ext_ack - netlink extended ACK report struct
+ * @_msg: message string to report - don't access directly, use
+ * %NL_SET_ERR_MSG
+ * @bad_attr: attribute with error
+ */
+struct netlink_ext_ack {
+ const char *_msg;
+ const struct nlattr *bad_attr;
+};
+
+/* 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.)
+ */
+#define NL_SET_ERR_MSG(extack, msg) do { \
+ static const char _msg[] = (msg); \
+ \
+ (extack)->_msg = _msg; \
+} while (0)
+
extern void netlink_kernel_release(struct sock *sk);
extern int __netlink_change_ngroups(struct sock *sk, unsigned int groups);
extern int netlink_change_ngroups(struct sock *sk, unsigned int groups);
extern void __netlink_clear_multicast_users(struct sock *sk, unsigned int
group);
-extern void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err);
+extern void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
+ const struct netlink_ext_ack *extack);
extern int netlink_has_listeners(struct sock *sk, unsigned int group);
extern int netlink_unicast(struct sock *ss
