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 {
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.
*/
uint8_t unused[82];
RunState state;
bool received;
} GlobalState;
Pairs with something like:
.fields = (VMStateField[]) {
/* Used to be "size" but never used on dest, so always ignored */
VMSTATE_UNUSED(4),
VMSTATE_BUFFER(runstate, GlobalState),
VMSTATE_BUFFER(vm_was_suspended, GlobalState),
/*
* This is actually all zeros, but just to differenciate from the
* last byte..
*/
VMSTATE_BUFFER(unused, GlobalState),
/*
* For historical reasons, the last byte must be 0x00 or it'll be
* overwritten by old qemu otherwise.
*/
VMSTATE_UNUSED(1),
VMSTATE_END_OF_LIST()
},
>
> In any case, a oneshot hack might be better than both our suggestions
> because we can just clean it up a couple of releases from now as if
> nothing happened.
It can be forgotten forever, then we keep the code less readable. If we
have a plan to do that and not so awkward, IMHO we should go directly with
that plan.
Thanks,
--
Peter Xu
