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

[Maxime Coquelin <maxime.coquelin@redhat.com>]

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;
>> +}
>> +
>