Re: [Qemu-devel] [PATCH v2 10/10] qdev: Do not let the user try to device_add when it cannot work
Marcel Apfelbaum writes: > On Wed, 2013-10-30 at 13:15 +0100, Markus Armbruster wrote: >> Marcel Apfelbaum writes: >> >> > On Tue, 2013-10-29 at 17:08 +0100, arm...@redhat.com wrote: >> >> From: Markus Armbruster >> >> >> >> 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 >> >> --- >> >> 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!
Re: [Qemu-devel] [PATCH v2 10/10] qdev: Do not let the user try to device_add when it cannot work
On Wed, 2013-10-30 at 13:15 +0100, Markus Armbruster wrote: > Marcel Apfelbaum writes: > > > On Tue, 2013-10-29 at 17:08 +0100, arm...@redhat.com wrote: > >> From: Markus Armbruster > >> > >> 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 > >> --- > >> 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 > > Thanks! >
Re: [Qemu-devel] [PATCH v2 10/10] qdev: Do not let the user try to device_add when it cannot work
Marcel Apfelbaum writes: > On Tue, 2013-10-29 at 17:08 +0100, arm...@redhat.com wrote: >> From: Markus Armbruster >> >> 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 >> --- >> 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 Thanks!
Re: [Qemu-devel] [PATCH v2 10/10] qdev: Do not let the user try to device_add when it cannot work
On Tue, 2013-10-29 at 17:08 +0100, arm...@redhat.com wrote: > From: Markus Armbruster > > 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 > --- > 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
[Qemu-devel] [PATCH v2 10/10] qdev: Do not let the user try to device_add when it cannot work
From: Markus Armbruster 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 --- 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; } -- 1.8.1.4