Re: [PATCH V6 05/14] migration: propagate suspended runstate

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@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 {
>       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.