Message ID | 1383062897-30084-11-git-send-email-armbru@redhat.com |
---|---|
State | New |
Headers | show |
On Tue, 2013-10-29 at 17:08 +0100, armbru@redhat.com wrote: > From: Markus Armbruster <armbru@redhat.com> > > Such devices have always been unavailable and omitted from the list of > available devices shown by device_add help. Until commit 18b6dad > silently broke the former, setting up nasty traps for unwary users, > like this one: > > $ qemu-system-x86_64 -nodefaults -monitor stdio -display none > QEMU 1.6.50 monitor - type 'help' for more information > (qemu) device_add apic > Segmentation fault (core dumped) > > I call that a regression. Fix it. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > qdev-monitor.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/qdev-monitor.c b/qdev-monitor.c > index 36f6f09..c538fec 100644 > --- a/qdev-monitor.c > +++ b/qdev-monitor.c > @@ -477,7 +477,7 @@ DeviceState *qdev_device_add(QemuOpts *opts) > } > } > > - if (!obj) { > + if (!obj || DEVICE_CLASS(obj)->cannot_instantiate_with_device_add_yet) { > qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", "device type"); > return NULL; > } Minor comment, if you move the if statement after k = DEVICE_CLASS(obj); you don't need the cast anymore. Seems OK to me. Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>
Marcel Apfelbaum <marcel.a@redhat.com> writes: > On Tue, 2013-10-29 at 17:08 +0100, armbru@redhat.com wrote: >> From: Markus Armbruster <armbru@redhat.com> >> >> Such devices have always been unavailable and omitted from the list of >> available devices shown by device_add help. Until commit 18b6dad >> silently broke the former, setting up nasty traps for unwary users, >> like this one: >> >> $ qemu-system-x86_64 -nodefaults -monitor stdio -display none >> QEMU 1.6.50 monitor - type 'help' for more information >> (qemu) device_add apic >> Segmentation fault (core dumped) >> >> I call that a regression. Fix it. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> qdev-monitor.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/qdev-monitor.c b/qdev-monitor.c >> index 36f6f09..c538fec 100644 >> --- a/qdev-monitor.c >> +++ b/qdev-monitor.c >> @@ -477,7 +477,7 @@ DeviceState *qdev_device_add(QemuOpts *opts) >> } >> } >> >> - if (!obj) { >> + if (!obj || DEVICE_CLASS(obj)->cannot_instantiate_with_device_add_yet) { >> qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", "device type"); >> return NULL; >> } > Minor comment, if you move the if statement after > k = DEVICE_CLASS(obj); > you don't need the cast anymore. Ignorant question: does DEVICE_CLASS(NULL) work and return NULL? > Seems OK to me. > Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com> Thanks!
On Wed, 2013-10-30 at 13:15 +0100, Markus Armbruster wrote: > Marcel Apfelbaum <marcel.a@redhat.com> writes: > > > On Tue, 2013-10-29 at 17:08 +0100, armbru@redhat.com wrote: > >> From: Markus Armbruster <armbru@redhat.com> > >> > >> Such devices have always been unavailable and omitted from the list of > >> available devices shown by device_add help. Until commit 18b6dad > >> silently broke the former, setting up nasty traps for unwary users, > >> like this one: > >> > >> $ qemu-system-x86_64 -nodefaults -monitor stdio -display none > >> QEMU 1.6.50 monitor - type 'help' for more information > >> (qemu) device_add apic > >> Segmentation fault (core dumped) > >> > >> I call that a regression. Fix it. > >> > >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > >> --- > >> qdev-monitor.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/qdev-monitor.c b/qdev-monitor.c > >> index 36f6f09..c538fec 100644 > >> --- a/qdev-monitor.c > >> +++ b/qdev-monitor.c > >> @@ -477,7 +477,7 @@ DeviceState *qdev_device_add(QemuOpts *opts) > >> } > >> } > >> > >> - if (!obj) { > >> + if (!obj || DEVICE_CLASS(obj)->cannot_instantiate_with_device_add_yet) { > >> qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", "device type"); > >> return NULL; > >> } > > Minor comment, if you move the if statement after > > k = DEVICE_CLASS(obj); > > you don't need the cast anymore. > > Ignorant question: does DEVICE_CLASS(NULL) work and return NULL? Checked it, yes, it will return NULL. Marcel > > > Seems OK to me. > > Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com> > > Thanks! >
Marcel Apfelbaum <marcel.a@redhat.com> writes: > On Wed, 2013-10-30 at 13:15 +0100, Markus Armbruster wrote: >> Marcel Apfelbaum <marcel.a@redhat.com> writes: >> >> > On Tue, 2013-10-29 at 17:08 +0100, armbru@redhat.com wrote: >> >> From: Markus Armbruster <armbru@redhat.com> >> >> >> >> Such devices have always been unavailable and omitted from the list of >> >> available devices shown by device_add help. Until commit 18b6dad >> >> silently broke the former, setting up nasty traps for unwary users, >> >> like this one: >> >> >> >> $ qemu-system-x86_64 -nodefaults -monitor stdio -display none >> >> QEMU 1.6.50 monitor - type 'help' for more information >> >> (qemu) device_add apic >> >> Segmentation fault (core dumped) >> >> >> >> I call that a regression. Fix it. >> >> >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> >> --- >> >> qdev-monitor.c | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/qdev-monitor.c b/qdev-monitor.c >> >> index 36f6f09..c538fec 100644 >> >> --- a/qdev-monitor.c >> >> +++ b/qdev-monitor.c >> >> @@ -477,7 +477,7 @@ DeviceState *qdev_device_add(QemuOpts *opts) >> >> } >> >> } >> >> >> >> - if (!obj) { >> >> + if (!obj || DEVICE_CLASS(obj)->cannot_instantiate_with_device_add_yet) { >> >> qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", "device type"); >> >> return NULL; >> >> } >> > Minor comment, if you move the if statement after >> > k = DEVICE_CLASS(obj); >> > you don't need the cast anymore. >> >> Ignorant question: does DEVICE_CLASS(NULL) work and return NULL? > Checked it, yes, it will return NULL. I'll give it a try when I respin. Thanks!
diff --git a/qdev-monitor.c b/qdev-monitor.c index 36f6f09..c538fec 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -477,7 +477,7 @@ DeviceState *qdev_device_add(QemuOpts *opts) } } - if (!obj) { + if (!obj || DEVICE_CLASS(obj)->cannot_instantiate_with_device_add_yet) { qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", "device type"); return NULL; }