[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: |
Ashijeet Acharya |
Subject: |
Re: [Qemu-devel] [PATCH] migrate: Introduce a 'dc->vmsd' check to avoid segfault for --only-migratable |
Date: |
Sat, 4 Feb 2017 14:14:57 +0530 |
On Mon, Jan 30, 2017 at 10:08 PM, Juan Quintela <address@hidden> wrote:
> 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>
I just realised that I had to make this fix at two different places
i.e. inside usbdevice_create() in hw/usb/bus.c as well. I will send a
v2 for that and already include Juan's R-b tag.
Ashijeet
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH] migrate: Introduce a 'dc->vmsd' check to avoid segfault for --only-migratable,
Ashijeet Acharya <=