Re: [PATCH] virt: Add vboxguest driver for Virtual Box Guest integration

[Greg Kroah-Hartman]

On Tue, Oct 03, 2017 at 01:41:46PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 03-10-17 12:04, Christoph Hellwig wrote:
> > Looks like you forgot to CC previous revierers.
> > 
> > > +#define CHECK_IOCTL_IN(req)						     \
> > > +do {									     \
> > > +	if ((req)->Hdr.cbIn != (sizeof((req)->Hdr) + sizeof((req)->u.In)) || \
> > > +	    (req)->Hdr.cbOut != sizeof((req)->Hdr))			     \
> > > +		return -EINVAL;						     \
> > > +} while (0)
> > 
> > It seems like you ignored the comments on the last version.
> > 
> > Get rid of the weird struct capilization.
> 
> The only capitalized structs are all from headers under include/uapi,
> I can remove the capitalization without breaking the ABI, but if I
> do that the VirtualBox Guest Additions userspace will no longer be
> able to actually be compiled against the in kernel version of the
> headers which seems undesirable.
> 
> Arnd, Greg KH, what is your opinion about this? I would like to
> be able to actually compile the userspace consumer of this API
> against the in kernel headers, I can change the struct names
> (and drop the typedefs) if that is considered something which I
> MUST fix to get this in mainline, but I would rather keep things
> so that the userspace tools can be compiled against the in kernel
> uapi headers.

My opinion is that kernel code, including headers, needs to look like
kernel code.  None of this "but this single, tiny, driver is special and
unique and gets to keep its bizarre coding style" stuff.  The longevity
of the developer community and codebase precludes that kind of "special
treatment".

And if userspace _really_ likes typedefs, then it's trivial for them to
just have something like a list of:

typedef struct virtual_box_check_ballon VBGLIOCCHECKBALLOON, *PVBGLIOCCHECKBALLOON;

in their .h file that they use after they include these uapi headers.

Remember, our coding style rules are there for a good reason, you want
others to fix, maintain, and understand the code, for a long time.  It's
not just there because we like to be mean.  It's your brain we care
about :)

So it should be fixed up.

> This patch adds a single driver, so there is no sensible way to split
> it up.

It's 6k lines, split it at least by the file level, can you read this
all in one sitting?

try something like:
	- uapi header files
	- util functions
	- "linux" core
	- rest
or something like that.  Be considerate of those who have to read this
stuff, you _want_ us to be happy to do so...

As it is, I'm not even going to look at it, it's too big, sorry.

thanks,

greg k-h