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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fb7f6dbb-16a4-0ebf-a349-1ac1d937e4dc@redhat.com>
Date:   Wed, 4 Oct 2017 11:32:10 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Christoph Hellwig <hch@...radead.org>,
        Arnd Bergmann <arnd@...db.de>,
        Michael Thayer <michael.thayer@...cle.com>,
        "Knut St . Osmundsen" <knut.osmundsen@...cle.com>,
        Larry Finger <Larry.Finger@...inger.net>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] virt: Add vboxguest driver for Virtual Box Guest
 integration

Hi,

On 03-10-17 14:40, Greg Kroah-Hartman wrote:
> 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.

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.

Regards,

Hans