From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Date: Fri, 18 Jun 2021 08:02:57 +0000 Subject: Re: [PATCH v11 2/7] KVM: stats: Add fd-based API to read binary stats data Message-Id: 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 , Jing Zhang Cc: 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 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. >> +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. > Why aren't these structures defined here in kerneldoc so that we can > understand them better? Putting them in a .rst file guarantees they > will get out of sync, and you can always directly import the kerneldoc > into the .rst file. This is a problem in general with Documentation/virt/kvm/api.rst. The file is organized to match the kerneldoc structs to the ioctl that they are used for, and sometimes a ioctl includes different structs for each architecture. It is probably possible to do it using :identifiers: and/or :doc:, but it would require running scripts/kernel-doc on the uAPI headers dozens of times. That is quite expensive at 0.3s each run, but that's what you get with Perl (gcc -fsyntax-only is 20 times faster). Paolo