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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [LKML Archive]     [Linux ARM Kernel]     [Linux ARM]     [Git]     [Yosemite News]     [Linux SCSI]     [Linux Hams]

  Powered by Linux