Re: [PATCH 2/3] kvm tools: Introduce virtio-rng

[Ingo Molnar <mingo@elte.hu>]

* 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