Re: [Qemu-devel] [RFC v2 3/4] vhost-user: add slave-req-fd support

[Peter Xu <peterx@redhat.com>]

On Fri, Apr 14, 2017 at 07:40:55PM +0200, Maxime Coquelin wrote:

[...]

> @@ -486,6 +500,18 @@ Message types
>        If VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated, slave must respond
>        with zero in case the specified MTU is valid, or non-zero otherwise.
>  
> + * VHOST_USER_SET_SLAVE_REQ_FD
> +
> +      Id: 21
> +      Equivalent ioctl: N/A
> +      Master payload: N/A
> +
> +      Set the socket file descriptor for slave initiated requests. It is passed
> +      in the ancillary data.
> +      This request should be sent only when VHOST_USER_F_PROTOCOL_FEATURES
> +      has been negotiated, and protocol feature bit VHOST_USER_PROTOCOL_F_SLAVE_REQ
> +      bit is present in VHOST_USER_GET_PROTOCOL_FEATURES.

Here, do we need to mention REPLY_ACK as well? Like:

      If VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated, slave must
      respond with zero in case the slave request channel is setup
      correctly, or non-zero otherwise.

Since I see the other two users are mentioning it.

[...]

> +static int vhost_setup_slave_channel(struct vhost_dev *dev)
> +{
> +    VhostUserMsg msg = {
> +        .request = VHOST_USER_SET_SLAVE_REQ_FD,
> +        .flags = VHOST_USER_VERSION,
> +    };
> +    struct vhost_user *u = dev->opaque;
> +    int sv[2];
> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> +                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
> +
> +    if (!virtio_has_feature(dev->protocol_features,
> +                            VHOST_USER_PROTOCOL_F_SLAVE_REQ)) {
> +        return 0;
> +    }
> +
> +    if (socketpair(PF_UNIX, SOCK_STREAM, 0, sv) == -1) {
> +        error_report("socketpair() failed");
> +        return -1;
> +    }
> +
> +    u->slave_fd = sv[0];
> +    qemu_set_fd_handler(u->slave_fd, slave_read, NULL, dev);
> +
> +    if (reply_supported) {
> +        msg.flags |= VHOST_USER_NEED_REPLY_MASK;
> +    }
> +
> +    vhost_user_write(dev, &msg, &sv[1], 1);

Do we need to close(sv[1]) afterward?

> +
> +    if (reply_supported) {
> +        return process_message_reply(dev, msg.request);

Here do we need to cleanup u->slave_fd if backend replied something
wrong? Or I guess it might be leaked.

Thanks,

> +    }
> +
> +    return 0;
> +}
> +

-- 
Peter Xu