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.