[PATCH 1/2] ss: fix the incorrect value of total UNIX_DIAG_* number

[PATCH 1/2] ss: fix the incorrect value of total UNIX_DIAG_* number

[Shan Wei]

From: Shan Wei <davidshan@tencent.com>

UNIX_DIAG_MAX is included in enum type.
It is equal to the total number of enum element.

But lots of enum MAX value is defined as the max enum element, e.g. INET_DIAG_MAX, XFRMA_MAX. 
The right fixing way seems to define UNIX_DIAG_MAX as UNIX_DIAG_MEMINFO,
but this way will break other user application.

So, just fix it on user application.


Signed-off-by: Shan Wei <davidshan@tencent.com>
---
 misc/ss.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/misc/ss.c b/misc/ss.c
index 4017918..5f70a26 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -2011,12 +2011,12 @@ void unix_list_print(struct unixstat *list, struct filter *f)
 static int unix_show_sock(struct nlmsghdr *nlh, struct filter *f)
 {
 	struct unix_diag_msg *r = NLMSG_DATA(nlh);
-	struct rtattr *tb[UNIX_DIAG_MAX+1];
+	struct rtattr *tb[UNIX_DIAG_MAX];
 	char name[128];
 	int peer_ino;
 	int rqlen;
 
-	parse_rtattr(tb, UNIX_DIAG_MAX, (struct rtattr*)(r+1),
+	parse_rtattr(tb, UNIX_DIAG_MAX-1, (struct rtattr*)(r+1),
 		     nlh->nlmsg_len - NLMSG_LENGTH(sizeof(*r)));
 
 	if (netid_width)
-- 
1.7.1

Re: [PATCH 1/2] ss: fix the incorrect value of total UNIX_DIAG_* number

[Stephen Hemminger]

On Wed, 25 Apr 2012 15:15:16 +0800
Shan Wei <shanwei88@gmail.com> wrote:

> From: Shan Wei <davidshan@tencent.com>
> 
> UNIX_DIAG_MAX is included in enum type.
> It is equal to the total number of enum element.
> 
> But lots of enum MAX value is defined as the max enum element, e.g. INET_DIAG_MAX, XFRMA_MAX. 
> The right fixing way seems to define UNIX_DIAG_MAX as UNIX_DIAG_MEMINFO,
> but this way will break other user application.
> 
> So, just fix it on user application.

Nak, we should fix this in the kernel. It is ridiculous to have
a convention that is true for one route attribute type but not
for all of them.

[PATCH] unix_diag: use netlink attribute MAX convention

[Stephen Hemminger]

Use the standard convention to define the number of elements
in unix diag attribute. This fixes future problems like the fact
the last element (MEMINFO) is not parsed by current iproute2 ss command.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/include/linux/unix_diag.h	2012-02-13 09:23:58.830508614 -0800
+++ b/include/linux/unix_diag.h	2012-04-25 09:36:20.752056963 -0700
@@ -37,9 +37,9 @@ enum {
 	UNIX_DIAG_ICONS,
 	UNIX_DIAG_RQLEN,
 	UNIX_DIAG_MEMINFO,
-
-	UNIX_DIAG_MAX,
+	__UNIX_DIAG_MAX
 };
+#define UNIX_DIAG_MAX (__UNIX_DIAG_MAX - 1)
 
 struct unix_diag_vfs {
 	__u32	udiag_vfs_ino;

Re: [PATCH] unix_diag: use netlink attribute MAX convention

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 25 Apr 2012 10:38:58 -0700

> Use the standard convention to define the number of elements
> in unix diag attribute. This fixes future problems like the fact
> the last element (MEMINFO) is not parsed by current iproute2 ss command.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

You can't change this, it's already out in the wild, and
for 2 releases.

Sorry.

Just accept the 'ss' patch that was posted and you objected to,
I'm not breaking userspace just so that you don't have to apply
a userland patch you perceive as an unacceptable quirk.

Re: [PATCH] unix_diag: use netlink attribute MAX convention

[Stephen Hemminger]

On Wed, 25 Apr 2012 14:16:58 -0400 (EDT)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Wed, 25 Apr 2012 10:38:58 -0700
> 
> > Use the standard convention to define the number of elements
> > in unix diag attribute. This fixes future problems like the fact
> > the last element (MEMINFO) is not parsed by current iproute2 ss command.
> > 
> > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> You can't change this, it's already out in the wild, and
> for 2 releases.
> 
> Sorry.
> 
> Just accept the 'ss' patch that was posted and you objected to,
> I'm not breaking userspace just so that you don't have to apply
> a userland patch you perceive as an unacceptable quirk.

It only changes a broken enum value used only by iproute2.
Do you expect the one utility to use it to have a workaround for
a broken initial version.

Re: [PATCH] unix_diag: use netlink attribute MAX convention

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 25 Apr 2012 11:56:09 -0700

> Do you expect the one utility to use it to have a workaround for
> a broken initial version.

I'm not taking that risk.

They thought the same exact thing when they did the autofs struct size
compat fix, and it turned out to break things.

Stephen I have an awesome suggestion for you if you want to avoid this
in the future, review iproute2 patches more aggressively so you can
catch things like this earlier.  Like, when we can actually still
safely change things.

Because currently you let patches rot in patchwork.  There's an
iproute2 patch in there assigned to you which is 3 months old, that
simply isn't how this is supposed to work.

I hate to keep beating a dead horse, but you don't stay on top of
patchwork like you should.  The object is not to let patches just
rot in "Under Review" state for months.

Either you apply them as soon as possible, or you mark them
appropriately as "Changes Requested" or "Deferred" so that the
submitter makes appropriate fixes you've asked for, or resubmits when
it's more appropriate for the change to go in.

"Under Review" doesn't mean, "I'm waiting for a kernel release with
the feature".  But that's how you use it.

Re: [PATCH] unix_diag: use netlink attribute MAX convention

[Stephen Hemminger]

On Wed, 25 Apr 2012 15:07:16 -0400 (EDT)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Wed, 25 Apr 2012 11:56:09 -0700
> 
> > Do you expect the one utility to use it to have a workaround for
> > a broken initial version.
> 
> I'm not taking that risk.
> 
> They thought the same exact thing when they did the autofs struct size
> compat fix, and it turned out to break things.
> 
> Stephen I have an awesome suggestion for you if you want to avoid this
> in the future, review iproute2 patches more aggressively so you can
> catch things like this earlier.  Like, when we can actually still
> safely change things.

Sorry it was more of cross project issue in this case. The original
kernel patch had the problem and was lost in the fog of the other
issues like the unix diag implementation not building.

A community works best if multiple people look at the code.
Don't think I would have spotted it unless I compared it to
other places.


> Because currently you let patches rot in patchwork.  There's an
> iproute2 patch in there assigned to you which is 3 months old, that
> simply isn't how this is supposed to work.
> 
> I hate to keep beating a dead horse, but you don't stay on top of
> patchwork like you should.  The object is not to let patches just
> rot in "Under Review" state for months.

I keep patches that are for -next in that state.

> Either you apply them as soon as possible, or you mark them
> appropriately as "Changes Requested" or "Deferred" so that the
> submitter makes appropriate fixes you've asked for, or resubmits when
> it's more appropriate for the change to go in.
> 
> "Under Review" doesn't mean, "I'm waiting for a kernel release with
> the feature".  But that's how you use it.

Ok. What is the suggested tag for that.

Re: [PATCH] unix_diag: use netlink attribute MAX convention

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 25 Apr 2012 12:47:43 -0700

> On Wed, 25 Apr 2012 15:07:16 -0400 (EDT)
> David Miller <davem@davemloft.net> wrote:
> 
>> Either you apply them as soon as possible, or you mark them
>> appropriately as "Changes Requested" or "Deferred" so that the
>> submitter makes appropriate fixes you've asked for, or resubmits when
>> it's more appropriate for the change to go in.
>> 
>> "Under Review" doesn't mean, "I'm waiting for a kernel release with
>> the feature".  But that's how you use it.
> 
> Ok. What is the suggested tag for that.

As I said above, "Deferred", with a note sent to the submitter to resubmit
the patch at the appropriate time.

Re: [PATCH] unix_diag: use netlink attribute MAX convention

[Stephen Hemminger]

On Wed, 25 Apr 2012 15:57:17 -0400 (EDT)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Wed, 25 Apr 2012 12:47:43 -0700
> 
> > On Wed, 25 Apr 2012 15:07:16 -0400 (EDT)
> > David Miller <davem@davemloft.net> wrote:
> > 
> >> Either you apply them as soon as possible, or you mark them
> >> appropriately as "Changes Requested" or "Deferred" so that the
> >> submitter makes appropriate fixes you've asked for, or resubmits when
> >> it's more appropriate for the change to go in.
> >> 
> >> "Under Review" doesn't mean, "I'm waiting for a kernel release with
> >> the feature".  But that's how you use it.
> > 
> > Ok. What is the suggested tag for that.
> 
> As I said above, "Deferred", with a note sent to the submitter to resubmit
> the patch at the appropriate time.

That doesn't work, not asking original submitter to resubmit.
I tried a branch, but git doesn't like branches coming and going
on remote repos.
Probably better to just have an ephemeral iproute2-next repo on kernel.org.

Re: [PATCH] unix_diag: use netlink attribute MAX convention

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 25 Apr 2012 13:09:06 -0700

> That doesn't work, not asking original submitter to resubmit.

It works every single time for me.

If they don't resubmit, they don't really care about their work being
included, and therefore neither should you.

End of story.

Re: [PATCH 1/2] ss: fix the incorrect value of total UNIX_DIAG_* number

[Stephen Hemminger]

On Wed, 25 Apr 2012 15:15:16 +0800
Shan Wei <shanwei88@gmail.com> wrote:

> From: Shan Wei <davidshan@tencent.com>
> 
> UNIX_DIAG_MAX is included in enum type.
> It is equal to the total number of enum element.
> 
> But lots of enum MAX value is defined as the max enum element, e.g. INET_DIAG_MAX, XFRMA_MAX. 
> The right fixing way seems to define UNIX_DIAG_MAX as UNIX_DIAG_MEMINFO,
> but this way will break other user application.
> 
> So, just fix it on user application.
> 
> 
> Signed-off-by: Shan Wei <davidshan@tencent.com>
> ---
>  misc/ss.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/misc/ss.c b/misc/ss.c
> index 4017918..5f70a26 100644
> --- a/misc/ss.c
> +++ b/misc/ss.c
> @@ -2011,12 +2011,12 @@ void unix_list_print(struct unixstat *list, struct filter *f)
>  static int unix_show_sock(struct nlmsghdr *nlh, struct filter *f)
>  {
>  	struct unix_diag_msg *r = NLMSG_DATA(nlh);
> -	struct rtattr *tb[UNIX_DIAG_MAX+1];
> +	struct rtattr *tb[UNIX_DIAG_MAX];
>  	char name[128];
>  	int peer_ino;
>  	int rqlen;
>  
> -	parse_rtattr(tb, UNIX_DIAG_MAX, (struct rtattr*)(r+1),
> +	parse_rtattr(tb, UNIX_DIAG_MAX-1, (struct rtattr*)(r+1),
>  		     nlh->nlmsg_len - NLMSG_LENGTH(sizeof(*r)));
>  
>  	if (netid_width)

Ok, I'll put this in but with a big juicy comment so that nobody
else is surprised by the fact we got it wrong here.