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

From: Hans de Goede
Date: Wed Oct 04 2017 - 05:32:18 EST


On 03-10-17 14:40, Greg Kroah-Hartman wrote:
On Tue, Oct 03, 2017 at 01:41:46PM +0200, Hans de Goede wrote:

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

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.

Ok, will fix for v2.

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

Ok, I will split this up for v2.