From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ingo Molnar <mingo@elte.hu>
Subject: Re: [PATCH 2/3] kvm tools: Introduce virtio-rng
Date: Thu, 5 May 2011 08:54:23 +0200
Message-ID: <20110505065423.GD28015@elte.hu>
References: <1304170225-4859-1-git-send-email-levinsasha928@gmail.com>
 <1304170225-4859-2-git-send-email-levinsasha928@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: penberg@kernel.org, asias.hejun@gmail.com, gorcunov@gmail.com,
	prasadjoshi124@gmail.com, kvm@vger.kernel.org
To: Sasha Levin <levinsasha928@gmail.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx2.mail.elte.hu ([157.181.151.9]:51030 "EHLO mx2.mail.elte.hu"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1750821Ab1EEGy2 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Thu, 5 May 2011 02:54:28 -0400
Content-Disposition: inline
In-Reply-To: <1304170225-4859-2-git-send-email-levinsasha928@gmail.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>


* Sasha Levin <levinsasha928@gmail.com> wrote:

> +#define PCI_VENDOR_ID_REDHAT_QUMRANET			0x1af4
> +#define PCI_DEVICE_ID_VIRTIO_RNG				0x1004
> +#define PCI_SUBSYSTEM_VENDOR_ID_REDHAT_QUMRANET	0x1af4
> +#define PCI_SUBSYSTEM_ID_VIRTIO_RNG				0x0004
> +#define PCI_VIRTIO_RNG_DEVNUM 4
> +
> +#define VIRTIO_RNG_IRQ		11
> +#define VIRTIO_RNG_PIN		1
> +
> +#define NUM_VIRT_QUEUES		1
> +
> +#define VIRTIO_RNG_QUEUE_SIZE	128
> +
> +struct rng_device {
> +	uint8_t				status;
> +	uint16_t			config_vector;
> +	int					fd_rng;
> +
> +	/* virtio queue */
> +	uint16_t			queue_selector;
> +	struct virt_queue	vqs[NUM_VIRT_QUEUES];
> +	void				*jobs[NUM_VIRT_QUEUES];
> +};


Really, have you *looked* at this source code from a distance? Does it look 
neat and orderly to you??

It does not look readable to me at all: it's full of vertical spacing 
misalignments even within the *same* syntactic unit. (Not to mention file-scope 
alignment convention which is all over the place.)

> +static struct ioport_operations virtio_rng_io_ops = {
> +	.io_in		= virtio_rng_pci_io_in,
> +	.io_out		= virtio_rng_pci_io_out,
> +};
> +
> +static struct pci_device_header virtio_rng_pci_device = {
> +	.vendor_id			= PCI_VENDOR_ID_REDHAT_QUMRANET,
> +	.device_id			= PCI_DEVICE_ID_VIRTIO_RNG,
> +	.header_type		= PCI_HEADER_TYPE_NORMAL,
> +	.revision_id		= 0,
> +	.class				= 0x010000,
> +	.subsys_vendor_id	= PCI_SUBSYSTEM_VENDOR_ID_REDHAT_QUMRANET,
> +	.subsys_id			= PCI_SUBSYSTEM_ID_VIRTIO_RNG,
> +	.bar[0]				= IOPORT_VIRTIO_RNG | PCI_BASE_ADDRESS_SPACE_IO,
> +	.irq_pin			= VIRTIO_RNG_PIN,
> +	.irq_line			= VIRTIO_RNG_IRQ,
> +};

Same here! It looks like as if random new lines were jumbled within something 
copy & pasted from elsewhere, with no care taken that they look good together 
...

There's also other details like:

> +	int					fd_rng;

The _rng postfix is superfluous, we already know that this is a rng thing, it's 
within struct rng_device! The result is suboptimal usage like this:

  rng_device.fd_rng

which says 'rng' twice and clutters the code needlessly.

Plus remember the blk_device argument i made yesterday? The *same* issue gets 
reintroduced here:

   static struct rng_device rng_device;

We generally try to use different names for the structure and the local (or as 
here, global) variables - and we try to use *short* (while still expressive) 
names for variables.

So the proper and canonical naming, in line with blk_dev and net_dev would be 
rng_dev, not rng_device.

The code looks correct but we really need to try harder to keep the tools/kvm/ 
code maintainable!

Thanks,

	Ingo