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

[Peter Xu <peterx@redhat.com>]

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