[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] migrate: Introduce a 'dc->vmsd' check to avoid
From: |
Juan Quintela |
Subject: |
Re: [Qemu-devel] [PATCH] migrate: Introduce a 'dc->vmsd' check to avoid segfault for --only-migratable |
Date: |
Mon, 30 Jan 2017 17:38:28 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) |
Peter Maydell <address@hidden> wrote:
> On 30 January 2017 at 14:41, Ashijeet Acharya <address@hidden> wrote:
>> Commit a3a3d8c7 introduced a segfault bug while checking for
>> 'dc->vmsd->unmigratable' which caused QEMU to crash when trying to add
>> devices which do no set their 'dc->vmsd' yet while initialization.
>> Place a 'dc->vmsd' check prior to it so that we do not segfault for
>> such devices.
>>
>> NOTE: This doesn't compromise the functioning of --only-migratable
>> option as all the unmigratable devices do set their 'dc->vmsd'.
>>
>> Signed-off-by: Ashijeet Acharya <address@hidden>
>> ---
>> qdev-monitor.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 81d01df..a1106fd 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -578,7 +578,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error
>> **errp)
>> return NULL;
>> }
>>
>> - if (only_migratable) {
>> + if (only_migratable && dc->vmsd) {
>> if (dc->vmsd->unmigratable) {
>> error_setg(errp, "Device %s is not migratable, but "
>> "--only-migratable was specified", driver);
>
> This seems like a good fix for the crash as a short term fix,
> but longer term I think it would be better to make setting
> dc->vmsd mandatory. I think devices which don't set it fall into
> one of these categories:
> * deliberately has no VMState struct as it has no state that
> needs migrating -- we should have some kind of flag for
> such devices to positively assert that they don't need to
> deal with migration
> * accidentally failed to provide a VMState struct -- this is
> a bug and the device should be fixed to at minimum mark
> itself as unmigratable (and ideally fixed to implement
> migration!)
> * didn't provide a VMState struct because they handle
> migration manually using vmstate_register() -- we should
> update these to use VMState, or failing that use the
> flag to say "deliberately not providing VMState"
>
> Then we can make QEMU assert if dc->vmsd is NULL, and
> catch future occurrences of "oops I didn't think about
> migration" bugs in new device models. We also have an
> easy way to grep the codebase for devices that need to
> have migration implemented.
+1
Reviewed-by: Juan Quintela <address@hidden>