From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Date: Fri, 18 Jun 2021 15:51:32 +0000 Subject: Re: [PATCH v11 2/7] KVM: stats: Add fd-based API to read binary stats data Message-Id: <22bb0eb6-1305-4af9-aecc-166d7e62e6c3@gnu.org> List-Id: References: <20210618044819.3690166-1-jingzhangos@google.com> <20210618044819.3690166-3-jingzhangos@google.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Greg KH Cc: Jing Zhang , KVM , KVMARM , LinuxMIPS , KVMPPC , LinuxS390 , Linuxkselftest , Marc Zyngier , James Morse , Julien Thierry , Suzuki K Poulose , Will Deacon , Huacai Chen , Aleksandar Markovic , Thomas Bogendoerfer , Paul Mackerras , Christian Borntraeger , Janosch Frank , David Hildenbrand , Cornelia Huck , Claudio Imbrenda , Sean Christopherson , Vitaly Kuznetsov , Jim Mattson , Peter Shier , Oliver Upton , David Rientjes , Emanuele Giuseppe Esposito , David Matlack , Ricardo Koller , Krish Sadhukhan , Fuad Tabba On 18/06/21 10:23, Greg KH wrote: > On Fri, Jun 18, 2021 at 10:02:57AM +0200, Paolo Bonzini wrote: >> On 18/06/21 09:00, Greg KH wrote: >>>> +struct kvm_stats_header { >>>> + __u32 name_size; >>>> + __u32 count; >>>> + __u32 desc_offset; >>>> + __u32 data_offset; >>>> + char id[]; >>>> +}; >>> >>> You mentioned before that the size of this really is the size of the >>> structure + KVM_STATS_ID_MAXLEN, right? Or is it - KVM_STATS_ID_MAXLEN? >>> >>> If so, why not put that value explicitly in: >>> char id[THE_REST_OF_THE_HEADER_SPACE]; >>> >>> As this is not a variable header size at all, and you can not change it >>> going forward, so the variable length array here feels disingenuous. >> >> It can change; the header goes up to desc_offset. Let's rename desc_offset >> to header_size. > > "Traditionally" the first field of a variable length structure like this > has the size. So maybe this needs to be: > > struct kvm_stats_header { > __u32 header_size; Thinking more about it, I slightly prefer id_offset so that we can later give a meaning to any bytes after kvm_stats_header and before id_offset. Adding four unused bytes (for now always zero) is also useful to future proof the struct a bit, thus: struct kvm_stats_header { __u32 flags; __u32 name_size; __u32 num_desc; __u32 id_offset; __u32 desc_offset; __u32 data_offset; } (Indeed num_desc is better than count). > Wait, what is "name_size" here for? So that you know the full size of the descriptors is (name_size + sizeof(kvm_stats_desc) + name_size) * num_desc. That's the memory you allocate and the size that you can then pass to a single pread system call starting from offset desc_offset. There is certainly room for improvement in that the length of id[] and name[] can be unified to name_size. >>>> +struct kvm_stats_desc { >>>> + __u32 flags; >>>> + __s16 exponent; >>>> + __u16 size; >>>> + __u32 offset; >>>> + __u32 unused; >>>> + char name[]; >>>> +}; >>> >>> What is the max length of name? >> >> It's name_size in the header. > > So it's specified in the _previous_ header? That feels wrong, shouldn't > this descriptor define what is in it? Compared to e.g. PCI where you can do random-access reads from memory or configuration space, reading from a file has slightly different tradeoffs. So designing a file format is slightly different compared to designing an in-memory format, or a wire protocol. Paolo