Re: [PATCH v4 06/24] chardev: set record/replay on the base device of a muxed device

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

Hi

On Mon, Mar 11, 2024 at 9:44 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>
> chardev events to a muxed device don't get recorded because e.g.,
> qemu_chr_be_write() checks whether the base device has the record flag
> set.
>
> This can be seen when replaying a trace that has characters typed into
> the console, an examination of the log shows they are not recorded.
>
> Setting QEMU_CHAR_FEATURE_REPLAY on the base chardev fixes the problem.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  chardev/char.c | 71 +++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 50 insertions(+), 21 deletions(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index 3c43fb1278..ba847b6e9e 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -615,11 +615,24 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts, Error **errp)
>      return backend;
>  }
>
> -Chardev *qemu_chr_new_from_opts(QemuOpts *opts, GMainContext *context,
> -                                Error **errp)
> +static void qemu_chardev_set_replay(Chardev *chr, Error **errp)
> +{
> +    if (replay_mode != REPLAY_MODE_NONE) {
> +        if (CHARDEV_GET_CLASS(chr)->chr_ioctl) {
> +            error_setg(errp, "Replay: ioctl is not supported "
> +                             "for serial devices yet");
> +            return;

You are changing the behaviour, the previous code was just printing an
error, but let it go. I think you should make this a separate change.

> +        }
> +        qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_REPLAY);
> +        replay_register_char_driver(chr);
> +    }
> +}
> +
> +static Chardev *__qemu_chr_new_from_opts(QemuOpts *opts, GMainContext *context,
> +                                         bool replay, Error **errp)

Having so many chr_new functions is starting to really damage
readability. Also don't use '__' prefix for global symbols, they are
reserved.

>  {>      const ChardevClass *cc;
> -    Chardev *chr = NULL;
> +    Chardev *base = NULL, *chr = NULL;
>      ChardevBackend *backend = NULL;
>      const char *name = qemu_opt_get(opts, "backend");
>      const char *id = qemu_opts_id(opts);
> @@ -657,11 +670,11 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts, GMainContext *context,
>      chr = qemu_chardev_new(bid ? bid : id,
>                             object_class_get_name(OBJECT_CLASS(cc)),
>                             backend, context, errp);
> -
>      if (chr == NULL) {
>          goto out;
>      }
>
> +    base = chr;
>      if (bid) {
>          Chardev *mux;
>          qapi_free_ChardevBackend(backend);
> @@ -681,11 +694,25 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts, GMainContext *context,
>  out:
>      qapi_free_ChardevBackend(backend);
>      g_free(bid);
> +
> +    if (replay && base) {
> +        /* RR should be set on the base device, not the mux */
> +        qemu_chardev_set_replay(base, errp);
> +    }
> +
>      return chr;
>  }
>
> -Chardev *qemu_chr_new_noreplay(const char *label, const char *filename,
> -                               bool permit_mux_mon, GMainContext *context)
> +Chardev *qemu_chr_new_from_opts(QemuOpts *opts, GMainContext *context,
> +                                Error **errp)
> +{
> +    /* XXX: should this really not record/replay? */

I don't understand the context here, make it a different commit to explain?

> +    return __qemu_chr_new_from_opts(opts, context, false, errp);
> +}
> +
> +static Chardev *__qemu_chr_new(const char *label, const char *filename,
> +                               bool permit_mux_mon, GMainContext *context,
> +                               bool replay)
>  {
>      const char *p;
>      Chardev *chr;
> @@ -693,14 +720,22 @@ Chardev *qemu_chr_new_noreplay(const char *label, const char *filename,
>      Error *err = NULL;
>
>      if (strstart(filename, "chardev:", &p)) {
> -        return qemu_chr_find(p);
> +        chr = qemu_chr_find(p);
> +        if (replay) {
> +            qemu_chardev_set_replay(chr, &err);
> +            if (err) {
> +                error_report_err(err);
> +                return NULL;
> +            }
> +        }
> +        return chr;
>      }
>
>      opts = qemu_chr_parse_compat(label, filename, permit_mux_mon);
>      if (!opts)
>          return NULL;
>
> -    chr = qemu_chr_new_from_opts(opts, context, &err);
> +    chr = __qemu_chr_new_from_opts(opts, context, replay, &err);
>      if (!chr) {
>          error_report_err(err);
>          goto out;
> @@ -722,24 +757,18 @@ out:
>      return chr;
>  }
>
> +Chardev *qemu_chr_new_noreplay(const char *label, const char *filename,
> +                               bool permit_mux_mon, GMainContext *context)
> +{
> +    return __qemu_chr_new(label, filename, permit_mux_mon, context, false);
> +}
> +
>  static Chardev *qemu_chr_new_permit_mux_mon(const char *label,
>                                            const char *filename,
>                                            bool permit_mux_mon,
>                                            GMainContext *context)
>  {
> -    Chardev *chr;
> -    chr = qemu_chr_new_noreplay(label, filename, permit_mux_mon, context);
> -    if (chr) {
> -        if (replay_mode != REPLAY_MODE_NONE) {
> -            qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_REPLAY);
> -        }
> -        if (qemu_chr_replay(chr) && CHARDEV_GET_CLASS(chr)->chr_ioctl) {
> -            error_report("Replay: ioctl is not supported "
> -                         "for serial devices yet");
> -        }
> -        replay_register_char_driver(chr);
> -    }
> -    return chr;
> +    return __qemu_chr_new(label, filename, permit_mux_mon, context, true);
>  }
>
>  Chardev *qemu_chr_new(const char *label, const char *filename,
> --
> 2.42.0
>
>

It's probably too late for 9.0, splitting the patch and preliminary
cleanup should really help reviewing the change and pointing out the
behaviour differences.

thanks




-- 
Marc-André Lureau