| Message ID | 1485787271-8754-1-git-send-email-ashijeetacharya@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 30 January 2017 at 14:41, Ashijeet Acharya <ashijeetacharya@gmail.com> 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 <ashijeetacharya@gmail.com> > --- > 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. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> wrote: > On 30 January 2017 at 14:41, Ashijeet Acharya <ashijeetacharya@gmail.com> 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 <ashijeetacharya@gmail.com> >> --- >> 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 <quintela@redhat.com>
On Mon, Jan 30, 2017 at 10:08 PM, Juan Quintela <quintela@redhat.com> wrote: > Peter Maydell <peter.maydell@linaro.org> wrote: >> On 30 January 2017 at 14:41, Ashijeet Acharya <ashijeetacharya@gmail.com> 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 <ashijeetacharya@gmail.com> >>> --- >>> 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 <quintela@redhat.com> 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
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);
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 <ashijeetacharya@gmail.com> --- qdev-monitor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)