Patchwork unix_diag: use netlink attribute MAX convention

login
register
mail settings
Submitter stephen hemminger
Date April 25, 2012, 5:38 p.m.
Message ID <20120425103858.04d149d8@nehalam.linuxnetplumber.net>
Download mbox | patch
Permalink /patch/155045/
State Rejected
Delegated to: David Miller
Headers show

Comments

stephen hemminger - April 25, 2012, 5:38 p.m.
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>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - April 25, 2012, 6:16 p.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
stephen hemminger - April 25, 2012, 6:56 p.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - April 25, 2012, 7:07 p.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
stephen hemminger - April 25, 2012, 7:47 p.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - April 25, 2012, 7:57 p.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
stephen hemminger - April 25, 2012, 8:09 p.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - April 25, 2012, 8:12 p.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

--- 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;