lkml.org 
[lkml]   [2017]   [Oct]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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.

\
 
 \ /
  Last update: 2017-10-03 12:05    [W:0.064 / U:8.112 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site