Hi Greg,
On Thu, Jun 17, 2021 at 2:24 AM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> 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?
>
Actually, it is really not easy to separate this change into two patches even by
following Paolo's suggestion. And it would be a surprise to userspace to see
only VM stats, no VCPU stats.
I guess it is the text that caused the confusion, I'll change the commit message
for this.
> 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.
>
Nice! Will do.
> > + 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 :)
>
Will do.
> > + /* 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?
>
Will add lines between code blocks.
> 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...
>
>
Sure, will document it.
> > + 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
Thanks,
Jing
