On 12/5/2023 11:52 AM, Fabiano Rosas wrote:
> Peter Xu <pet...@redhat.com> writes:
>
>> 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 prefer wasting the byte and make the code more obvious to people who
> might not immediately understand what's going on. We could even
> assert(!global_state.reserved) to sanity check the assumption. Anyway,
> that's minor, I'm fine with it either way.
>
>> 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.
>
> Ah right, I was initially thinking of letting the new qemu overrun
> runstate[16] so we wouldn't have to change the code. But that's indeed
> not necessary, your additions to the vmstate make it ok. Thanks.
There is no need to reserve byte 100 in the new scheme. The incoming side
sets s->runstate[sizeof(s->runstate) - 1] = 0 to protect itself, and now
sizeof(runstate) is smaller.
- Steve
