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 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



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux