From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751444AbdJCKE6 (ORCPT ); Tue, 3 Oct 2017 06:04:58 -0400 Received: from bombadil.infradead.org ([65.50.211.133]:37872 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750787AbdJCKE5 (ORCPT ); Tue, 3 Oct 2017 06:04:57 -0400 Date: Tue, 3 Oct 2017 03:04:49 -0700 From: Christoph Hellwig To: Hans de Goede Cc: Arnd Bergmann , Greg Kroah-Hartman , Michael Thayer , "Knut St . Osmundsen" , Larry Finger , linux-kernel@vger.kernel.org Subject: Re: [PATCH] virt: Add vboxguest driver for Virtual Box Guest integration Message-ID: <20171003100449.GA5491@infradead.org> References: <20171003092115.11341-1-hdegoede@redhat.com> <20171003092115.11341-2-hdegoede@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171003092115.11341-2-hdegoede@redhat.com> User-Agent: Mutt/1.8.3 (2017-05-23) X-SRS-Rewrite: SMTP reverse-path rewritten from by bombadil.infradead.org. See http://www.infradead.org/rpr.html Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. Make these things functions instead of macros. > + VMMDevReqHypervisorInfo *req; > + void *guest_mappings[GUEST_MAPPINGS_TRIES]; Fix your structure names. > + for (i = 0; i < (size >> PAGE_SHIFT); i++) Remove pointless inner braces. > +#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. 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.