Re: [Qemu-devel] [PATCH v1 2/4] vhost-user: Shutdown vhost-user connection when wrong messages are passed

[Stefan Hajnoczi]

On Fri, May 29, 2015 at 01:42:28PM +0900, Tetsuya Mukawa wrote:
> When wrong vhost-user message are passed, the connection should be
> shutdown.
> 
> Signed-off-by: Tetsuya Mukawa <address@hidden>
> ---
>  hw/virtio/vhost-user.c | 17 ++++++++++-------
>  include/sysemu/char.h  |  7 +++++++
>  qemu-char.c            | 15 +++++++++++++++
>  3 files changed, 32 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index e7ab829..4d7e3ba 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -183,6 +183,7 @@ static int vhost_user_write(struct vhost_dev *dev, 
> VhostUserMsg *msg,
>  static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
>          void *arg)
>  {
> +    CharDriverState *chr = dev->opaque;
>      VhostUserMsg msg;
>      VhostUserRequest msg_request;
>      struct vhost_vring_file *file = 0;
> @@ -237,7 +238,7 @@ static int vhost_user_call(struct vhost_dev *dev, 
> unsigned long int request,
>          if (!fd_num) {
>              error_report("Failed initializing vhost-user memory map, "
>                      "consider using -object memory-backend-file share=on");
> -            return -1;
> +            goto close;
>          }
>  
>          msg.size = sizeof(m.memory.nregions);
> @@ -281,7 +282,7 @@ static int vhost_user_call(struct vhost_dev *dev, 
> unsigned long int request,
>          break;
>      default:
>          error_report("vhost-user trying to send unhandled ioctl");
> -        return -1;
> +        goto close;
>          break;
>      }
>  
> @@ -297,32 +298,34 @@ static int vhost_user_call(struct vhost_dev *dev, 
> unsigned long int request,
>          if (msg_request != msg.request) {
>              error_report("Received unexpected msg type."
>                      " Expected %d received %d", msg_request, msg.request);
> -            return -1;
> +            goto close;
>          }
>  
>          switch (msg_request) {
>          case VHOST_USER_GET_FEATURES:
>              if (msg.size != sizeof(m.u64)) {
>                  error_report("Received bad msg size.");
> -                return -1;
> +                goto close;
>              }
>              *((__u64 *) arg) = msg.u64;
>              break;
>          case VHOST_USER_GET_VRING_BASE:
>              if (msg.size != sizeof(m.state)) {
>                  error_report("Received bad msg size.");
> -                return -1;
> +                goto close;
>              }
>              memcpy(arg, &msg.state, sizeof(struct vhost_vring_state));
>              break;
>          default:
>              error_report("Received unexpected msg type.");
> -            return -1;
> -            break;
>          }
>      }
>  
>      return 0;
> +
> +close:
> +    qemu_chr_shutdown(chr, SHUT_RDWR);
> +    return -1;
>  }

Why is shutdown(2) necessary?  We're aborting the connection and don't
expect to process any more data, so we could just close it.

> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index 832b7fe..d944ded 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -70,6 +70,7 @@ struct CharDriverState {
>      IOReadHandler *chr_read;
>      void *handler_opaque;
>      void (*chr_close)(struct CharDriverState *chr);
> +    void (*chr_shutdown)(struct CharDriverState *chr, int how);
>      void (*chr_accept_input)(struct CharDriverState *chr);
>      void (*chr_set_echo)(struct CharDriverState *chr, bool echo);
>      void (*chr_set_fe_open)(struct CharDriverState *chr, int fe_open);
> @@ -124,6 +125,12 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
>   */
>  CharDriverState *qemu_chr_new(const char *label, const char *filename,
>                                void (*init)(struct CharDriverState *s));
> +/**
> + * @qemu_chr_shutdown:
> + *
> + * Shutdown a fd of character backend.
> + */
> +void qemu_chr_shutdown(CharDriverState *chr, int how);
>  
>  /**
>   * @qemu_chr_delete:
> diff --git a/qemu-char.c b/qemu-char.c
> index d0c1564..2b28808 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2839,6 +2839,13 @@ static void tcp_chr_disconnect(CharDriverState *chr)
>      }
>  }
>  
> +static void tcp_chr_shutdown(CharDriverState *chr, int how)
> +{
> +    TCPCharDriver *s = chr->opaque;
> +
> +    shutdown(s->fd, how);
> +}
> +
>  static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void 
> *opaque)
>  {
>      CharDriverState *chr = opaque;
> @@ -3836,6 +3843,13 @@ void qemu_chr_fe_release(CharDriverState *s)
>      s->avail_connections++;
>  }
>  
> +void qemu_chr_shutdown(CharDriverState *chr, int how)
> +{
> +    if (chr->chr_shutdown) {
> +        chr->chr_shutdown(chr, how);
> +    }
> +}
> +
>  void qemu_chr_delete(CharDriverState *chr)
>  {
>      QTAILQ_REMOVE(&chardevs, chr, next);
> @@ -4154,6 +4168,7 @@ static CharDriverState 
> *qmp_chardev_open_socket(ChardevSocket *sock,
>      chr->chr_write = tcp_chr_write;
>      chr->chr_sync_read = tcp_chr_sync_read;
>      chr->chr_close = tcp_chr_close;
> +    chr->chr_shutdown = tcp_chr_shutdown;
>      chr->get_msgfds = tcp_get_msgfds;
>      chr->set_msgfds = tcp_set_msgfds;
>      chr->chr_add_client = tcp_chr_add_client;

Please split this into a separate qemu-char.c patch.  I hesitate to add
a shutdown(2) interface since it adds a new state that the rest of the
qemu-char.c code doesn't know about.

pgpW0wfJsDTsr.pgp
Description: PGP signature