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

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

 



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



[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux