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

[Maxime Coquelin]

On 04/17/2017 05:29 AM, Peter Xu wrote:
> 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.

Sure, it won't hurt. I'll add this in next revision.

> [...]
>
> > +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?

Right, thanks. It will be leaked otherwise.

> > +
> > +    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.

That make sense, I'll fix this.


Thanks!
Maxime
> Thanks,
>
> > +    }
> > +
> > +    return 0;
> > +}
> > +