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