On 12/5/2023 7:44 AM, 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 {
>> 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.
I'll use this scheme, thanks. It is a clearer than implicitly packing strings.
- Steve
