Re: [PATCH v10 2/5] KVM: stats: Add fd-based API to read binary stats data

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Jun 17, 2021 at 04:41:43AM +0000, Jing Zhang wrote:
> Provides a file descriptor per VM to read VM stats info/data.
> Provides a file descriptor per vCPU to read vCPU stats info/data.

Shouldn't this be two separate patches, one for each thing as these are
two different features being added?

Anyway, an implementation question for both of these:

> +static ssize_t kvm_stats_read(struct _kvm_stats_header *header,
> +		struct _kvm_stats_desc *desc, void *stats, size_t size_stats,
> +		char __user *user_buffer, size_t size, loff_t *offset)
> +{
> +	ssize_t copylen, len, remain = size;

You are "burying" the fact that remain is initialized here, not nice, I
totally missed it when reading this the first time.

This should be:
	ssize_t copylen;
	ssize_t len;
	ssize_t remain = size;
to be obvious.

Remember you will be looking at this code for the next 20 years, make it
easy to read.

> +	size_t size_header, size_desc;
> +	loff_t pos = *offset;
> +	char __user *dest = user_buffer;
> +	void *src;
> +
> +	size_header = sizeof(*header);
> +	size_desc = header->header.count * sizeof(*desc);
> +
> +	len = size_header + size_desc + size_stats - pos;
> +	len = min(len, remain);
> +	if (len <= 0)
> +		return 0;
> +	remain = len;
> +
> +	/* Copy kvm stats header */
> +	copylen = size_header - pos;
> +	copylen = min(copylen, remain);
> +	if (copylen > 0) {
> +		src = (void *)header + pos;
> +		if (copy_to_user(dest, src, copylen))
> +			return -EFAULT;
> +		remain -= copylen;
> +		pos += copylen;
> +		dest += copylen;
> +	}

I thought you said that you would not provide the header for each read,
if you keep reading from the fd.  It looks like you are adding it here
to each read, or is there some "magic" with pos happening here that I do
not understand?

And if there is "magic" with pos, you should document it as it's not
very obvious :)

> +	/* Copy kvm stats descriptors */
> +	copylen = header->header.desc_offset + size_desc - pos;
> +	copylen = min(copylen, remain);
> +	if (copylen > 0) {
> +		src = (void *)desc + pos - header->header.desc_offset;
> +		if (copy_to_user(dest, src, copylen))
> +			return -EFAULT;
> +		remain -= copylen;
> +		pos += copylen;
> +		dest += copylen;
> +	}
> +	/* Copy kvm stats values */

New lines between code blocks of doing things?

And again, why copy the decriptor again?  or is it being skipped
somehow?  Ah, I think I see how it's being skipped, if I look really
closely.  But again, it's not obvious, and I could be wrong.  Please
document this REALLY well.

Write code for the developer first, compiler second.  Again, you are
going to be maintaining it for 20+ years, think of your future self...


> +	copylen = header->header.data_offset + size_stats - pos;
> +	copylen = min(copylen, remain);
> +	if (copylen > 0) {
> +		src = stats + pos - header->header.data_offset;
> +		if (copy_to_user(dest, src, copylen))
> +			return -EFAULT;
> +		remain -= copylen;
> +		pos += copylen;
> +		dest += copylen;
> +	}
> +
> +	*offset = pos;
> +	return len;
> +}

thanks,

greg k-h