Re: [PULL 15/21] chardev: set record/replay on the base device of a muxed device

[Peter Maydell]

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