Re: Sanitise ethtool.h and mii.h for userspace.

Re: Sanitise ethtool.h and mii.h for userspace.

[Jeff Garzik]

Linux Kernel Mailing List wrote:
> commit c3ce7e203af5d8eab7c3390fc991a1fcb152f741
> tree 43b0837c42a1deb5b0f87800bf6a5ed8eea2eafe
> parent 56142536868a2be34f261ed8fdca1610f8a73fbd
> author David Woodhouse <dwmw2@shinybook.infradead.org> Sat, 29 Apr 2006 01:53:47 +0100
> committer David Woodhouse <dwmw2@infradead.org> Sat, 29 Apr 2006 01:53:47 +0100
> 
> Sanitise ethtool.h and mii.h for userspace.
> 
> They shouldn't be using 'u32' et al in structures which are used for
> communication with userspace. Switch to the proper types (__u32 etc).
> 
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>

How can reviewers make an informed decision, when you completely failed 
to note:

* This breaks the primary userspace user of this header, ethtool(8)
* The patch was NAK'd (and I don't even get a "Naked-by:" header :))
* Despite knowing all this for quite some time, no associated userspace 
fix patch has ever appeared.

If you are going to break stuff, AT LEAST TELL PEOPLE IN ALL CAPS ABOUT 
IT, rather than providing the highly deceptive description as above. 
And be courteous enough to help fix the breakage, if you please.

	Jeff, ethtool maintainer

Re: Sanitise ethtool.h and mii.h for userspace.

[David Woodhouse]

On Tue, 2006-06-20 at 21:18 -0400, Jeff Garzik wrote:
> How can reviewers make an informed decision, when you completely failed 
> to note:
> 
> * This breaks the primary userspace user of this header, ethtool(8)

I cannot reproduce with either an ethtool-3 tarball or a fresh checkout
from your git tree. Can you show me the error?

It's a somewhat surprising allegation, because ethtool doesn't even
_use_ the header directly from the kernel. It has its own copy, and
currently includes it like this...

typedef unsigned long long u64;         /* hack, so we may include kernel's ethtool.h */
typedef __uint32_t u32;         /* ditto */
typedef __uint16_t u16;         /* ditto */
typedef __uint8_t u8;           /* ditto */
#include "ethtool-copy.h"

> * The patch was NAK'd (and I don't even get a "Naked-by:" header :))

Sorry, my recollection was that you backed down after everyone turned on
you and declared that "but I _want_ to use 'u32' for userspace stuff
because underscores hurt my eyes" was too silly for words. You didn't
get a mention in the commit comment because I'd already committed it by
the time we had the discussion. But I did mention it to Andrew at the
time I asked him to put the hdrcleanup tree into -mm, and he seemed
perfectly happy with the change.

> * Despite knowing all this for quite some time, no associated userspace 
> fix patch has ever appeared.

That's because to the best of my knowledge, userspace doesn't _need_ any
fix at the moment. We've built the whole of the Fedora Core 6 test 1
release against (a subset of) these headers, and that includes ethtool.

> If you are going to break stuff, AT LEAST TELL PEOPLE IN ALL CAPS ABOUT 
> IT, rather than providing the highly deceptive description as above. 
> And be courteous enough to help fix the breakage, if you please. 

If I break stuff, I promise I'll bear that in mind.

-- 
dwmw2