| Message ID | 20201202081854.4126071-12-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Finish cleaning up qemu_init | expand |
On Wed, 2 Dec 2020 03:18:50 -0500 Paolo Bonzini <pbonzini@redhat.com> wrote: > The qtest server right now can only be created using the -qtest > and -qtest-log options. Allow an alternative way to create it > using "-object qtest,chardev=...,log=...". > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > softmmu/qtest.c | 144 ++++++++++++++++++++++++++++++++++++++++++++---- > softmmu/vl.c | 5 +- > 2 files changed, 135 insertions(+), 14 deletions(-) > > diff --git a/softmmu/qtest.c b/softmmu/qtest.c > index 7965dc9a16..d255c9681a 100644 > --- a/softmmu/qtest.c > +++ b/softmmu/qtest.c > @@ -27,6 +27,8 @@ > #include "qemu/error-report.h" > #include "qemu/module.h" > #include "qemu/cutils.h" > +#include "qapi/qmp/qerror.h" > +#include "qom/object_interfaces.h" > #include CONFIG_DEVICES > #ifdef CONFIG_PSERIES > #include "hw/ppc/spapr_rtas.h" > @@ -849,18 +851,9 @@ static void qtest_event(void *opaque, QEMUChrEvent event) > break; > } > } > -void qtest_server_init(const char *qtest_chrdev, const char *qtest_log, Error **errp) > -{ > - Chardev *chr; > - > - chr = qemu_chr_new("qtest", qtest_chrdev, NULL); > - > - if (chr == NULL) { > - error_setg(errp, "Failed to initialize device for qtest: \"%s\"", > - qtest_chrdev); > - return; > - } > > +static bool qtest_server_start(Chardev *chr, const char *qtest_log, Error **errp) > +{ > if (qtest_log) { > if (strcmp(qtest_log, "none") != 0) { > qtest_log_fp = fopen(qtest_log, "w+"); > @@ -869,7 +862,9 @@ void qtest_server_init(const char *qtest_chrdev, const char *qtest_log, Error ** > qtest_log_fp = stderr; > } > > - qemu_chr_fe_init(&qtest_chr, chr, errp); > + if (!qemu_chr_fe_init(&qtest_chr, chr, errp)) { > + return false; > + } > qemu_chr_fe_set_handlers(&qtest_chr, qtest_can_read, qtest_read, > qtest_event, NULL, &qtest_chr, NULL, true); > qemu_chr_fe_set_echo(&qtest_chr, true); > @@ -879,8 +874,25 @@ void qtest_server_init(const char *qtest_chrdev, const char *qtest_log, Error ** > if (!qtest_server_send) { > qtest_server_set_send_handler(qtest_server_char_be_send, &qtest_chr); > } > + return true; > +} > + > +void qtest_server_init(const char *qtest_chrdev, const char *qtest_log, Error **errp) > +{ > + Chardev *chr; > + > + chr = qemu_chr_new("qtest", qtest_chrdev, NULL); > + > + if (chr == NULL) { > + error_setg(errp, "Failed to initialize device for qtest: \"%s\"", > + qtest_chrdev); > + return; > + } > + > + qtest_server_start(chr, qtest_log, errp); why not create qtest object here instead of trying to preserve old way, or create it directly at the place that calls qtest_server_init()? > } > > + > void qtest_server_set_send_handler(void (*send)(void*, const char*), > void *opaque) > { > @@ -905,3 +917,111 @@ void qtest_server_inproc_recv(void *dummy, const char *buf) > g_string_truncate(gstr, 0); > } > } > + > +#define TYPE_QTEST "qtest" > + > +OBJECT_DECLARE_SIMPLE_TYPE(QTest, QTEST) > + > +struct QTest { > + Object parent; > + > + bool complete; > + char *chr_name; > + Chardev *chr; > + char *log; > +}; > + > +static void qtest_complete(UserCreatable *uc, Error **errp) > +{ > + QTest *q = QTEST(uc); > + if (qtest_driver()) { > + error_setg(errp, "Only one instance of qtest can be created"); > + return; > + } > + if (!q->chr_name) { > + error_setg(errp, "No backend specified"); > + return; > + } > + > + if (!qtest_server_start(q->chr, q->log, errp)) { > + return; > + } > + q->complete = true; > +} > + > +static void qtest_set_log(Object *obj, const char *value, Error **errp) > +{ > + QTest *q = QTEST(obj); > + > + if (q->complete) { > + error_setg(errp, QERR_PERMISSION_DENIED); > + } else { > + g_free(q->log); > + q->log = g_strdup(value); > + } > +} > + > +static char *qtest_get_log(Object *obj, Error **errp) > +{ > + QTest *q = QTEST(obj); > + > + return g_strdup(q->log); > +} > + > +static void qtest_set_chardev(Object *obj, const char *value, Error **errp) > +{ > + QTest *q = QTEST(obj); > + Chardev *chr; > + > + if (q->complete) { > + error_setg(errp, QERR_PERMISSION_DENIED); > + return; > + } > + > + chr = qemu_chr_find(value); > + if (!chr) { > + error_setg(errp, "Cannot find character device '%s'", value); > + return; > + } > + > + g_free(q->chr_name); > + q->chr_name = g_strdup(value); > + q->chr = chr; > +} > + > +static char *qtest_get_chardev(Object *obj, Error **errp) > +{ > + QTest *q = QTEST(obj); > + > + return g_strdup(q->chr_name); > +} > + > +static void qtest_class_init(ObjectClass *oc, void *data) > +{ > + UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc); > + > + ucc->complete = qtest_complete; > + > + object_class_property_add_str(oc, "chardev", > + qtest_get_chardev, qtest_set_chardev); > + object_class_property_add_str(oc, "log", > + qtest_get_log, qtest_set_log); > +} > + > +static const TypeInfo qtest_info = { > + .name = TYPE_QTEST, > + .parent = TYPE_OBJECT, > + .class_init = qtest_class_init, > + .instance_size = sizeof(QTest), > + .interfaces = (InterfaceInfo[]) { > + { TYPE_USER_CREATABLE }, > + { } > + } > +}; > + > +static void register_types(void) > +{ > + type_register_static(&qtest_info); > +} > + > +type_init(register_types); > diff --git a/softmmu/vl.c b/softmmu/vl.c > index 0f7222af31..e5f3c42049 100644 > --- a/softmmu/vl.c > +++ b/softmmu/vl.c > @@ -1685,8 +1685,9 @@ static bool object_create_early(const char *type, QemuOpts *opts) > * add one, state the reason in a comment! > */ > > - /* Reason: rng-egd property "chardev" */ > - if (g_str_equal(type, "rng-egd")) { > + /* Reason: property "chardev" */ > + if (g_str_equal(type, "rng-egd") || > + g_str_equal(type, "qtest")) { > return false; > } >
On 07/12/20 17:24, Igor Mammedov wrote: >> +void qtest_server_init(const char *qtest_chrdev, const char *qtest_log, Error **errp) >> +{ >> + Chardev *chr; >> + >> + chr = qemu_chr_new("qtest", qtest_chrdev, NULL); >> + >> + if (chr == NULL) { >> + error_setg(errp, "Failed to initialize device for qtest: \"%s\"", >> + qtest_chrdev); >> + return; >> + } >> + >> + qtest_server_start(chr, qtest_log, errp); > why not create qtest object here instead of trying to preserve old way, > or create it directly at the place that calls qtest_server_init()? Because I wasn't sure of where to put it in the QOM object tree. So I punted and left it for later. Paolo
On Mon, 7 Dec 2020 17:43:16 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 07/12/20 17:24, Igor Mammedov wrote: > >> +void qtest_server_init(const char *qtest_chrdev, const char *qtest_log, Error **errp) > >> +{ > >> + Chardev *chr; > >> + > >> + chr = qemu_chr_new("qtest", qtest_chrdev, NULL); > >> + > >> + if (chr == NULL) { > >> + error_setg(errp, "Failed to initialize device for qtest: \"%s\"", > >> + qtest_chrdev); > >> + return; > >> + } > >> + > >> + qtest_server_start(chr, qtest_log, errp); > > why not create qtest object here instead of trying to preserve old way, > > or create it directly at the place that calls qtest_server_init()? > > Because I wasn't sure of where to put it in the QOM object tree. So I > punted and left it for later. but you implicitly decided where it should be (with -object qtest), it goes to /objects. So I'd wouldn't put anywhere else to be consistent. > > Paolo >
Il lun 7 dic 2020, 17:57 Igor Mammedov <imammedo@redhat.com> ha scritto: > On Mon, 7 Dec 2020 17:43:16 +0100 > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 07/12/20 17:24, Igor Mammedov wrote: > > >> +void qtest_server_init(const char *qtest_chrdev, const char > *qtest_log, Error **errp) > > >> +{ > > >> + Chardev *chr; > > >> + > > >> + chr = qemu_chr_new("qtest", qtest_chrdev, NULL); > > >> + > > >> + if (chr == NULL) { > > >> + error_setg(errp, "Failed to initialize device for qtest: > \"%s\"", > > >> + qtest_chrdev); > > >> + return; > > >> + } > > >> + > > >> + qtest_server_start(chr, qtest_log, errp); > > > why not create qtest object here instead of trying to preserve old way, > > > or create it directly at the place that calls qtest_server_init()? > > > > Because I wasn't sure of where to put it in the QOM object tree. So I > > punted and left it for later. > > but you implicitly decided where it should be (with -object qtest), > it goes to /objects. > So I'd wouldn't put anywhere else to be consistent. > No, /objects is for stuff created with -object exclusively. I suppose I could have the "well-known path" be /machine/qtest, and it would be either a child (for -qtest) or a link to /objects/some-id (for -object qtest). Should I implement that (as a separate patch on top of this one)? Paolo > > > > Paolo > > > >
On Mon, 7 Dec 2020 18:22:55 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il lun 7 dic 2020, 17:57 Igor Mammedov <imammedo@redhat.com> ha scritto: > > > On Mon, 7 Dec 2020 17:43:16 +0100 > > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > On 07/12/20 17:24, Igor Mammedov wrote: > > > >> +void qtest_server_init(const char *qtest_chrdev, const char > > *qtest_log, Error **errp) > > > >> +{ > > > >> + Chardev *chr; > > > >> + > > > >> + chr = qemu_chr_new("qtest", qtest_chrdev, NULL); > > > >> + > > > >> + if (chr == NULL) { > > > >> + error_setg(errp, "Failed to initialize device for qtest: > > \"%s\"", > > > >> + qtest_chrdev); > > > >> + return; > > > >> + } > > > >> + > > > >> + qtest_server_start(chr, qtest_log, errp); > > > > why not create qtest object here instead of trying to preserve old way, > > > > or create it directly at the place that calls qtest_server_init()? > > > > > > Because I wasn't sure of where to put it in the QOM object tree. So I > > > punted and left it for later. > > > > but you implicitly decided where it should be (with -object qtest), > > it goes to /objects. > > So I'd wouldn't put anywhere else to be consistent. > > > > No, /objects is for stuff created with -object exclusively. I suppose I > could have the "well-known path" be /machine/qtest, and it would be either > a child (for -qtest) or a link to /objects/some-id (for -object qtest). > Should I implement that (as a separate patch on top of this one)? yes > > Paolo > > > > > > > > > Paolo > > > > > > >
diff --git a/softmmu/qtest.c b/softmmu/qtest.c index 7965dc9a16..d255c9681a 100644 --- a/softmmu/qtest.c +++ b/softmmu/qtest.c @@ -27,6 +27,8 @@ #include "qemu/error-report.h" #include "qemu/module.h" #include "qemu/cutils.h" +#include "qapi/qmp/qerror.h" +#include "qom/object_interfaces.h" #include CONFIG_DEVICES #ifdef CONFIG_PSERIES #include "hw/ppc/spapr_rtas.h" @@ -849,18 +851,9 @@ static void qtest_event(void *opaque, QEMUChrEvent event) break; } } -void qtest_server_init(const char *qtest_chrdev, const char *qtest_log, Error **errp) -{ - Chardev *chr; - - chr = qemu_chr_new("qtest", qtest_chrdev, NULL); - - if (chr == NULL) { - error_setg(errp, "Failed to initialize device for qtest: \"%s\"", - qtest_chrdev); - return; - } +static bool qtest_server_start(Chardev *chr, const char *qtest_log, Error **errp) +{ if (qtest_log) { if (strcmp(qtest_log, "none") != 0) { qtest_log_fp = fopen(qtest_log, "w+"); @@ -869,7 +862,9 @@ void qtest_server_init(const char *qtest_chrdev, const char *qtest_log, Error ** qtest_log_fp = stderr; } - qemu_chr_fe_init(&qtest_chr, chr, errp); + if (!qemu_chr_fe_init(&qtest_chr, chr, errp)) { + return false; + } qemu_chr_fe_set_handlers(&qtest_chr, qtest_can_read, qtest_read, qtest_event, NULL, &qtest_chr, NULL, true); qemu_chr_fe_set_echo(&qtest_chr, true); @@ -879,8 +874,25 @@ void qtest_server_init(const char *qtest_chrdev, const char *qtest_log, Error ** if (!qtest_server_send) { qtest_server_set_send_handler(qtest_server_char_be_send, &qtest_chr); } + return true; +} + +void qtest_server_init(const char *qtest_chrdev, const char *qtest_log, Error **errp) +{ + Chardev *chr; + + chr = qemu_chr_new("qtest", qtest_chrdev, NULL); + + if (chr == NULL) { + error_setg(errp, "Failed to initialize device for qtest: \"%s\"", + qtest_chrdev); + return; + } + + qtest_server_start(chr, qtest_log, errp); } + void qtest_server_set_send_handler(void (*send)(void*, const char*), void *opaque) { @@ -905,3 +917,111 @@ void qtest_server_inproc_recv(void *dummy, const char *buf) g_string_truncate(gstr, 0); } } + +#define TYPE_QTEST "qtest" + +OBJECT_DECLARE_SIMPLE_TYPE(QTest, QTEST) + +struct QTest { + Object parent; + + bool complete; + char *chr_name; + Chardev *chr; + char *log; +}; + +static void qtest_complete(UserCreatable *uc, Error **errp) +{ + QTest *q = QTEST(uc); + if (qtest_driver()) { + error_setg(errp, "Only one instance of qtest can be created"); + return; + } + if (!q->chr_name) { + error_setg(errp, "No backend specified"); + return; + } + + if (!qtest_server_start(q->chr, q->log, errp)) { + return; + } + q->complete = true; +} + +static void qtest_set_log(Object *obj, const char *value, Error **errp) +{ + QTest *q = QTEST(obj); + + if (q->complete) { + error_setg(errp, QERR_PERMISSION_DENIED); + } else { + g_free(q->log); + q->log = g_strdup(value); + } +} + +static char *qtest_get_log(Object *obj, Error **errp) +{ + QTest *q = QTEST(obj); + + return g_strdup(q->log); +} + +static void qtest_set_chardev(Object *obj, const char *value, Error **errp) +{ + QTest *q = QTEST(obj); + Chardev *chr; + + if (q->complete) { + error_setg(errp, QERR_PERMISSION_DENIED); + return; + } + + chr = qemu_chr_find(value); + if (!chr) { + error_setg(errp, "Cannot find character device '%s'", value); + return; + } + + g_free(q->chr_name); + q->chr_name = g_strdup(value); + q->chr = chr; +} + +static char *qtest_get_chardev(Object *obj, Error **errp) +{ + QTest *q = QTEST(obj); + + return g_strdup(q->chr_name); +} + +static void qtest_class_init(ObjectClass *oc, void *data) +{ + UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc); + + ucc->complete = qtest_complete; + + object_class_property_add_str(oc, "chardev", + qtest_get_chardev, qtest_set_chardev); + object_class_property_add_str(oc, "log", + qtest_get_log, qtest_set_log); +} + +static const TypeInfo qtest_info = { + .name = TYPE_QTEST, + .parent = TYPE_OBJECT, + .class_init = qtest_class_init, + .instance_size = sizeof(QTest), + .interfaces = (InterfaceInfo[]) { + { TYPE_USER_CREATABLE }, + { } + } +}; + +static void register_types(void) +{ + type_register_static(&qtest_info); +} + +type_init(register_types); diff --git a/softmmu/vl.c b/softmmu/vl.c index 0f7222af31..e5f3c42049 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -1685,8 +1685,9 @@ static bool object_create_early(const char *type, QemuOpts *opts) * add one, state the reason in a comment! */ - /* Reason: rng-egd property "chardev" */ - if (g_str_equal(type, "rng-egd")) { + /* Reason: property "chardev" */ + if (g_str_equal(type, "rng-egd") || + g_str_equal(type, "qtest")) { return false; }
The qtest server right now can only be created using the -qtest and -qtest-log options. Allow an alternative way to create it using "-object qtest,chardev=...,log=...". Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- softmmu/qtest.c | 144 ++++++++++++++++++++++++++++++++++++++++++++---- softmmu/vl.c | 5 +- 2 files changed, 135 insertions(+), 14 deletions(-)