Re: [PATCH v1] migration/savevm: Allow stub VMSDs

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Mon, Mar 16, 2026 at 04:47:59PM -0300, Fabiano Rosas wrote:
>> After the referenced commit, empty VMStateDescription objects declared
>> as part of stubs have started reaching the vmstate code.
>> 
>> A valid VMStateDescription must at minimum have a name and either the
>> .fields or .unmigratable fields set. Stubs, being empty, have
>> none. Code that assumes a non-NULL name field will now cause a
>> crash. E.g.
>> 
>> $ ./build/mips/qemu-system-mipsel -nographic -drive if=none,format=qcow2,file=dummy.qcow2
>>     [Type "C-a c" to get the "(qemu)" monitor prompt)]
>>     (qemu) savevm foo
>> 
>>     Backtrace from doing this under gdb:
>> 
>>       #0  0x0000555555df7d4d in vmsd_can_compress (field=0x5555564f78a0
>>       <__compound_literal.3>) at ../../migration/vmstate.c:339
>>       #1  0x0000555555df7dbb in vmsd_desc_field_start
>>           (vmsd=0x555556431ba0 <vmstate_cpuhp_state>, vmdesc=0x555556918690,
>>       field=0x5555564f78a0 <__compound_literal.3>, i=0, max=1) at
>>       ../../migration/vmstate.c:362
>>       #2  0x0000555555df85a7 in vmstate_save_state_v
>>           (f=0x555556b5a0c0, vmsd=0x555556431ba0 <vmstate_cpuhp_state>,
>>       opaque=0x555556c9aac0, vmdesc=0x555556918690, version_id=1,
>>       errp=0x7fffffffc948) at ../../migration/vmstate.c:528
>>       #3  0x0000555555df8032 in vmstate_save_state
>>           (f=0x555556b5a0c0, vmsd=0x555556431ba0 <vmstate_cpuhp_state>,
>>       opaque=0x555556c9aac0, vmdesc_id=0x555556918690, errp=0x7fffffffc948)
>>       at ../../migration/vmstate.c:427
>>       #4  0x0000555555df8f83 in vmstate_subsection_save
>>           (f=0x555556b5a0c0, vmsd=0x555556431c40 <vmstate_acpi>,
>>       opaque=0x555556c9aac0, vmdesc=0x555556918690, errp=0x7fffffffc948)
>>           at ../../migration/vmstate.c:695
>> 
>> Due to their very nature, it's better to allow stubs to be
>> completely empty instead of forcing any rules. Teach the code to skip
>> them.
>> 
>> Fixes: 7aa563630b ("pc: Start with modern CPU hotplug interface by default")
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/savevm.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index dd58f2a705..e8d3360877 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -861,6 +861,10 @@ static void vmstate_check(const VMStateDescription *vmsd)
>>      const VMStateField *field = vmsd->fields;
>>      const VMStateDescription * const *subsection = vmsd->subsections;
>>  
>> +    if (!vmsd->name) {
>> +        return;
>> +    }
>
> IIUC this is fine but isn't needed for stubs' use case, because "field" is
> checked right below..
>
>> +
>>      if (field) {
>
> .. here.
>
>>          while (field->name) {
>>              if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) {
>> @@ -897,6 +901,11 @@ int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id,
>>  {
>>      SaveStateEntry *se;
>>  
>> +    if (!vmsd->name) {
>> +        /* assume it's a stub and ignore */
>> +        return 0;
>> +    }
>
> Two questions on this one..
>
> (1) when someone adds some new VMSD to device and forgot to attach a name,
>     it'll silently ignore that new VMSD.. of course anyone testing this
>     will find something strange, but silently ignoring an VMSD when not
>     having name does sound like unexpected..
>
> (2) would this fix the above crash?  The problem is the commit got fixed
>     used VMSTATE_CPU_HOTPLUG which is an internal vmsd, this one only
>     checks the top VMSD (which should have a name here..).  So I'm not sure
>     if it'll work..
>

Ouch, you're right. I checked the CI, but the issue caught by the CI was
the other one. My brain is fried, I'll get back to you guys
tomorrow. Sorry for the noise.

> Is that MIPS board the only one that is broken?  Do we want to make sure
> nothing is missed? We can definitely decide to break the migration, but we
> can also choose to just revert the needed() back to normal.  It just that
> it looks like we don't strongly need to break it.. as I don't see what we
> get from breaking it yet.. (say, it's not like we can get rid of some weird
> API or legacy function, we need to support needed() anyway..)
>
>> +
>>      /* If this triggers, alias support can be dropped for the vmsd. */
>>      assert(alias_id == -1 || required_for_version >= vmsd->minimum_version_id);
>>  
>> -- 
>> 2.51.0
>>