Re: [Qemu-devel] [PATCH v4 1/5] replay: character devices

[Paolo Bonzini <pbonzini@redhat.com>]

On 10/03/2016 12:55, Pavel Dovgalyuk wrote:
> gdbstub which also acts as a backend is not recorded to allow controlling
> the replaying through gdb.

Perhaps the monitor too?

Overall the patch is nice and can definitely go in 2.6, but there are a
couple changes to do...

> @@ -245,6 +246,9 @@ int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len)
>          qemu_chr_fe_write_log(s, buf, ret);
>      }
>  
> +    if (s->replay) {
> +        replay_data_int(&ret);
> +    }

I think this is wrong.  The logic should be

    if (replaying) {
	read event(&ret);
	assert(ret <= len);
	len = ret;
    }

    qemu_mutex_lock(&s->chr_write_lock);
    ret = s->chr_write(s, buf, len);

    if (ret > 0) {
        qemu_chr_fe_write_log(s, buf, ret);
    }
    qemu_mutex_unlock(&s->chr_write_lock);

    if (recording) {
        write event(ret);
    }

>      qemu_mutex_unlock(&s->chr_write_lock);
>      return ret;
>  }
> @@ -318,9 +322,19 @@ int qemu_chr_fe_read_all(CharDriverState *s, uint8_t *buf, int len)
>  
>  int qemu_chr_fe_ioctl(CharDriverState *s, int cmd, void *arg)
>  {
> -    if (!s->chr_ioctl)
> -        return -ENOTSUP;
> -    return s->chr_ioctl(s, cmd, arg);
> +    int res;
> +    if (!s->chr_ioctl) {
> +        res = -ENOTSUP;
> +    } else {
> +        res = s->chr_ioctl(s, cmd, arg);
> +        if (s->replay) {
> +            fprintf(stderr,
> +                    "Replay: ioctl is not supported for serial devices yet\n");
> +            exit(1);

Is it possible to print this warning just once per device and return
-ENOTSUP instead?

> +void replay_register_char_driver(CharDriverState *chr)
> +{
> +    if (replay_mode == REPLAY_MODE_NONE) {
> +        return;
> +    }
> +    char_drivers = g_realloc(char_drivers,
> +                             sizeof(*char_drivers) * (drivers_count + 1));
> +    char_drivers[drivers_count++] = chr;
> +}

You need a way to unregister character drivers when they are
hot-unplugged, or at least you should block chardev-del if in record and
replay mode.

> +    /* for int data */
> +    EVENT_DATA_INT,

I think you should call the event EVENT_CHAR_WRITE (and perhaps rename
REPLAY_ASYNC_EVENT_CHAR to REPLAY_ASYNC_EVENT_CHAR_READ).  And as
mentioned above, I think the load and save cases should be separated in
qemu-char.c, so I'd prefer to have a separate function to read and write
the event as well.

Thanks,

Paolo