Hi
On Mon, Mar 11, 2024 at 9:44 PM Nicholas Piggin <npig...@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 <npig...@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
