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

[Hans de Goede <hdegoede@redhat.com>]

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