On Tue, Dec 05, 2023 at 09:44:12AM -0300, Fabiano Rosas wrote:
> Peter Xu <pet...@redhat.com> writes:
>
> > On Mon, Dec 04, 2023 at 06:09:16PM -0300, Fabiano Rosas wrote:
> >> Right, I got your point. I just think we could avoid designing this new
> >> string format by creating new fields with the extra space:
> >>
> >> typedef struct QEMU_PACKED {
> >> uint32_t size;
> >> uint8_t runstate[50];
> >> uint8_t unused[50];
> >> RunState state;
> >> bool received;
> >> } GlobalState;
> >>
> >> In my mind this works seamlessly, or am I mistaken?
> >
> > I think what you proposed should indeed work.
> >
> > Currently it's:
> >
> > .fields = (VMStateField[]) {
> > VMSTATE_UINT32(size, GlobalState),
> > VMSTATE_BUFFER(runstate, GlobalState),
> > VMSTATE_END_OF_LIST()
> > },
> >
> > I had a quick look at vmstate_info_buffer, it mostly only get()/put() those
> > buffers with its sizeof(), so looks all fine. For sure in all cases we'd
> > better test it to verify.
> >
> > One side note is since we so far use qapi_enum_parse() for the runstate, I
> > think the "size" is not ever used..
> >
> > If we do want a split, IMHO we can consider making runstate[] even smaller
> > to just free up the rest spaces all in one shot:
> >
> > typedef struct QEMU_PACKED {
[1]
> > uint32_t size;
> > /*
> > * Assuming 16 is good enough to fit all possible runstate strings..
> > * This field must be a string ending with '\0'.
> > */
> > uint8_t runstate[16];
> > /* 0x00 when QEMU doesn't support it, or "0"/"1" to reflect its state
> > */
> > uint8_t vm_was_suspended[1];
> > /*
> > * Still free of use space. Note that we only have 99 bytes for use
> > * because the last byte (the 100th byte) must be zero due to legacy
> > * reasons, if not it may be set to zero after loaded on dest QEMU.
> > */
>
> I'd add a 'uint8_t reserved;' to go along with this comment instead of
> leaving a hole.
Note that "struct GlobalState" is not a binary format but only some
internal storage, what really matters is vmstate_globalstate. Here the
"uint8_reserved" will be a pure waste of 1 byte in QEMU binary, imho.
I think I just copied what you had previously and extended it, logically I
don't think we ever need QEMU_PACKED right above [1]. We can also drop
"size" directly here, but this can be done later.
--
Peter Xu
