| Message ID | 1355760006-891-1-git-send-email-borntraeger@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
Ping? On 17/12/12 17:00, Christian Borntraeger wrote: > since > > commit 18b6dade8c0799c48f5c5e124b8c407cd5e22e96 > qdev: refactor device creation to allow bus_info to be set only in class > > A user can specify a device that is no_user. > For example on my i386 box, I can add a 2nd kvmvapic device. > > This patch checks for no-user and rejects the device_add. > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > hw/qdev-monitor.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c > index a1b4d6a..b2c34e7 100644 > --- a/hw/qdev-monitor.c > +++ b/hw/qdev-monitor.c > @@ -426,6 +426,11 @@ DeviceState *qdev_device_add(QemuOpts *opts) > } > > k = DEVICE_CLASS(obj); > + if (k->no_user) { > + qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", "a driver name"); > + error_printf_unless_qmp("Try with argument 'help' for a list.\n"); > + return NULL; > + } > > /* find bus */ > path = qemu_opt_get(opts, "bus"); >
Am 17.12.2012 17:00, schrieb Christian Borntraeger: > since > > commit 18b6dade8c0799c48f5c5e124b8c407cd5e22e96 > qdev: refactor device creation to allow bus_info to be set only in class > > A user can specify a device that is no_user. > For example on my i386 box, I can add a 2nd kvmvapic device. > > This patch checks for no-user and rejects the device_add. > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > hw/qdev-monitor.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c > index a1b4d6a..b2c34e7 100644 > --- a/hw/qdev-monitor.c > +++ b/hw/qdev-monitor.c > @@ -426,6 +426,11 @@ DeviceState *qdev_device_add(QemuOpts *opts) > } > > k = DEVICE_CLASS(obj); > + if (k->no_user) { Ack for this check... > + qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", "a driver name"); > + error_printf_unless_qmp("Try with argument 'help' for a list.\n"); ...but I'm not sure if this error reporting with QERR_... is still in the desired form, cc'ing Luiz. Andreas > + return NULL; > + } > > /* find bus */ > path = qemu_opt_get(opts, "bus");
On Mon, 07 Jan 2013 15:20:17 +0100 Andreas Färber <afaerber@suse.de> wrote: > Am 17.12.2012 17:00, schrieb Christian Borntraeger: > > since > > > > commit 18b6dade8c0799c48f5c5e124b8c407cd5e22e96 > > qdev: refactor device creation to allow bus_info to be set only in class > > > > A user can specify a device that is no_user. > > For example on my i386 box, I can add a 2nd kvmvapic device. > > > > This patch checks for no-user and rejects the device_add. > > > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > > --- > > hw/qdev-monitor.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c > > index a1b4d6a..b2c34e7 100644 > > --- a/hw/qdev-monitor.c > > +++ b/hw/qdev-monitor.c > > @@ -426,6 +426,11 @@ DeviceState *qdev_device_add(QemuOpts *opts) > > } > > > > k = DEVICE_CLASS(obj); > > + if (k->no_user) { > > Ack for this check... > > > + qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", "a driver name"); > > + error_printf_unless_qmp("Try with argument 'help' for a list.\n"); > > ...but I'm not sure if this error reporting with QERR_... is still in > the desired form, cc'ing Luiz. The device_add command hasn't been converted to the qapi yet, so it's fine to use qerror_report(). However, I'd drop the error_printf_unless_qmp() call, as this has the potential of making the conversion harder. > > Andreas > > > + return NULL; > > + } > > > > /* find bus */ > > path = qemu_opt_get(opts, "bus"); >
Christian Borntraeger <borntraeger@de.ibm.com> writes: > since > > commit 18b6dade8c0799c48f5c5e124b8c407cd5e22e96 > qdev: refactor device creation to allow bus_info to be set only in class > > A user can specify a device that is no_user. > For example on my i386 box, I can add a 2nd kvmvapic device. > > This patch checks for no-user and rejects the device_add. > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> I specifically broke this when QOM was introduced because no_user precludes a management tool from constructing a machine directlt. The real problem you're trying to solve is that it's an error to have two kvmvapic devices, not that users shouldn't be allowed to create them via -device. Regards, Anthony Liguori > --- > hw/qdev-monitor.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c > index a1b4d6a..b2c34e7 100644 > --- a/hw/qdev-monitor.c > +++ b/hw/qdev-monitor.c > @@ -426,6 +426,11 @@ DeviceState *qdev_device_add(QemuOpts *opts) > } > > k = DEVICE_CLASS(obj); > + if (k->no_user) { > + qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", "a driver name"); > + error_printf_unless_qmp("Try with argument 'help' for a list.\n"); > + return NULL; > + } > > /* find bus */ > path = qemu_opt_get(opts, "bus"); > -- > 1.7.11.4
On 7 January 2013 19:32, Anthony Liguori <aliguori@us.ibm.com> wrote: > Christian Borntraeger <borntraeger@de.ibm.com> writes: > >> since >> >> commit 18b6dade8c0799c48f5c5e124b8c407cd5e22e96 >> qdev: refactor device creation to allow bus_info to be set only in class >> >> A user can specify a device that is no_user. >> For example on my i386 box, I can add a 2nd kvmvapic device. >> >> This patch checks for no-user and rejects the device_add. >> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > > I specifically broke this when QOM was introduced because no_user > precludes a management tool from constructing a machine directlt. You can't do that anyway... > The real problem you're trying to solve is that it's an error to have > two kvmvapic devices, not that users shouldn't be allowed to create them > via -device. That's not the only thing no-user gets used for. A bunch of the ARM sysbus devices have it set presumably because it's just flat impossible for a user to correctly create and wire in a sysbus device at all, so you might as well not confuse matters by listing them in '-device help' output. (We're not consistent about that, though.) It seems to me like arbitrarily allowing the monitor to construct no-user devices isn't really the right way to attack the problem of "allow complete machine construction by management tools"... -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 7 January 2013 19:32, Anthony Liguori <aliguori@us.ibm.com> wrote: >> Christian Borntraeger <borntraeger@de.ibm.com> writes: >> >>> since >>> >>> commit 18b6dade8c0799c48f5c5e124b8c407cd5e22e96 >>> qdev: refactor device creation to allow bus_info to be set only in class >>> >>> A user can specify a device that is no_user. >>> For example on my i386 box, I can add a 2nd kvmvapic device. >>> >>> This patch checks for no-user and rejects the device_add. >>> >>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> >> I specifically broke this when QOM was introduced because no_user >> precludes a management tool from constructing a machine directlt. > > You can't do that anyway... > >> The real problem you're trying to solve is that it's an error to have >> two kvmvapic devices, not that users shouldn't be allowed to create them >> via -device. > > That's not the only thing no-user gets used for. A bunch of the > ARM sysbus devices have it set presumably because it's just > flat impossible for a user to correctly create and wire in a sysbus > device at all, so you might as well not confuse matters by listing > them in '-device help' output. (We're not consistent about that, > though.) > > It seems to me like arbitrarily allowing the monitor to construct > no-user devices isn't really the right way to attack the problem > of "allow complete machine construction by management tools"... There is no such thing as a 'no-user' device. It's a silly distinction that has never had a consistent meaning. There's really no good reason why kvmvapic isn't supported by -device other than it was an implementation short cut. Regards, Anthony Liguori > > -- PMM
On 7 January 2013 20:12, Anthony Liguori <aliguori@us.ibm.com> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: >> It seems to me like arbitrarily allowing the monitor to construct >> no-user devices isn't really the right way to attack the problem >> of "allow complete machine construction by management tools"... > > There is no such thing as a 'no-user' device. It's a silly distinction > that has never had a consistent meaning. Then let's just rip that flag out completely. -- PMM
On 07/01/13 20:32, Anthony Liguori wrote: > Christian Borntraeger <borntraeger@de.ibm.com> writes: > >> since >> >> commit 18b6dade8c0799c48f5c5e124b8c407cd5e22e96 >> qdev: refactor device creation to allow bus_info to be set only in class >> >> A user can specify a device that is no_user. >> For example on my i386 box, I can add a 2nd kvmvapic device. >> >> This patch checks for no-user and rejects the device_add. >> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > > I specifically broke this when QOM was introduced because no_user > precludes a management tool from constructing a machine directlt. Breaking it but leaving it in the code doesnt seem to be the right thing. The commit message from 18b6dade doesnt give any hint that this is now broken and nobody audited the callers of no_user that they handle things gracefully. So whats the plan? Totally remove no_user tree-wide? Christian
Peter Maydell <peter.maydell@linaro.org> writes: > On 7 January 2013 20:12, Anthony Liguori <aliguori@us.ibm.com> wrote: >> Peter Maydell <peter.maydell@linaro.org> writes: >>> It seems to me like arbitrarily allowing the monitor to construct >>> no-user devices isn't really the right way to attack the problem >>> of "allow complete machine construction by management tools"... >> >> There is no such thing as a 'no-user' device. It's a silly distinction >> that has never had a consistent meaning. > > Then let's just rip that flag out completely. I have no objection to that. I would have done that but it wasn't easy to script at the time. Regards, Anthony Liguori > > -- PMM
Christian Borntraeger <borntraeger@de.ibm.com> writes: > On 07/01/13 20:32, Anthony Liguori wrote: >> Christian Borntraeger <borntraeger@de.ibm.com> writes: >> >>> since >>> >>> commit 18b6dade8c0799c48f5c5e124b8c407cd5e22e96 >>> qdev: refactor device creation to allow bus_info to be set only in class >>> >>> A user can specify a device that is no_user. >>> For example on my i386 box, I can add a 2nd kvmvapic device. >>> >>> This patch checks for no-user and rejects the device_add. >>> >>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> >> I specifically broke this when QOM was introduced because no_user >> precludes a management tool from constructing a machine directlt. > > Breaking it but leaving it in the code doesnt seem to be the right thing. > The commit message from 18b6dade doesnt give any hint that this is now > broken and nobody audited the callers of no_user that they handle things > gracefully. > > So whats the plan? Totally remove no_user tree-wide? One of the reasons I left no_user is that it's exposed to users via 'info qdm'. I don't think it matters anymore so we can probably safely remove it. Regards, Anthony Liguori > > Christian
Am 07.01.2013 21:16, schrieb Peter Maydell: > On 7 January 2013 20:12, Anthony Liguori <aliguori@us.ibm.com> wrote: >> Peter Maydell <peter.maydell@linaro.org> writes: >>> It seems to me like arbitrarily allowing the monitor to construct >>> no-user devices isn't really the right way to attack the problem >>> of "allow complete machine construction by management tools"... >> >> There is no such thing as a 'no-user' device. It's a silly distinction >> that has never had a consistent meaning. > > Then let's just rip that flag out completely. That's a bad idea, given that we are about to make the CPU a device. So yes, there are devices that are not meant to be instantiated more than once (e.g., sclp-console). And there are devices that are not meant to be instantiated by the user at all. BTW this patch exposed that there is silly code in this function: When the bus is NULL, it is initialized as the SysBus. But we all know that SysBus is not hotplug-capable in the first place, so we can never device_add on SysBus. Regards, Andreas
On 7 January 2013 22:10, Andreas Färber <afaerber@suse.de> wrote: > Am 07.01.2013 21:16, schrieb Peter Maydell: >> On 7 January 2013 20:12, Anthony Liguori <aliguori@us.ibm.com> wrote: >>> Peter Maydell <peter.maydell@linaro.org> writes: >>>> It seems to me like arbitrarily allowing the monitor to construct >>>> no-user devices isn't really the right way to attack the problem >>>> of "allow complete machine construction by management tools"... >>> >>> There is no such thing as a 'no-user' device. It's a silly distinction >>> that has never had a consistent meaning. >> >> Then let's just rip that flag out completely. > > That's a bad idea, given that we are about to make the CPU a device. We already have a pile of devices which the user can't usefully use -device on...CPUs would be just another one, right? -- PMM
Am 07.01.2013 23:20, schrieb Peter Maydell: > On 7 January 2013 22:10, Andreas Färber <afaerber@suse.de> wrote: >> Am 07.01.2013 21:16, schrieb Peter Maydell: >>> On 7 January 2013 20:12, Anthony Liguori <aliguori@us.ibm.com> wrote: >>>> Peter Maydell <peter.maydell@linaro.org> writes: >>>>> It seems to me like arbitrarily allowing the monitor to construct >>>>> no-user devices isn't really the right way to attack the problem >>>>> of "allow complete machine construction by management tools"... >>>> >>>> There is no such thing as a 'no-user' device. It's a silly distinction >>>> that has never had a consistent meaning. >>> >>> Then let's just rip that flag out completely. >> >> That's a bad idea, given that we are about to make the CPU a device. > > We already have a pile of devices which the user can't usefully > use -device on...CPUs would be just another one, right? Not sure what you're arguing for here? The CPU is the worst example of a device I know in that it is not limited to a particular bus and messes with global state and threads. Also, I'm sure that there will be objects/devices that a management tool is not supposed to mess with either, once we start using object_initialize() more. If we agree that "no_user = 1" is not ideal or used inconsistently, then we should IMO talk about how to replace/amend it and not conclude to just rip it out and leave users without sensible error messages while praying for The Omnipotent Management Tool that AFAICS we don't have today... Andreas
Andreas Färber <afaerber@suse.de> writes: > Am 07.01.2013 23:20, schrieb Peter Maydell: >> On 7 January 2013 22:10, Andreas Färber <afaerber@suse.de> wrote: >>> Am 07.01.2013 21:16, schrieb Peter Maydell: >>>> On 7 January 2013 20:12, Anthony Liguori <aliguori@us.ibm.com> wrote: >>>>> Peter Maydell <peter.maydell@linaro.org> writes: >>>>>> It seems to me like arbitrarily allowing the monitor to construct >>>>>> no-user devices isn't really the right way to attack the problem >>>>>> of "allow complete machine construction by management tools"... >>>>> >>>>> There is no such thing as a 'no-user' device. It's a silly distinction >>>>> that has never had a consistent meaning. >>>> >>>> Then let's just rip that flag out completely. >>> >>> That's a bad idea, given that we are about to make the CPU a device. >> >> We already have a pile of devices which the user can't usefully >> use -device on...CPUs would be just another one, right? > > Not sure what you're arguing for here? The CPU is the worst example of a > device I know in that it is not limited to a particular bus and messes > with global state and threads. > > Also, I'm sure that there will be objects/devices that a management tool > is not supposed to mess with either, once we start using > object_initialize() more. > > If we agree that "no_user = 1" is not ideal or used inconsistently, then > we should IMO talk about how to replace/amend it and not conclude to > just rip it out and leave users without sensible error messages while > praying for The Omnipotent Management Tool that AFAICS we don't have > today... If you want to replace 'no_user' with 'cannot_instantiate_with_device_add_yet_due_to_internal_bugs' I'm okay with that. I'm all for having friendly errors for users. But no_user doesn't just serve a 'broken' flag today. Random devices are marked as no_user that really shouldn't be like isa-fdc. Regards, Anthony Liguori > > Andreas > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c index a1b4d6a..b2c34e7 100644 --- a/hw/qdev-monitor.c +++ b/hw/qdev-monitor.c @@ -426,6 +426,11 @@ DeviceState *qdev_device_add(QemuOpts *opts) } k = DEVICE_CLASS(obj); + if (k->no_user) { + qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", "a driver name"); + error_printf_unless_qmp("Try with argument 'help' for a list.\n"); + return NULL; + } /* find bus */ path = qemu_opt_get(opts, "bus");
since commit 18b6dade8c0799c48f5c5e124b8c407cd5e22e96 qdev: refactor device creation to allow bus_info to be set only in class A user can specify a device that is no_user. For example on my i386 box, I can add a 2nd kvmvapic device. This patch checks for no-user and rejects the device_add. Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> --- hw/qdev-monitor.c | 5 +++++ 1 file changed, 5 insertions(+)