From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751267AbdJDJcP (ORCPT ); Wed, 4 Oct 2017 05:32:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49188 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751114AbdJDJcO (ORCPT ); Wed, 4 Oct 2017 05:32:14 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com CCFA780E6A Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=hdegoede@redhat.com Subject: Re: [PATCH] virt: Add vboxguest driver for Virtual Box Guest integration To: Greg Kroah-Hartman Cc: Christoph Hellwig , Arnd Bergmann , Michael Thayer , "Knut St . Osmundsen" , Larry Finger , linux-kernel@vger.kernel.org References: <20171003092115.11341-1-hdegoede@redhat.com> <20171003092115.11341-2-hdegoede@redhat.com> <20171003100449.GA5491@infradead.org> <20171003124028.GA4739@kroah.com> From: Hans de Goede Message-ID: Date: Wed, 4 Oct 2017 11:32:10 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20171003124028.GA4739@kroah.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Wed, 04 Oct 2017 09:32:14 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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