From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751642AbdJCLlv (ORCPT ); Tue, 3 Oct 2017 07:41:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42878 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750797AbdJCLlu (ORCPT ); Tue, 3 Oct 2017 07:41:50 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 367B725762 Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.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: Christoph Hellwig Cc: Arnd Bergmann , Greg Kroah-Hartman , 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> From: Hans de Goede Message-ID: Date: Tue, 3 Oct 2017 13:41:46 +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: <20171003100449.GA5491@infradead.org> 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.39]); Tue, 03 Oct 2017 11:41:50 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > Make these things functions instead of macros. Turning these into functions is a good idea I will do so for v2. >> + VMMDevReqHypervisorInfo *req; >> + void *guest_mappings[GUEST_MAPPINGS_TRIES]; > > Fix your structure names. > >> + for (i = 0; i < (size >> PAGE_SHIFT); i++) > > Remove pointless inner braces. One person's pointless braces are another person's way to improve code readability. Using braces around one of the 2 operands of a "<" operation is quite a normal thing to do. > >> +#ifdef CONFIG_X86_64 >> + req1->guestInfo.osType = VBOXOSTYPE_Linux26_x64; >> +#else >> + req1->guestInfo.osType = VBOXOSTYPE_Linux26; >> +#endif > > Hardcoding architecvtures is almost always a bug. If you think it > isn't here it needs a big fat comment explaining that rationale. > >> + req->guestStatus.status = active ? VBoxGuestFacilityStatus_Active : >> + VBoxGuestFacilityStatus_Inactive; > > Please use if/else for readability. > >> +/** @name Memory Ballooning >> + * @{ >> + */ > > Please get rid of this whacky comment style. > >> + >> +/** >> + * Inflate the balloon by one chunk. >> + * >> + * The caller owns the balloon mutex. >> + * >> + * @returns 0 or negative errno value. >> + * @param gdev The Guest extension device. >> + * @param chunk_idx Index of the chunk. >> + */ > > An this one isn't proper kerneldoc either. > >> + pages = kmalloc(sizeof(*pages) * VMMDEV_MEMORY_BALLOON_CHUNK_PAGES, > >> +/** >> + * Callback for heartbeat timer. >> + */ > > /** > > as the comment start is reserved for kerneldoc comments, this one > isn't a kerneldoc comment, so please remove it. > >> +static void vbg_heartbeat_timer(unsigned long data) >> +{ >> + struct vbg_dev *gdev = (struct vbg_dev *)data; >> + >> + vbg_req_perform(gdev, gdev->guest_heartbeat_req); >> + mod_timer(&gdev->heartbeat_timer, >> + msecs_to_jiffies(gdev->heartbeat_interval_ms)); >> +} > > Please use timer_setup and from_timer instead of the data argument. > (this is new in -rc3). > >> + list_add(&session->list_node, &gdev->session_list); >> + spin_unlock_irqrestore(&gdev->session_spinlock, flags); > > Session_list isn't used anywhere. And if it was it would probably > buggy due to the lack of reference counting. > >> + >> + *session_ret = session; >> + >> + return 0; > > Just return the session or an ERR_PTR. > >> + guest_status = (const VMMDevReportGuestStatus *)req; > > struct pointer asts are a ba idea - this probably should use > container_of magic or a more generic strucure with unioned elements. > >> +int vbg_status_code_to_errno(int rc) >> +{ >> + if (rc >= 0) >> + return 0; >> + >> + switch (rc) { >> + case VERR_ACCESS_DENIED: return -EPERM; > > Please never put code in the same line as the switch statement. > > Also in general code like this is better done by looping over a lookup > table. > > Anyway, this was a super shallow review that everyone could do. Thank you for your review, I will try to address most of your remarks for v2 of this patch. > The actual ioctls and pv hardware interfaces look even worse, but > such a messy giant blob they aren't reviewable. This patch needs > a major cleanup pass first, and then a split into reviewable chunks. This patch adds a single driver, so there is no sensible way to split it up. As for the pv interface that is part of existing deployed (virtual) hardware, we can pretty much change that just as much as we can change the hardware-interface of real hardware. Regards, Hans