From: Nicholas Piggin <npiggin@gmail.com>

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.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Message-Id: <20240813050638.446172-8-npiggin@gmail.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20240813202329.1237572-16-alex.bennee@linaro.org>

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;
+        }
+        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)
 {
     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? */
+    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.39.2

On Thu, 15 Aug 2024 at 15:53, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> From: Nicholas Piggin <npiggin@gmail.com>
>
> 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.

Hi; Coverity points out a bug in this code (CID 1559470):

> -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;
> +        }
> +        qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_REPLAY);
> +        replay_register_char_driver(chr);
> +    }
> +}

qemu_chardev_set_replay() assumes it is passed a non NULL
'chr' pointer...

> @@ -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);

...but qemu_chr_find() returns NULL if it can't find the
chardev, and we don't catch that here...

> +        if (replay) {
> +            qemu_chardev_set_replay(chr, &err);

...so we will pass it to qemu_chardev_set_replay(), which
dumps core:

$ ./build/x86/qemu-system-arm -icount
shift=auto,rr=record,rrfile=replay.bin  -serial chardev:bang -M virt
Segmentation fault (core dumped)

(Compare the non-rr behaviour:
$ ./build/x86/qemu-system-arm  -serial chardev:bang -M virt
qemu-system-arm: -serial chardev:bang: could not connect serial device
to character backend 'chardev:bang'
)

Would you mind sending in a patch to fix this?

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

Side note: the "__" prefix is reserved for the system, so
we don't generally use it in QEMU function names. Could
you also submit a patch to rename the __qemu_chr_new()
and __qemu_chr_new_from_opts() functions, please?
(One common pattern for this kind of "function that does
the actual work behind foo()" is to call it "do_foo()".)

thanks
-- PMM