Messages in this thread | | | Date | Tue, 3 Oct 2017 03:04:49 -0700 | From | Christoph Hellwig <> | Subject | Re: [PATCH] virt: Add vboxguest driver for Virtual Box Guest integration |
| |
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.
| |