Re: [Qemu-devel] [PATCH 07/11] chardev: add tty chardev support to chardev-add (qmp)

[Paolo Bonzini <pbonzini@redhat.com>]

Il 07/01/2013 14:55, Gerd Hoffmann ha scritto:
> Simliar to file, except that no separate in/out files are supported
> because it's pointless for direct device access.  Also the special
> tty ioctl hooks (pass through linespeed settings etc) are activated.
> Both file names and file descriptor passing is supported.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  qapi-schema.json |    6 ++++++
>  qemu-char.c      |   53 +++++++++++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 53 insertions(+), 6 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 8904d36..7e5c8c2 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3036,9 +3036,15 @@
>  { 'type': 'ChardevFile', 'data': { '*in' : 'ChardevFileSource',
>                                     'out' : 'ChardevFileSource' } }
>  
> +{ 'enum': 'ChardevPortKind', 'data': [ 'tty' ] }
> +
> +{ 'type': 'ChardevPort', 'data': { 'device' : 'ChardevFileSource',
> +                                   'type'   : 'ChardevPortKind'} }
> +
>  { 'type': 'ChardevDummy', 'data': { } }
>  
>  { 'union': 'ChardevBackend', 'data': { 'file' : 'ChardevFile',
> +                                       'port' : 'ChardevPort',
>                                         'null' : 'ChardevDummy' } }
>  
>  { 'command': 'chardev-add', 'data': {'id'      : 'str',
> diff --git a/qemu-char.c b/qemu-char.c
> index 91b0a57..764321b 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -1230,21 +1230,27 @@ static void qemu_chr_close_tty(CharDriverState *chr)
>      }
>  }
>  
> +static CharDriverState *qemu_chr_open_tty_fd(int fd)
> +{
> +    CharDriverState *chr;
> +
> +    tty_serial_init(fd, 115200, 'N', 8, 1);
> +    chr = qemu_chr_open_fd(fd, fd);
> +    chr->chr_ioctl = tty_serial_ioctl;
> +    chr->chr_close = qemu_chr_close_tty;
> +    return chr;
> +}
> +
>  static CharDriverState *qemu_chr_open_tty(QemuOpts *opts)
>  {
>      const char *filename = qemu_opt_get(opts, "path");
> -    CharDriverState *chr;
>      int fd;
>  
>      TFR(fd = qemu_open(filename, O_RDWR | O_NONBLOCK));
>      if (fd < 0) {
>          return NULL;
>      }
> -    tty_serial_init(fd, 115200, 'N', 8, 1);
> -    chr = qemu_chr_open_fd(fd, fd);
> -    chr->chr_ioctl = tty_serial_ioctl;
> -    chr->chr_close = qemu_chr_close_tty;
> -    return chr;
> +    return qemu_chr_open_tty_fd(fd);
>  }
>  #endif /* __linux__ || __sun__ */
>  
> @@ -2962,6 +2968,15 @@ static CharDriverState *qmp_chardev_open_file(ChardevFile *file, Error **errp)
>      return qemu_chr_open_win_file(out);
>  }
>  
> +static CharDriverState *qmp_chardev_open_port(ChardevPort *port, Error **errp)
> +{
> +    switch (port->type) {
> +    default:
> +        error_setg(errp, "unknown chardev port (%d)", port->type);
> +        return NULL;
> +    }
> +}
> +
>  #else /* WIN32 */
>  
>  static int qmp_chardev_open_file_source(ChardevFileSource *src, int flags,
> @@ -2978,6 +2993,9 @@ static int qmp_chardev_open_file_source(ChardevFileSource *src, int flags,
>          break;
>      case CHARDEV_FILE_SOURCE_KIND_FD:
>          fd = monitor_get_fd(cur_mon, src->fd, errp);
> +        if (flags & O_NONBLOCK) {
> +            socket_set_nonblock(fd);
> +        }
>          break;
>      default:
>          error_setg(errp, "unknown chardev file source (%d)", src->kind);
> @@ -3008,6 +3026,26 @@ static CharDriverState *qmp_chardev_open_file(ChardevFile *file, Error **errp)
>      return qemu_chr_open_fd(in, out);
>  }
>  
> +static CharDriverState *qmp_chardev_open_port(ChardevPort *port, Error **errp)
> +{
> +    int flags, fd;
> +
> +    switch (port->type) {
> +#ifdef HAVE_CHARDEV_TTY
> +    case CHARDEV_PORT_KIND_TTY:
> +        flags = O_RDWR | O_NONBLOCK;
> +        fd = qmp_chardev_open_file_source(port->device, flags, errp);
> +        if (error_is_set(errp)) {
> +            return NULL;
> +        }
> +        return qemu_chr_open_tty_fd(fd);
> +#endif
> +    default:
> +        error_setg(errp, "unknown chardev port (%d)", port->type);
> +        return NULL;
> +    }
> +}
> +
>  #endif /* WIN32 */
>  
>  void qmp_chardev_add(const char *id, ChardevBackend *backend, Error **errp)
> @@ -3018,6 +3056,9 @@ void qmp_chardev_add(const char *id, ChardevBackend *backend, Error **errp)
>      case CHARDEV_BACKEND_KIND_FILE:
>          chr = qmp_chardev_open_file(backend->file, errp);
>          break;
> +    case CHARDEV_BACKEND_KIND_PORT:
> +        chr = qmp_chardev_open_port(backend->port, errp);
> +        break;
>      case CHARDEV_BACKEND_KIND_NULL:
>          chr = qemu_chr_open_null(NULL);
>          break;
> 

Apart from the naming question (see my review of 8/11),

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo