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

["Nicholas Piggin" <npiggin@gmail.com>]

On Tue Mar 12, 2024 at 10:39 PM AEST, Marc-André Lureau wrote:
> 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.

Ah yes true, I will 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.

Yeah I agree they're not good, I tried to think of a better name.

>
> >  {>      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?

Good point. I was wondering whether devices created by this
call should have record/replay enabled.

>
> > +    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.

Agree. Fixing the rr flag setting could be considered a bug fix.
So a minimal patch that does not change other behaviour might be
suitable for 9.0. But that could be decided at the time, no need
to rush this in before the soft freeze.

Thanks,
Nick