Re: [PATCH 11/15] qtest: add a QOM object for qtest

From: Igor Mammedov <imammedo@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: qemu-devel@nongnu.org
Subject: Re: [PATCH 11/15] qtest: add a QOM object for qtest
Date: Mon, 7 Dec 2020 17:24:47 +0100	[thread overview]
Message-ID: <20201207172447.77d22d17@redhat.com> (raw)
In-Reply-To: <20201202081854.4126071-12-pbonzini@redhat.com>

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;
>      }
>  