[dpdk-dev] [PATCH v2 0/5] vhost_user.c code cleanup

[dpdk-dev] [PATCH v2 0/5] vhost_user.c code cleanup

[Nikolay Nikolaev]

vhost: vhost_user.c code cleanup

This patchesries introduce a set of code redesigns in vhost_user.c.

The goal is to unify and simplify vhost-user message handling. The
patches do not intend to introduce any functional changes.

v2 changes:
 - Fix the comments by Tiwei Bie
 - Keep the old behavior
   - Fall through when the callback returns VH_RESULT_ERR
   - Fall through if the request is out of range

---

Nikolay Nikolaev (5):
      vhost: unify VhostUserMsg usage
      vhost: make message handling functions prepare the reply
      vhost: handle unsupported message types in functions
      vhost: unify message handling function signature
      vhost: message handling implemented as a callback array


 lib/librte_vhost/vhost_user.c |  392 ++++++++++++++++++++++-------------------
 1 file changed, 208 insertions(+), 184 deletions(-)

--
Signature

[dpdk-dev] [PATCH v2 1/5] vhost: unify VhostUserMsg usage

[Nikolay Nikolaev]

Use the typedef version of struct VhostUserMsg. Also unify the related
parameter name.

Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
---
 lib/librte_vhost/vhost_user.c |   50 +++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index dc53ff712..d51616695 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -778,10 +778,10 @@ vhost_memory_changed(struct VhostUserMemory *new,
 }
 
 static int
-vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *pmsg)
+vhost_user_set_mem_table(struct virtio_net **pdev, VhostUserMsg *msg)
 {
 	struct virtio_net *dev = *pdev;
-	struct VhostUserMemory memory = pmsg->payload.memory;
+	struct VhostUserMemory memory = msg->payload.memory;
 	struct rte_vhost_mem_region *reg;
 	void *mmap_addr;
 	uint64_t mmap_size;
@@ -802,7 +802,7 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *pmsg)
 			"(%d) memory regions not changed\n", dev->vid);
 
 		for (i = 0; i < memory.nregions; i++)
-			close(pmsg->fds[i]);
+			close(msg->fds[i]);
 
 		return 0;
 	}
@@ -838,7 +838,7 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *pmsg)
 	dev->mem->nregions = memory.nregions;
 
 	for (i = 0; i < memory.nregions; i++) {
-		fd  = pmsg->fds[i];
+		fd  = msg->fds[i];
 		reg = &dev->mem->regions[i];
 
 		reg->guest_phys_addr = memory.regions[i].guest_phys_addr;
@@ -987,16 +987,16 @@ virtio_is_ready(struct virtio_net *dev)
 }
 
 static void
-vhost_user_set_vring_call(struct virtio_net *dev, struct VhostUserMsg *pmsg)
+vhost_user_set_vring_call(struct virtio_net *dev, VhostUserMsg *msg)
 {
 	struct vhost_vring_file file;
 	struct vhost_virtqueue *vq;
 
-	file.index = pmsg->payload.u64 & VHOST_USER_VRING_IDX_MASK;
-	if (pmsg->payload.u64 & VHOST_USER_VRING_NOFD_MASK)
+	file.index = msg->payload.u64 & VHOST_USER_VRING_IDX_MASK;
+	if (msg->payload.u64 & VHOST_USER_VRING_NOFD_MASK)
 		file.fd = VIRTIO_INVALID_EVENTFD;
 	else
-		file.fd = pmsg->fds[0];
+		file.fd = msg->fds[0];
 	RTE_LOG(INFO, VHOST_CONFIG,
 		"vring call idx:%d file:%d\n", file.index, file.fd);
 
@@ -1008,17 +1008,17 @@ vhost_user_set_vring_call(struct virtio_net *dev, struct VhostUserMsg *pmsg)
 }
 
 static void
-vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *pmsg)
+vhost_user_set_vring_kick(struct virtio_net **pdev, VhostUserMsg *msg)
 {
 	struct vhost_vring_file file;
 	struct vhost_virtqueue *vq;
 	struct virtio_net *dev = *pdev;
 
-	file.index = pmsg->payload.u64 & VHOST_USER_VRING_IDX_MASK;
-	if (pmsg->payload.u64 & VHOST_USER_VRING_NOFD_MASK)
+	file.index = msg->payload.u64 & VHOST_USER_VRING_IDX_MASK;
+	if (msg->payload.u64 & VHOST_USER_VRING_NOFD_MASK)
 		file.fd = VIRTIO_INVALID_EVENTFD;
 	else
-		file.fd = pmsg->fds[0];
+		file.fd = msg->fds[0];
 	RTE_LOG(INFO, VHOST_CONFIG,
 		"vring kick idx:%d file:%d\n", file.index, file.fd);
 
@@ -1145,7 +1145,7 @@ vhost_user_set_vring_enable(struct virtio_net *dev,
 
 static void
 vhost_user_get_protocol_features(struct virtio_net *dev,
-				 struct VhostUserMsg *msg)
+				 VhostUserMsg *msg)
 {
 	uint64_t features, protocol_features;
 
@@ -1176,7 +1176,7 @@ vhost_user_set_protocol_features(struct virtio_net *dev,
 }
 
 static int
-vhost_user_set_log_base(struct virtio_net *dev, struct VhostUserMsg *msg)
+vhost_user_set_log_base(struct virtio_net *dev, VhostUserMsg *msg)
 {
 	int fd = msg->fds[0];
 	uint64_t size, off;
@@ -1243,7 +1243,7 @@ vhost_user_set_log_base(struct virtio_net *dev, struct VhostUserMsg *msg)
  * a flag 'broadcast_rarp' to let rte_vhost_dequeue_burst() inject it.
  */
 static int
-vhost_user_send_rarp(struct virtio_net *dev, struct VhostUserMsg *msg)
+vhost_user_send_rarp(struct virtio_net *dev, VhostUserMsg *msg)
 {
 	uint8_t *mac = (uint8_t *)&msg->payload.u64;
 	struct rte_vdpa_device *vdpa_dev;
@@ -1272,7 +1272,7 @@ vhost_user_send_rarp(struct virtio_net *dev, struct VhostUserMsg *msg)
 }
 
 static int
-vhost_user_net_set_mtu(struct virtio_net *dev, struct VhostUserMsg *msg)
+vhost_user_net_set_mtu(struct virtio_net *dev, VhostUserMsg *msg)
 {
 	if (msg->payload.u64 < VIRTIO_MIN_MTU ||
 			msg->payload.u64 > VIRTIO_MAX_MTU) {
@@ -1288,7 +1288,7 @@ vhost_user_net_set_mtu(struct virtio_net *dev, struct VhostUserMsg *msg)
 }
 
 static int
-vhost_user_set_req_fd(struct virtio_net *dev, struct VhostUserMsg *msg)
+vhost_user_set_req_fd(struct virtio_net *dev, VhostUserMsg *msg)
 {
 	int fd = msg->fds[0];
 
@@ -1354,7 +1354,7 @@ is_vring_iotlb_invalidate(struct vhost_virtqueue *vq,
 }
 
 static int
-vhost_user_iotlb_msg(struct virtio_net **pdev, struct VhostUserMsg *msg)
+vhost_user_iotlb_msg(struct virtio_net **pdev, VhostUserMsg *msg)
 {
 	struct virtio_net *dev = *pdev;
 	struct vhost_iotlb_msg *imsg = &msg->payload.iotlb;
@@ -1400,7 +1400,7 @@ vhost_user_iotlb_msg(struct virtio_net **pdev, struct VhostUserMsg *msg)
 
 /* return bytes# of read on success or negative val on failure. */
 static int
-read_vhost_message(int sockfd, struct VhostUserMsg *msg)
+read_vhost_message(int sockfd, VhostUserMsg *msg)
 {
 	int ret;
 
@@ -1429,7 +1429,7 @@ read_vhost_message(int sockfd, struct VhostUserMsg *msg)
 }
 
 static int
-send_vhost_message(int sockfd, struct VhostUserMsg *msg, int *fds, int fd_num)
+send_vhost_message(int sockfd, VhostUserMsg *msg, int *fds, int fd_num)
 {
 	if (!msg)
 		return 0;
@@ -1439,7 +1439,7 @@ send_vhost_message(int sockfd, struct VhostUserMsg *msg, int *fds, int fd_num)
 }
 
 static int
-send_vhost_reply(int sockfd, struct VhostUserMsg *msg)
+send_vhost_reply(int sockfd, VhostUserMsg *msg)
 {
 	if (!msg)
 		return 0;
@@ -1453,7 +1453,7 @@ send_vhost_reply(int sockfd, struct VhostUserMsg *msg)
 }
 
 static int
-send_vhost_slave_message(struct virtio_net *dev, struct VhostUserMsg *msg,
+send_vhost_slave_message(struct virtio_net *dev, VhostUserMsg *msg,
 			 int *fds, int fd_num)
 {
 	int ret;
@@ -1544,7 +1544,7 @@ int
 vhost_user_msg_handler(int vid, int fd)
 {
 	struct virtio_net *dev;
-	struct VhostUserMsg msg;
+	VhostUserMsg msg;
 	struct rte_vdpa_device *vdpa_dev;
 	int did = -1;
 	int ret;
@@ -1832,7 +1832,7 @@ int
 vhost_user_iotlb_miss(struct virtio_net *dev, uint64_t iova, uint8_t perm)
 {
 	int ret;
-	struct VhostUserMsg msg = {
+	VhostUserMsg msg = {
 		.request.slave = VHOST_USER_SLAVE_IOTLB_MSG,
 		.flags = VHOST_USER_VERSION,
 		.size = sizeof(msg.payload.iotlb),
@@ -1862,7 +1862,7 @@ static int vhost_user_slave_set_vring_host_notifier(struct virtio_net *dev,
 	int *fdp = NULL;
 	size_t fd_num = 0;
 	int ret;
-	struct VhostUserMsg msg = {
+	VhostUserMsg msg = {
 		.request.slave = VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG,
 		.flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY,
 		.size = sizeof(msg.payload.area),

Re: [dpdk-dev] [PATCH v2 1/5] vhost: unify VhostUserMsg usage

[Maxime Coquelin]

Hi Nikolay,

On 07/19/2018 09:13 PM, Nikolay Nikolaev wrote:
> Use the typedef version of struct VhostUserMsg. Also unify the related
> parameter name.
> 
> Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
> ---
>   lib/librte_vhost/vhost_user.c |   50 +++++++++++++++++++++--------------------
>   1 file changed, 25 insertions(+), 25 deletions(-)
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

[dpdk-dev] [PATCH v2 2/5] vhost: make message handling functions prepare the reply

[Nikolay Nikolaev]

As VhostUserMsg structure is resued to generate the reply, move the
relevant fields update into the respective message handling functions.

Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
---
 lib/librte_vhost/vhost_user.c |   26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index d51616695..e97b2d563 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -146,11 +146,15 @@ vhost_user_reset_owner(struct virtio_net *dev)
  * The features that we support are requested.
  */
 static uint64_t
-vhost_user_get_features(struct virtio_net *dev)
+vhost_user_get_features(struct virtio_net *dev, VhostUserMsg *msg)
 {
 	uint64_t features = 0;
 
 	rte_vhost_driver_get_features(dev->ifname, &features);
+
+	msg->payload.u64 = features;
+	msg->size = sizeof(msg->payload.u64);
+
 	return features;
 }
 
@@ -158,11 +162,15 @@ vhost_user_get_features(struct virtio_net *dev)
  * The queue number that we support are requested.
  */
 static uint32_t
-vhost_user_get_queue_num(struct virtio_net *dev)
+vhost_user_get_queue_num(struct virtio_net *dev, VhostUserMsg *msg)
 {
 	uint32_t queue_num = 0;
 
 	rte_vhost_driver_get_queue_num(dev->ifname, &queue_num);
+
+	msg->payload.u64 = (uint64_t)queue_num;
+	msg->size = sizeof(msg->payload.u64);
+
 	return queue_num;
 }
 
@@ -1109,6 +1117,8 @@ vhost_user_get_vring_base(struct virtio_net *dev,
 	rte_free(vq->batch_copy_elems);
 	vq->batch_copy_elems = NULL;
 
+	msg->size = sizeof(msg->payload.state);
+
 	return 0;
 }
 
@@ -1231,6 +1241,8 @@ vhost_user_set_log_base(struct virtio_net *dev, VhostUserMsg *msg)
 	dev->log_base = dev->log_addr + off;
 	dev->log_size = size;
 
+	msg->size = sizeof(msg->payload.u64);
+
 	return 0;
 }
 
@@ -1644,8 +1656,7 @@ vhost_user_msg_handler(int vid, int fd)
 
 	switch (msg.request.master) {
 	case VHOST_USER_GET_FEATURES:
-		msg.payload.u64 = vhost_user_get_features(dev);
-		msg.size = sizeof(msg.payload.u64);
+		vhost_user_get_features(dev, &msg);
 		send_vhost_reply(fd, &msg);
 		break;
 	case VHOST_USER_SET_FEATURES:
@@ -1675,9 +1686,6 @@ vhost_user_msg_handler(int vid, int fd)
 
 	case VHOST_USER_SET_LOG_BASE:
 		vhost_user_set_log_base(dev, &msg);
-
-		/* it needs a reply */
-		msg.size = sizeof(msg.payload.u64);
 		send_vhost_reply(fd, &msg);
 		break;
 	case VHOST_USER_SET_LOG_FD:
@@ -1697,7 +1705,6 @@ vhost_user_msg_handler(int vid, int fd)
 
 	case VHOST_USER_GET_VRING_BASE:
 		vhost_user_get_vring_base(dev, &msg);
-		msg.size = sizeof(msg.payload.state);
 		send_vhost_reply(fd, &msg);
 		break;
 
@@ -1715,8 +1722,7 @@ vhost_user_msg_handler(int vid, int fd)
 		break;
 
 	case VHOST_USER_GET_QUEUE_NUM:
-		msg.payload.u64 = (uint64_t)vhost_user_get_queue_num(dev);
-		msg.size = sizeof(msg.payload.u64);
+		vhost_user_get_queue_num(dev, &msg);
 		send_vhost_reply(fd, &msg);
 		break;
 

Re: [dpdk-dev] [PATCH v2 2/5] vhost: make message handling functions prepare the reply

[Maxime Coquelin]

On 07/19/2018 09:13 PM, Nikolay Nikolaev wrote:
> As VhostUserMsg structure is resued to generate the reply, move the

s/resued/reused/

> relevant fields update into the respective message handling functions.
> 
> Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
> ---
>   lib/librte_vhost/vhost_user.c |   26 ++++++++++++++++----------
>   1 file changed, 16 insertions(+), 10 deletions(-)
> 

Agree this is cleaner:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

For the typo, don't resend the series just for this if this is the only
thing to fix. I suspect a rebase may be required anyway as it may
conflict with a patch from Ilya.

Thanks,
Maxime

[dpdk-dev] [PATCH v2 3/5] vhost: handle unsupported message types in functions

[Nikolay Nikolaev]

Add new functions to handle the unsupported vhost message types:
 - vhost_user_set_vring_err
 - vhost_user_set_log_fd

Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
---
 lib/librte_vhost/vhost_user.c |   22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index e97b2d563..b408460f0 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -1015,6 +1015,14 @@ vhost_user_set_vring_call(struct virtio_net *dev, VhostUserMsg *msg)
 	vq->callfd = file.fd;
 }
 
+static void vhost_user_set_vring_err(struct virtio_net *dev __rte_unused,
+			VhostUserMsg *msg)
+{
+	if (!(msg->payload.u64 & VHOST_USER_VRING_NOFD_MASK))
+		close(msg->fds[0]);
+	RTE_LOG(INFO, VHOST_CONFIG, "not implemented\n");
+}
+
 static void
 vhost_user_set_vring_kick(struct virtio_net **pdev, VhostUserMsg *msg)
 {
@@ -1246,6 +1254,13 @@ vhost_user_set_log_base(struct virtio_net *dev, VhostUserMsg *msg)
 	return 0;
 }
 
+static void
+vhost_user_set_log_fd(struct virtio_net *dev __rte_unused, VhostUserMsg *msg)
+{
+	close(msg->fds[0]);
+	RTE_LOG(INFO, VHOST_CONFIG, "not implemented.\n");
+}
+
 /*
  * An rarp packet is constructed and broadcasted to notify switches about
  * the new location of the migrated VM, so that packets from outside will
@@ -1689,8 +1704,7 @@ vhost_user_msg_handler(int vid, int fd)
 		send_vhost_reply(fd, &msg);
 		break;
 	case VHOST_USER_SET_LOG_FD:
-		close(msg.fds[0]);
-		RTE_LOG(INFO, VHOST_CONFIG, "not implemented.\n");
+		vhost_user_set_log_fd(dev, &msg);
 		break;
 
 	case VHOST_USER_SET_VRING_NUM:
@@ -1716,9 +1730,7 @@ vhost_user_msg_handler(int vid, int fd)
 		break;
 
 	case VHOST_USER_SET_VRING_ERR:
-		if (!(msg.payload.u64 & VHOST_USER_VRING_NOFD_MASK))
-			close(msg.fds[0]);
-		RTE_LOG(INFO, VHOST_CONFIG, "not implemented\n");
+		vhost_user_set_vring_err(dev, &msg);
 		break;
 
 	case VHOST_USER_GET_QUEUE_NUM:

Re: [dpdk-dev] [PATCH v2 3/5] vhost: handle unsupported message types in functions

[Maxime Coquelin]

On 07/19/2018 09:13 PM, Nikolay Nikolaev wrote:
> Add new functions to handle the unsupported vhost message types:
>   - vhost_user_set_vring_err
>   - vhost_user_set_log_fd
> 
> Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
> ---
>   lib/librte_vhost/vhost_user.c |   22 +++++++++++++++++-----
>   1 file changed, 17 insertions(+), 5 deletions(-)

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

[dpdk-dev] [PATCH v2 4/5] vhost: unify message handling function signature

[Nikolay Nikolaev]

Each vhost-user message handling function will return an int result
which is described in the new enum vh_result: error, OK and reply.
All functions will now have two arguments, virtio_net double pointer
and VhostUserMsg pointer.

Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
---
 lib/librte_vhost/vhost_user.c |  219 ++++++++++++++++++++++++-----------------
 1 file changed, 130 insertions(+), 89 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index b408460f0..6b39d1e30 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -71,6 +71,16 @@ static const char *vhost_message_str[VHOST_USER_MAX] = {
 	[VHOST_USER_CRYPTO_CLOSE_SESS] = "VHOST_USER_CRYPTO_CLOSE_SESS",
 };
 
+/* The possible results of a message handling function */
+enum vh_result {
+	/* Message handling failed */
+	VH_RESULT_ERR   = -1,
+	/* Message handling successful */
+	VH_RESULT_OK    =  0,
+	/* Message handling successful and reply prepared */
+	VH_RESULT_REPLY =  1,
+};
+
 static uint64_t
 get_blk_size(int fd)
 {
@@ -127,27 +137,31 @@ vhost_backend_cleanup(struct virtio_net *dev)
  * the device hasn't been initialised.
  */
 static int
-vhost_user_set_owner(void)
+vhost_user_set_owner(struct virtio_net **pdev __rte_unused,
+		VhostUserMsg * msg __rte_unused)
 {
-	return 0;
+	return VH_RESULT_OK;
 }
 
 static int
-vhost_user_reset_owner(struct virtio_net *dev)
+vhost_user_reset_owner(struct virtio_net **pdev,
+		VhostUserMsg * msg __rte_unused)
 {
+	struct virtio_net *dev = *pdev;
 	vhost_destroy_device_notify(dev);
 
 	cleanup_device(dev, 0);
 	reset_device(dev);
-	return 0;
+	return VH_RESULT_OK;
 }
 
 /*
  * The features that we support are requested.
  */
-static uint64_t
-vhost_user_get_features(struct virtio_net *dev, VhostUserMsg *msg)
+static int
+vhost_user_get_features(struct virtio_net **pdev, VhostUserMsg *msg)
 {
+	struct virtio_net *dev = *pdev;
 	uint64_t features = 0;
 
 	rte_vhost_driver_get_features(dev->ifname, &features);
@@ -155,15 +169,16 @@ vhost_user_get_features(struct virtio_net *dev, VhostUserMsg *msg)
 	msg->payload.u64 = features;
 	msg->size = sizeof(msg->payload.u64);
 
-	return features;
+	return VH_RESULT_REPLY;
 }
 
 /*
  * The queue number that we support are requested.
  */
-static uint32_t
-vhost_user_get_queue_num(struct virtio_net *dev, VhostUserMsg *msg)
+static int
+vhost_user_get_queue_num(struct virtio_net **pdev, VhostUserMsg *msg)
 {
+	struct virtio_net *dev = *pdev;
 	uint32_t queue_num = 0;
 
 	rte_vhost_driver_get_queue_num(dev->ifname, &queue_num);
@@ -171,15 +186,17 @@ vhost_user_get_queue_num(struct virtio_net *dev, VhostUserMsg *msg)
 	msg->payload.u64 = (uint64_t)queue_num;
 	msg->size = sizeof(msg->payload.u64);
 
-	return queue_num;
+	return VH_RESULT_REPLY;
 }
 
 /*
  * We receive the negotiated features supported by us and the virtio device.
  */
 static int
-vhost_user_set_features(struct virtio_net *dev, uint64_t features)
+vhost_user_set_features(struct virtio_net **pdev, VhostUserMsg *msg)
 {
+	struct virtio_net *dev = *pdev;
+	uint64_t features = msg->payload.u64;
 	uint64_t vhost_features = 0;
 	struct rte_vdpa_device *vdpa_dev;
 	int did = -1;
@@ -189,12 +206,12 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features)
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"(%d) received invalid negotiated features.\n",
 			dev->vid);
-		return -1;
+		return VH_RESULT_ERR;
 	}
 
 	if (dev->flags & VIRTIO_DEV_RUNNING) {
 		if (dev->features == features)
-			return 0;
+			return VH_RESULT_OK;
 
 		/*
 		 * Error out if master tries to change features while device is
@@ -205,7 +222,7 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features)
 			RTE_LOG(ERR, VHOST_CONFIG,
 				"(%d) features changed while device is running.\n",
 				dev->vid);
-			return -1;
+			return VH_RESULT_ERR;
 		}
 
 		if (dev->notify_ops->features_changed)
@@ -250,16 +267,17 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features)
 	if (vdpa_dev && vdpa_dev->ops->set_features)
 		vdpa_dev->ops->set_features(dev->vid);
 
-	return 0;
+	return VH_RESULT_OK;
 }
 
 /*
  * The virtio device sends us the size of the descriptor ring.
  */
 static int
-vhost_user_set_vring_num(struct virtio_net *dev,
+vhost_user_set_vring_num(struct virtio_net **pdev,
 			 VhostUserMsg *msg)
 {
+	struct virtio_net *dev = *pdev;
 	struct vhost_virtqueue *vq = dev->virtqueue[msg->payload.state.index];
 
 	vq->size = msg->payload.state.num;
@@ -272,7 +290,7 @@ vhost_user_set_vring_num(struct virtio_net *dev,
 	if ((vq->size & (vq->size - 1)) || vq->size > 32768) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"invalid virtqueue size %u\n", vq->size);
-		return -1;
+		return VH_RESULT_ERR;
 	}
 
 	if (dev->dequeue_zero_copy) {
@@ -298,7 +316,7 @@ vhost_user_set_vring_num(struct virtio_net *dev,
 		if (!vq->shadow_used_packed) {
 			RTE_LOG(ERR, VHOST_CONFIG,
 					"failed to allocate memory for shadow used ring.\n");
-			return -1;
+			return VH_RESULT_ERR;
 		}
 
 	} else {
@@ -308,7 +326,7 @@ vhost_user_set_vring_num(struct virtio_net *dev,
 		if (!vq->shadow_used_split) {
 			RTE_LOG(ERR, VHOST_CONFIG,
 					"failed to allocate memory for shadow used ring.\n");
-			return -1;
+			return VH_RESULT_ERR;
 		}
 	}
 
@@ -318,10 +336,10 @@ vhost_user_set_vring_num(struct virtio_net *dev,
 	if (!vq->batch_copy_elems) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"failed to allocate memory for batching copy.\n");
-		return -1;
+		return VH_RESULT_ERR;
 	}
 
-	return 0;
+	return VH_RESULT_OK;
 }
 
 /*
@@ -619,12 +637,12 @@ translate_ring_addresses(struct virtio_net *dev, int vq_index)
 static int
 vhost_user_set_vring_addr(struct virtio_net **pdev, VhostUserMsg *msg)
 {
+	struct virtio_net *dev = *pdev;
 	struct vhost_virtqueue *vq;
 	struct vhost_vring_addr *addr = &msg->payload.addr;
-	struct virtio_net *dev = *pdev;
 
 	if (dev->mem == NULL)
-		return -1;
+		return VH_RESULT_ERR;
 
 	/* addr->index refers to the queue index. The txq 1, rxq is 0. */
 	vq = dev->virtqueue[msg->payload.addr.index];
@@ -641,27 +659,28 @@ vhost_user_set_vring_addr(struct virtio_net **pdev, VhostUserMsg *msg)
 				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) {
 		dev = translate_ring_addresses(dev, msg->payload.addr.index);
 		if (!dev)
-			return -1;
+			return VH_RESULT_ERR;
 
 		*pdev = dev;
 	}
 
-	return 0;
+	return VH_RESULT_OK;
 }
 
 /*
  * The virtio device sends us the available ring last used index.
  */
 static int
-vhost_user_set_vring_base(struct virtio_net *dev,
+vhost_user_set_vring_base(struct virtio_net **pdev,
 			  VhostUserMsg *msg)
 {
+	struct virtio_net *dev = *pdev;
 	dev->virtqueue[msg->payload.state.index]->last_used_idx  =
 			msg->payload.state.num;
 	dev->virtqueue[msg->payload.state.index]->last_avail_idx =
 			msg->payload.state.num;
 
-	return 0;
+	return VH_RESULT_OK;
 }
 
 static int
@@ -802,7 +821,7 @@ vhost_user_set_mem_table(struct virtio_net **pdev, VhostUserMsg *msg)
 	if (memory.nregions > VHOST_MEMORY_MAX_NREGIONS) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"too many memory regions (%u)\n", memory.nregions);
-		return -1;
+		return VH_RESULT_ERR;
 	}
 
 	if (dev->mem && !vhost_memory_changed(&memory, dev->mem)) {
@@ -812,7 +831,7 @@ vhost_user_set_mem_table(struct virtio_net **pdev, VhostUserMsg *msg)
 		for (i = 0; i < memory.nregions; i++)
 			close(msg->fds[i]);
 
-		return 0;
+		return VH_RESULT_OK;
 	}
 
 	if (dev->mem) {
@@ -831,7 +850,7 @@ vhost_user_set_mem_table(struct virtio_net **pdev, VhostUserMsg *msg)
 				"(%d) failed to allocate memory "
 				"for dev->guest_pages\n",
 				dev->vid);
-			return -1;
+			return VH_RESULT_ERR;
 		}
 	}
 
@@ -841,7 +860,7 @@ vhost_user_set_mem_table(struct virtio_net **pdev, VhostUserMsg *msg)
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"(%d) failed to allocate memory for dev->mem\n",
 			dev->vid);
-		return -1;
+		return VH_RESULT_ERR;
 	}
 	dev->mem->nregions = memory.nregions;
 
@@ -938,7 +957,7 @@ vhost_user_set_mem_table(struct virtio_net **pdev, VhostUserMsg *msg)
 
 			dev = translate_ring_addresses(dev, i);
 			if (!dev)
-				return -1;
+				return VH_RESULT_ERR;
 
 			*pdev = dev;
 		}
@@ -946,13 +965,13 @@ vhost_user_set_mem_table(struct virtio_net **pdev, VhostUserMsg *msg)
 
 	dump_guest_pages(dev);
 
-	return 0;
+	return VH_RESULT_OK;
 
 err_mmap:
 	free_mem_region(dev);
 	rte_free(dev->mem);
 	dev->mem = NULL;
-	return -1;
+	return VH_RESULT_ERR;
 }
 
 static bool
@@ -994,9 +1013,10 @@ virtio_is_ready(struct virtio_net *dev)
 	return 1;
 }
 
-static void
-vhost_user_set_vring_call(struct virtio_net *dev, VhostUserMsg *msg)
+static int
+vhost_user_set_vring_call(struct virtio_net **pdev, VhostUserMsg *msg)
 {
+	struct virtio_net *dev = *pdev;
 	struct vhost_vring_file file;
 	struct vhost_virtqueue *vq;
 
@@ -1013,22 +1033,26 @@ vhost_user_set_vring_call(struct virtio_net *dev, VhostUserMsg *msg)
 		close(vq->callfd);
 
 	vq->callfd = file.fd;
+
+	return VH_RESULT_OK;
 }
 
-static void vhost_user_set_vring_err(struct virtio_net *dev __rte_unused,
+static int vhost_user_set_vring_err(struct virtio_net **pdev __rte_unused,
 			VhostUserMsg *msg)
 {
 	if (!(msg->payload.u64 & VHOST_USER_VRING_NOFD_MASK))
 		close(msg->fds[0]);
 	RTE_LOG(INFO, VHOST_CONFIG, "not implemented\n");
+
+	return VH_RESULT_OK;
 }
 
-static void
+static int
 vhost_user_set_vring_kick(struct virtio_net **pdev, VhostUserMsg *msg)
 {
+	struct virtio_net *dev = *pdev;
 	struct vhost_vring_file file;
 	struct vhost_virtqueue *vq;
-	struct virtio_net *dev = *pdev;
 
 	file.index = msg->payload.u64 & VHOST_USER_VRING_IDX_MASK;
 	if (msg->payload.u64 & VHOST_USER_VRING_NOFD_MASK)
@@ -1041,7 +1065,7 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, VhostUserMsg *msg)
 	/* Interpret ring addresses only when ring is started. */
 	dev = translate_ring_addresses(dev, file.index);
 	if (!dev)
-		return;
+		return VH_RESULT_OK;
 
 	*pdev = dev;
 
@@ -1058,6 +1082,8 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, VhostUserMsg *msg)
 	if (vq->kickfd >= 0)
 		close(vq->kickfd);
 	vq->kickfd = file.fd;
+
+	return VH_RESULT_OK;
 }
 
 static void
@@ -1080,9 +1106,10 @@ free_zmbufs(struct vhost_virtqueue *vq)
  * when virtio is stopped, qemu will send us the GET_VRING_BASE message.
  */
 static int
-vhost_user_get_vring_base(struct virtio_net *dev,
+vhost_user_get_vring_base(struct virtio_net **pdev,
 			  VhostUserMsg *msg)
 {
+	struct virtio_net *dev = *pdev;
 	struct vhost_virtqueue *vq = dev->virtqueue[msg->payload.state.index];
 
 	/* We have to stop the queue (virtio) if it is running. */
@@ -1127,7 +1154,7 @@ vhost_user_get_vring_base(struct virtio_net *dev,
 
 	msg->size = sizeof(msg->payload.state);
 
-	return 0;
+	return VH_RESULT_OK;
 }
 
 /*
@@ -1135,9 +1162,10 @@ vhost_user_get_vring_base(struct virtio_net *dev,
  * enable the virtio queue pair.
  */
 static int
-vhost_user_set_vring_enable(struct virtio_net *dev,
+vhost_user_set_vring_enable(struct virtio_net **pdev,
 			    VhostUserMsg *msg)
 {
+	struct virtio_net *dev = *pdev;
 	int enable = (int)msg->payload.state.num;
 	int index = (int)msg->payload.state.index;
 	struct rte_vdpa_device *vdpa_dev;
@@ -1158,13 +1186,14 @@ vhost_user_set_vring_enable(struct virtio_net *dev,
 
 	dev->virtqueue[index]->enabled = enable;
 
-	return 0;
+	return VH_RESULT_OK;
 }
 
-static void
-vhost_user_get_protocol_features(struct virtio_net *dev,
+static int
+vhost_user_get_protocol_features(struct virtio_net **pdev,
 				 VhostUserMsg *msg)
 {
+	struct virtio_net *dev = *pdev;
 	uint64_t features, protocol_features;
 
 	rte_vhost_driver_get_features(dev->ifname, &features);
@@ -1181,35 +1210,42 @@ vhost_user_get_protocol_features(struct virtio_net *dev,
 
 	msg->payload.u64 = protocol_features;
 	msg->size = sizeof(msg->payload.u64);
+
+	return VH_RESULT_OK;
 }
 
-static void
-vhost_user_set_protocol_features(struct virtio_net *dev,
-				 uint64_t protocol_features)
+static int
+vhost_user_set_protocol_features(struct virtio_net **pdev,
+				 VhostUserMsg *msg)
 {
+	struct virtio_net *dev = *pdev;
+	uint64_t protocol_features = msg->payload.u64;
 	if (protocol_features & ~VHOST_USER_PROTOCOL_FEATURES)
-		return;
+		return VH_RESULT_OK;
 
 	dev->protocol_features = protocol_features;
+
+	return VH_RESULT_OK;
 }
 
 static int
-vhost_user_set_log_base(struct virtio_net *dev, VhostUserMsg *msg)
+vhost_user_set_log_base(struct virtio_net **pdev, VhostUserMsg *msg)
 {
+	struct virtio_net *dev = *pdev;
 	int fd = msg->fds[0];
 	uint64_t size, off;
 	void *addr;
 
 	if (fd < 0) {
 		RTE_LOG(ERR, VHOST_CONFIG, "invalid log fd: %d\n", fd);
-		return -1;
+		return VH_RESULT_ERR;
 	}
 
 	if (msg->size != sizeof(VhostUserLog)) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"invalid log base msg size: %"PRId32" != %d\n",
 			msg->size, (int)sizeof(VhostUserLog));
-		return -1;
+		return VH_RESULT_ERR;
 	}
 
 	size = msg->payload.log.mmap_size;
@@ -1220,7 +1256,7 @@ vhost_user_set_log_base(struct virtio_net *dev, VhostUserMsg *msg)
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"log offset %#"PRIx64" exceeds log size %#"PRIx64"\n",
 			off, size);
-		return -1;
+		return VH_RESULT_ERR;
 	}
 
 	RTE_LOG(INFO, VHOST_CONFIG,
@@ -1235,7 +1271,7 @@ vhost_user_set_log_base(struct virtio_net *dev, VhostUserMsg *msg)
 	close(fd);
 	if (addr == MAP_FAILED) {
 		RTE_LOG(ERR, VHOST_CONFIG, "mmap log base failed!\n");
-		return -1;
+		return VH_RESULT_ERR;
 	}
 
 	/*
@@ -1251,14 +1287,16 @@ vhost_user_set_log_base(struct virtio_net *dev, VhostUserMsg *msg)
 
 	msg->size = sizeof(msg->payload.u64);
 
-	return 0;
+	return VH_RESULT_OK;
 }
 
-static void
-vhost_user_set_log_fd(struct virtio_net *dev __rte_unused, VhostUserMsg *msg)
+static int
+vhost_user_set_log_fd(struct virtio_net **pdev __rte_unused, VhostUserMsg *msg)
 {
 	close(msg->fds[0]);
 	RTE_LOG(INFO, VHOST_CONFIG, "not implemented.\n");
+
+	return VH_RESULT_OK;
 }
 
 /*
@@ -1270,8 +1308,9 @@ vhost_user_set_log_fd(struct virtio_net *dev __rte_unused, VhostUserMsg *msg)
  * a flag 'broadcast_rarp' to let rte_vhost_dequeue_burst() inject it.
  */
 static int
-vhost_user_send_rarp(struct virtio_net *dev, VhostUserMsg *msg)
+vhost_user_send_rarp(struct virtio_net **pdev, VhostUserMsg *msg)
 {
+	struct virtio_net *dev = *pdev;
 	uint8_t *mac = (uint8_t *)&msg->payload.u64;
 	struct rte_vdpa_device *vdpa_dev;
 	int did = -1;
@@ -1295,40 +1334,42 @@ vhost_user_send_rarp(struct virtio_net *dev, VhostUserMsg *msg)
 	if (vdpa_dev && vdpa_dev->ops->migration_done)
 		vdpa_dev->ops->migration_done(dev->vid);
 
-	return 0;
+	return VH_RESULT_OK;
 }
 
 static int
-vhost_user_net_set_mtu(struct virtio_net *dev, VhostUserMsg *msg)
+vhost_user_net_set_mtu(struct virtio_net **pdev, VhostUserMsg *msg)
 {
+	struct virtio_net *dev = *pdev;
 	if (msg->payload.u64 < VIRTIO_MIN_MTU ||
 			msg->payload.u64 > VIRTIO_MAX_MTU) {
 		RTE_LOG(ERR, VHOST_CONFIG, "Invalid MTU size (%"PRIu64")\n",
 				msg->payload.u64);
 
-		return -1;
+		return VH_RESULT_ERR;
 	}
 
 	dev->mtu = msg->payload.u64;
 
-	return 0;
+	return VH_RESULT_OK;
 }
 
 static int
-vhost_user_set_req_fd(struct virtio_net *dev, VhostUserMsg *msg)
+vhost_user_set_req_fd(struct virtio_net **pdev, VhostUserMsg *msg)
 {
+	struct virtio_net *dev = *pdev;
 	int fd = msg->fds[0];
 
 	if (fd < 0) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 				"Invalid file descriptor for slave channel (%d)\n",
 				fd);
-		return -1;
+		return VH_RESULT_ERR;
 	}
 
 	dev->slave_req_fd = fd;
 
-	return 0;
+	return VH_RESULT_OK;
 }
 
 static int
@@ -1393,7 +1434,7 @@ vhost_user_iotlb_msg(struct virtio_net **pdev, VhostUserMsg *msg)
 		len = imsg->size;
 		vva = qva_to_vva(dev, imsg->uaddr, &len);
 		if (!vva)
-			return -1;
+			return VH_RESULT_ERR;
 
 		for (i = 0; i < dev->nr_vring; i++) {
 			struct vhost_virtqueue *vq = dev->virtqueue[i];
@@ -1419,10 +1460,10 @@ vhost_user_iotlb_msg(struct virtio_net **pdev, VhostUserMsg *msg)
 	default:
 		RTE_LOG(ERR, VHOST_CONFIG, "Invalid IOTLB message type (%d)\n",
 				imsg->type);
-		return -1;
+		return VH_RESULT_ERR;
 	}
 
-	return 0;
+	return VH_RESULT_OK;
 }
 
 /* return bytes# of read on success or negative val on failure. */
@@ -1671,28 +1712,28 @@ vhost_user_msg_handler(int vid, int fd)
 
 	switch (msg.request.master) {
 	case VHOST_USER_GET_FEATURES:
-		vhost_user_get_features(dev, &msg);
+		ret = vhost_user_get_features(&dev, &msg);
 		send_vhost_reply(fd, &msg);
 		break;
 	case VHOST_USER_SET_FEATURES:
-		ret = vhost_user_set_features(dev, msg.payload.u64);
+		ret = vhost_user_set_features(&dev, &msg);
 		if (ret)
 			return -1;
 		break;
 
 	case VHOST_USER_GET_PROTOCOL_FEATURES:
-		vhost_user_get_protocol_features(dev, &msg);
+		ret = vhost_user_get_protocol_features(&dev, &msg);
 		send_vhost_reply(fd, &msg);
 		break;
 	case VHOST_USER_SET_PROTOCOL_FEATURES:
-		vhost_user_set_protocol_features(dev, msg.payload.u64);
+		ret = vhost_user_set_protocol_features(&dev, &msg);
 		break;
 
 	case VHOST_USER_SET_OWNER:
-		vhost_user_set_owner();
+		ret = vhost_user_set_owner(&dev, &msg);
 		break;
 	case VHOST_USER_RESET_OWNER:
-		vhost_user_reset_owner(dev);
+		ret = vhost_user_reset_owner(&dev, &msg);
 		break;
 
 	case VHOST_USER_SET_MEM_TABLE:
@@ -1700,57 +1741,57 @@ vhost_user_msg_handler(int vid, int fd)
 		break;
 
 	case VHOST_USER_SET_LOG_BASE:
-		vhost_user_set_log_base(dev, &msg);
+		ret = vhost_user_set_log_base(&dev, &msg);
 		send_vhost_reply(fd, &msg);
 		break;
 	case VHOST_USER_SET_LOG_FD:
-		vhost_user_set_log_fd(dev, &msg);
+		ret = vhost_user_set_log_fd(&dev, &msg);
 		break;
 
 	case VHOST_USER_SET_VRING_NUM:
-		vhost_user_set_vring_num(dev, &msg);
+		ret = vhost_user_set_vring_num(&dev, &msg);
 		break;
 	case VHOST_USER_SET_VRING_ADDR:
-		vhost_user_set_vring_addr(&dev, &msg);
+		ret = vhost_user_set_vring_addr(&dev, &msg);
 		break;
 	case VHOST_USER_SET_VRING_BASE:
-		vhost_user_set_vring_base(dev, &msg);
+		ret = vhost_user_set_vring_base(&dev, &msg);
 		break;
 
 	case VHOST_USER_GET_VRING_BASE:
-		vhost_user_get_vring_base(dev, &msg);
+		ret = vhost_user_get_vring_base(&dev, &msg);
 		send_vhost_reply(fd, &msg);
 		break;
 
 	case VHOST_USER_SET_VRING_KICK:
-		vhost_user_set_vring_kick(&dev, &msg);
+		ret = vhost_user_set_vring_kick(&dev, &msg);
 		break;
 	case VHOST_USER_SET_VRING_CALL:
-		vhost_user_set_vring_call(dev, &msg);
+		ret = vhost_user_set_vring_call(&dev, &msg);
 		break;
 
 	case VHOST_USER_SET_VRING_ERR:
-		vhost_user_set_vring_err(dev, &msg);
+		ret = vhost_user_set_vring_err(&dev, &msg);
 		break;
 
 	case VHOST_USER_GET_QUEUE_NUM:
-		vhost_user_get_queue_num(dev, &msg);
+		ret = vhost_user_get_queue_num(&dev, &msg);
 		send_vhost_reply(fd, &msg);
 		break;
 
 	case VHOST_USER_SET_VRING_ENABLE:
-		vhost_user_set_vring_enable(dev, &msg);
+		ret = vhost_user_set_vring_enable(&dev, &msg);
 		break;
 	case VHOST_USER_SEND_RARP:
-		vhost_user_send_rarp(dev, &msg);
+		ret = vhost_user_send_rarp(&dev, &msg);
 		break;
 
 	case VHOST_USER_NET_SET_MTU:
-		ret = vhost_user_net_set_mtu(dev, &msg);
+		ret = vhost_user_net_set_mtu(&dev, &msg);
 		break;
 
 	case VHOST_USER_SET_SLAVE_REQ_FD:
-		ret = vhost_user_set_req_fd(dev, &msg);
+		ret = vhost_user_set_req_fd(&dev, &msg);
 		break;
 
 	case VHOST_USER_IOTLB_MSG:

Re: [dpdk-dev] [PATCH v2 4/5] vhost: unify message handling function signature

[Maxime Coquelin]

On 07/19/2018 09:13 PM, Nikolay Nikolaev wrote:
> Each vhost-user message handling function will return an int result
> which is described in the new enum vh_result: error, OK and reply.
> All functions will now have two arguments, virtio_net double pointer
> and VhostUserMsg pointer.
> 
> Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
> ---
>   lib/librte_vhost/vhost_user.c |  219 ++++++++++++++++++++++++-----------------
>   1 file changed, 130 insertions(+), 89 deletions(-)

Adding Ilya in cc, who has submitted a patch that seems to be
complementary with yours: http://patches.dpdk.org/patch/44168/

Other than that, it looks good to me:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime
> 
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index b408460f0..6b39d1e30 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -71,6 +71,16 @@ static const char *vhost_message_str[VHOST_USER_MAX] = {
>   	[VHOST_USER_CRYPTO_CLOSE_SESS] = "VHOST_USER_CRYPTO_CLOSE_SESS",
>   };
>   
> +/* The possible results of a message handling function */
> +enum vh_result {
> +	/* Message handling failed */
> +	VH_RESULT_ERR   = -1,
> +	/* Message handling successful */
> +	VH_RESULT_OK    =  0,
> +	/* Message handling successful and reply prepared */
> +	VH_RESULT_REPLY =  1,
> +};
> +
>   static uint64_t
>   get_blk_size(int fd)
>   {
> @@ -127,27 +137,31 @@ vhost_backend_cleanup(struct virtio_net *dev)
>    * the device hasn't been initialised.
>    */
>   static int
> -vhost_user_set_owner(void)
> +vhost_user_set_owner(struct virtio_net **pdev __rte_unused,
> +		VhostUserMsg * msg __rte_unused)
>   {
> -	return 0;
> +	return VH_RESULT_OK;
>   }
>   
>   static int
> -vhost_user_reset_owner(struct virtio_net *dev)
> +vhost_user_reset_owner(struct virtio_net **pdev,
> +		VhostUserMsg * msg __rte_unused)
>   {
> +	struct virtio_net *dev = *pdev;
>   	vhost_destroy_device_notify(dev);
>   
>   	cleanup_device(dev, 0);
>   	reset_device(dev);
> -	return 0;
> +	return VH_RESULT_OK;
>   }
>   
>   /*
>    * The features that we support are requested.
>    */
> -static uint64_t
> -vhost_user_get_features(struct virtio_net *dev, VhostUserMsg *msg)
> +static int
> +vhost_user_get_features(struct virtio_net **pdev, VhostUserMsg *msg)
>   {
> +	struct virtio_net *dev = *pdev;
>   	uint64_t features = 0;
>   
>   	rte_vhost_driver_get_features(dev->ifname, &features);
> @@ -155,15 +169,16 @@ vhost_user_get_features(struct virtio_net *dev, VhostUserMsg *msg)
>   	msg->payload.u64 = features;
>   	msg->size = sizeof(msg->payload.u64);
>   
> -	return features;
> +	return VH_RESULT_REPLY;
>   }
>   
>   /*
>    * The queue number that we support are requested.
>    */
> -static uint32_t
> -vhost_user_get_queue_num(struct virtio_net *dev, VhostUserMsg *msg)
> +static int
> +vhost_user_get_queue_num(struct virtio_net **pdev, VhostUserMsg *msg)
>   {
> +	struct virtio_net *dev = *pdev;
>   	uint32_t queue_num = 0;
>   
>   	rte_vhost_driver_get_queue_num(dev->ifname, &queue_num);
> @@ -171,15 +186,17 @@ vhost_user_get_queue_num(struct virtio_net *dev, VhostUserMsg *msg)
>   	msg->payload.u64 = (uint64_t)queue_num;
>   	msg->size = sizeof(msg->payload.u64);
>   
> -	return queue_num;
> +	return VH_RESULT_REPLY;
>   }
>   
>   /*
>    * We receive the negotiated features supported by us and the virtio device.
>    */
>   static int
> -vhost_user_set_features(struct virtio_net *dev, uint64_t features)
> +vhost_user_set_features(struct virtio_net **pdev, VhostUserMsg *msg)
>   {
> +	struct virtio_net *dev = *pdev;
> +	uint64_t features = msg->payload.u64;
>   	uint64_t vhost_features = 0;
>   	struct rte_vdpa_device *vdpa_dev;
>   	int did = -1;
> @@ -189,12 +206,12 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features)
>   		RTE_LOG(ERR, VHOST_CONFIG,
>   			"(%d) received invalid negotiated features.\n",
>   			dev->vid);
> -		return -1;
> +		return VH_RESULT_ERR;
>   	}
>   
>   	if (dev->flags & VIRTIO_DEV_RUNNING) {
>   		if (dev->features == features)
> -			return 0;
> +			return VH_RESULT_OK;
>   
>   		/*
>   		 * Error out if master tries to change features while device is
> @@ -205,7 +222,7 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features)
>   			RTE_LOG(ERR, VHOST_CONFIG,
>   				"(%d) features changed while device is running.\n",
>   				dev->vid);
> -			return -1;
> +			return VH_RESULT_ERR;
>   		}
>   
>   		if (dev->notify_ops->features_changed)
> @@ -250,16 +267,17 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features)
>   	if (vdpa_dev && vdpa_dev->ops->set_features)
>   		vdpa_dev->ops->set_features(dev->vid);
>   
> -	return 0;
> +	return VH_RESULT_OK;
>   }
>   
>   /*
>    * The virtio device sends us the size of the descriptor ring.
>    */
>   static int
> -vhost_user_set_vring_num(struct virtio_net *dev,
> +vhost_user_set_vring_num(struct virtio_net **pdev,
>   			 VhostUserMsg *msg)
>   {
> +	struct virtio_net *dev = *pdev;
>   	struct vhost_virtqueue *vq = dev->virtqueue[msg->payload.state.index];
>   
>   	vq->size = msg->payload.state.num;
> @@ -272,7 +290,7 @@ vhost_user_set_vring_num(struct virtio_net *dev,
>   	if ((vq->size & (vq->size - 1)) || vq->size > 32768) {
>   		RTE_LOG(ERR, VHOST_CONFIG,
>   			"invalid virtqueue size %u\n", vq->size);
> -		return -1;
> +		return VH_RESULT_ERR;
>   	}
>   
>   	if (dev->dequeue_zero_copy) {
> @@ -298,7 +316,7 @@ vhost_user_set_vring_num(struct virtio_net *dev,
>   		if (!vq->shadow_used_packed) {
>   			RTE_LOG(ERR, VHOST_CONFIG,
>   					"failed to allocate memory for shadow used ring.\n");
> -			return -1;
> +			return VH_RESULT_ERR;
>   		}
>   
>   	} else {
> @@ -308,7 +326,7 @@ vhost_user_set_vring_num(struct virtio_net *dev,
>   		if (!vq->shadow_used_split) {
>   			RTE_LOG(ERR, VHOST_CONFIG,
>   					"failed to allocate memory for shadow used ring.\n");
> -			return -1;
> +			return VH_RESULT_ERR;
>   		}
>   	}
>   
> @@ -318,10 +336,10 @@ vhost_user_set_vring_num(struct virtio_net *dev,
>   	if (!vq->batch_copy_elems) {
>   		RTE_LOG(ERR, VHOST_CONFIG,
>   			"failed to allocate memory for batching copy.\n");
> -		return -1;
> +		return VH_RESULT_ERR;
>   	}
>   
> -	return 0;
> +	return VH_RESULT_OK;
>   }
>   
>   /*
> @@ -619,12 +637,12 @@ translate_ring_addresses(struct virtio_net *dev, int vq_index)
>   static int
>   vhost_user_set_vring_addr(struct virtio_net **pdev, VhostUserMsg *msg)
>   {
> +	struct virtio_net *dev = *pdev;
>   	struct vhost_virtqueue *vq;
>   	struct vhost_vring_addr *addr = &msg->payload.addr;
> -	struct virtio_net *dev = *pdev;
>   
>   	if (dev->mem == NULL)
> -		return -1;
> +		return VH_RESULT_ERR;
>   
>   	/* addr->index refers to the queue index. The txq 1, rxq is 0. */
>   	vq = dev->virtqueue[msg->payload.addr.index];
> @@ -641,27 +659,28 @@ vhost_user_set_vring_addr(struct virtio_net **pdev, VhostUserMsg *msg)
>   				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) {
>   		dev = translate_ring_addresses(dev, msg->payload.addr.index);
>   		if (!dev)
> -			return -1;
> +			return VH_RESULT_ERR;
>   
>   		*pdev = dev;
>   	}
>   
> -	return 0;
> +	return VH_RESULT_OK;
>   }
>   
>   /*
>    * The virtio device sends us the available ring last used index.
>    */
>   static int
> -vhost_user_set_vring_base(struct virtio_net *dev,
> +vhost_user_set_vring_base(struct virtio_net **pdev,
>   			  VhostUserMsg *msg)
>   {
> +	struct virtio_net *dev = *pdev;
>   	dev->virtqueue[msg->payload.state.index]->last_used_idx  =
>   			msg->payload.state.num;
>   	dev->virtqueue[msg->payload.state.index]->last_avail_idx =
>   			msg->payload.state.num;
>   
> -	return 0;
> +	return VH_RESULT_OK;
>   }
>   
>   static int
> @@ -802,7 +821,7 @@ vhost_user_set_mem_table(struct virtio_net **pdev, VhostUserMsg *msg)
>   	if (memory.nregions > VHOST_MEMORY_MAX_NREGIONS) {
>   		RTE_LOG(ERR, VHOST_CONFIG,
>   			"too many memory regions (%u)\n", memory.nregions);
> -		return -1;
> +		return VH_RESULT_ERR;
>   	}
>   
>   	if (dev->mem && !vhost_memory_changed(&memory, dev->mem)) {
> @@ -812,7 +831,7 @@ vhost_user_set_mem_table(struct virtio_net **pdev, VhostUserMsg *msg)
>   		for (i = 0; i < memory.nregions; i++)
>   			close(msg->fds[i]);
>   
> -		return 0;
> +		return VH_RESULT_OK;
>   	}
>   
>   	if (dev->mem) {
> @@ -831,7 +850,7 @@ vhost_user_set_mem_table(struct virtio_net **pdev, VhostUserMsg *msg)
>   				"(%d) failed to allocate memory "
>   				"for dev->guest_pages\n",
>   				dev->vid);
> -			return -1;
> +			return VH_RESULT_ERR;
>   		}
>   	}
>   
> @@ -841,7 +860,7 @@ vhost_user_set_mem_table(struct virtio_net **pdev, VhostUserMsg *msg)
>   		RTE_LOG(ERR, VHOST_CONFIG,
>   			"(%d) failed to allocate memory for dev->mem\n",
>   			dev->vid);
> -		return -1;
> +		return VH_RESULT_ERR;
>   	}
>   	dev->mem->nregions = memory.nregions;
>   
> @@ -938,7 +957,7 @@ vhost_user_set_mem_table(struct virtio_net **pdev, VhostUserMsg *msg)
>   
>   			dev = translate_ring_addresses(dev, i);
>   			if (!dev)
> -				return -1;
> +				return VH_RESULT_ERR;
>   
>   			*pdev = dev;
>   		}
> @@ -946,13 +965,13 @@ vhost_user_set_mem_table(struct virtio_net **pdev, VhostUserMsg *msg)
>   
>   	dump_guest_pages(dev);
>   
> -	return 0;
> +	return VH_RESULT_OK;
>   
>   err_mmap:
>   	free_mem_region(dev);
>   	rte_free(dev->mem);
>   	dev->mem = NULL;
> -	return -1;
> +	return VH_RESULT_ERR;
>   }
>   
>   static bool
> @@ -994,9 +1013,10 @@ virtio_is_ready(struct virtio_net *dev)
>   	return 1;
>   }
>   
> -static void
> -vhost_user_set_vring_call(struct virtio_net *dev, VhostUserMsg *msg)
> +static int
> +vhost_user_set_vring_call(struct virtio_net **pdev, VhostUserMsg *msg)
>   {
> +	struct virtio_net *dev = *pdev;
>   	struct vhost_vring_file file;
>   	struct vhost_virtqueue *vq;
>   
> @@ -1013,22 +1033,26 @@ vhost_user_set_vring_call(struct virtio_net *dev, VhostUserMsg *msg)
>   		close(vq->callfd);
>   
>   	vq->callfd = file.fd;
> +
> +	return VH_RESULT_OK;
>   }
>   
> -static void vhost_user_set_vring_err(struct virtio_net *dev __rte_unused,
> +static int vhost_user_set_vring_err(struct virtio_net **pdev __rte_unused,
>   			VhostUserMsg *msg)
>   {
>   	if (!(msg->payload.u64 & VHOST_USER_VRING_NOFD_MASK))
>   		close(msg->fds[0]);
>   	RTE_LOG(INFO, VHOST_CONFIG, "not implemented\n");
> +
> +	return VH_RESULT_OK;
>   }
>   
> -static void
> +static int
>   vhost_user_set_vring_kick(struct virtio_net **pdev, VhostUserMsg *msg)
>   {
> +	struct virtio_net *dev = *pdev;
>   	struct vhost_vring_file file;
>   	struct vhost_virtqueue *vq;
> -	struct virtio_net *dev = *pdev;
>   
>   	file.index = msg->payload.u64 & VHOST_USER_VRING_IDX_MASK;
>   	if (msg->payload.u64 & VHOST_USER_VRING_NOFD_MASK)
> @@ -1041,7 +1065,7 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, VhostUserMsg *msg)
>   	/* Interpret ring addresses only when ring is started. */
>   	dev = translate_ring_addresses(dev, file.index);
>   	if (!dev)
> -		return;
> +		return VH_RESULT_OK;
>   
>   	*pdev = dev;
>   
> @@ -1058,6 +1082,8 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, VhostUserMsg *msg)
>   	if (vq->kickfd >= 0)
>   		close(vq->kickfd);
>   	vq->kickfd = file.fd;
> +
> +	return VH_RESULT_OK;
>   }
>   
>   static void
> @@ -1080,9 +1106,10 @@ free_zmbufs(struct vhost_virtqueue *vq)
>    * when virtio is stopped, qemu will send us the GET_VRING_BASE message.
>    */
>   static int
> -vhost_user_get_vring_base(struct virtio_net *dev,
> +vhost_user_get_vring_base(struct virtio_net **pdev,
>   			  VhostUserMsg *msg)
>   {
> +	struct virtio_net *dev = *pdev;
>   	struct vhost_virtqueue *vq = dev->virtqueue[msg->payload.state.index];
>   
>   	/* We have to stop the queue (virtio) if it is running. */
> @@ -1127,7 +1154,7 @@ vhost_user_get_vring_base(struct virtio_net *dev,
>   
>   	msg->size = sizeof(msg->payload.state);
>   
> -	return 0;
> +	return VH_RESULT_OK;
>   }
>   
>   /*
> @@ -1135,9 +1162,10 @@ vhost_user_get_vring_base(struct virtio_net *dev,
>    * enable the virtio queue pair.
>    */
>   static int
> -vhost_user_set_vring_enable(struct virtio_net *dev,
> +vhost_user_set_vring_enable(struct virtio_net **pdev,
>   			    VhostUserMsg *msg)
>   {
> +	struct virtio_net *dev = *pdev;
>   	int enable = (int)msg->payload.state.num;
>   	int index = (int)msg->payload.state.index;
>   	struct rte_vdpa_device *vdpa_dev;
> @@ -1158,13 +1186,14 @@ vhost_user_set_vring_enable(struct virtio_net *dev,
>   
>   	dev->virtqueue[index]->enabled = enable;
>   
> -	return 0;
> +	return VH_RESULT_OK;
>   }
>   
> -static void
> -vhost_user_get_protocol_features(struct virtio_net *dev,
> +static int
> +vhost_user_get_protocol_features(struct virtio_net **pdev,
>   				 VhostUserMsg *msg)
>   {
> +	struct virtio_net *dev = *pdev;
>   	uint64_t features, protocol_features;
>   
>   	rte_vhost_driver_get_features(dev->ifname, &features);
> @@ -1181,35 +1210,42 @@ vhost_user_get_protocol_features(struct virtio_net *dev,
>   
>   	msg->payload.u64 = protocol_features;
>   	msg->size = sizeof(msg->payload.u64);
> +
> +	return VH_RESULT_OK;
>   }
>   
> -static void
> -vhost_user_set_protocol_features(struct virtio_net *dev,
> -				 uint64_t protocol_features)
> +static int
> +vhost_user_set_protocol_features(struct virtio_net **pdev,
> +				 VhostUserMsg *msg)
>   {
> +	struct virtio_net *dev = *pdev;
> +	uint64_t protocol_features = msg->payload.u64;
>   	if (protocol_features & ~VHOST_USER_PROTOCOL_FEATURES)
> -		return;
> +		return VH_RESULT_OK;
>   
>   	dev->protocol_features = protocol_features;
> +
> +	return VH_RESULT_OK;
>   }
>   
>   static int
> -vhost_user_set_log_base(struct virtio_net *dev, VhostUserMsg *msg)
> +vhost_user_set_log_base(struct virtio_net **pdev, VhostUserMsg *msg)
>   {
> +	struct virtio_net *dev = *pdev;
>   	int fd = msg->fds[0];
>   	uint64_t size, off;
>   	void *addr;
>   
>   	if (fd < 0) {
>   		RTE_LOG(ERR, VHOST_CONFIG, "invalid log fd: %d\n", fd);
> -		return -1;
> +		return VH_RESULT_ERR;
>   	}
>   
>   	if (msg->size != sizeof(VhostUserLog)) {
>   		RTE_LOG(ERR, VHOST_CONFIG,
>   			"invalid log base msg size: %"PRId32" != %d\n",
>   			msg->size, (int)sizeof(VhostUserLog));
> -		return -1;
> +		return VH_RESULT_ERR;
>   	}
>   
>   	size = msg->payload.log.mmap_size;
> @@ -1220,7 +1256,7 @@ vhost_user_set_log_base(struct virtio_net *dev, VhostUserMsg *msg)
>   		RTE_LOG(ERR, VHOST_CONFIG,
>   			"log offset %#"PRIx64" exceeds log size %#"PRIx64"\n",
>   			off, size);
> -		return -1;
> +		return VH_RESULT_ERR;
>   	}
>   
>   	RTE_LOG(INFO, VHOST_CONFIG,
> @@ -1235,7 +1271,7 @@ vhost_user_set_log_base(struct virtio_net *dev, VhostUserMsg *msg)
>   	close(fd);
>   	if (addr == MAP_FAILED) {
>   		RTE_LOG(ERR, VHOST_CONFIG, "mmap log base failed!\n");
> -		return -1;
> +		return VH_RESULT_ERR;
>   	}
>   
>   	/*
> @@ -1251,14 +1287,16 @@ vhost_user_set_log_base(struct virtio_net *dev, VhostUserMsg *msg)
>   
>   	msg->size = sizeof(msg->payload.u64);
>   
> -	return 0;
> +	return VH_RESULT_OK;
>   }
>   
> -static void
> -vhost_user_set_log_fd(struct virtio_net *dev __rte_unused, VhostUserMsg *msg)
> +static int
> +vhost_user_set_log_fd(struct virtio_net **pdev __rte_unused, VhostUserMsg *msg)
>   {
>   	close(msg->fds[0]);
>   	RTE_LOG(INFO, VHOST_CONFIG, "not implemented.\n");
> +
> +	return VH_RESULT_OK;
>   }
>   
>   /*
> @@ -1270,8 +1308,9 @@ vhost_user_set_log_fd(struct virtio_net *dev __rte_unused, VhostUserMsg *msg)
>    * a flag 'broadcast_rarp' to let rte_vhost_dequeue_burst() inject it.
>    */
>   static int
> -vhost_user_send_rarp(struct virtio_net *dev, VhostUserMsg *msg)
> +vhost_user_send_rarp(struct virtio_net **pdev, VhostUserMsg *msg)
>   {
> +	struct virtio_net *dev = *pdev;
>   	uint8_t *mac = (uint8_t *)&msg->payload.u64;
>   	struct rte_vdpa_device *vdpa_dev;
>   	int did = -1;
> @@ -1295,40 +1334,42 @@ vhost_user_send_rarp(struct virtio_net *dev, VhostUserMsg *msg)
>   	if (vdpa_dev && vdpa_dev->ops->migration_done)
>   		vdpa_dev->ops->migration_done(dev->vid);
>   
> -	return 0;
> +	return VH_RESULT_OK;
>   }
>   
>   static int
> -vhost_user_net_set_mtu(struct virtio_net *dev, VhostUserMsg *msg)
> +vhost_user_net_set_mtu(struct virtio_net **pdev, VhostUserMsg *msg)
>   {
> +	struct virtio_net *dev = *pdev;
>   	if (msg->payload.u64 < VIRTIO_MIN_MTU ||
>   			msg->payload.u64 > VIRTIO_MAX_MTU) {
>   		RTE_LOG(ERR, VHOST_CONFIG, "Invalid MTU size (%"PRIu64")\n",
>   				msg->payload.u64);
>   
> -		return -1;
> +		return VH_RESULT_ERR;
>   	}
>   
>   	dev->mtu = msg->payload.u64;
>   
> -	return 0;
> +	return VH_RESULT_OK;
>   }
>   
>   static int
> -vhost_user_set_req_fd(struct virtio_net *dev, VhostUserMsg *msg)
> +vhost_user_set_req_fd(struct virtio_net **pdev, VhostUserMsg *msg)
>   {
> +	struct virtio_net *dev = *pdev;
>   	int fd = msg->fds[0];
>   
>   	if (fd < 0) {
>   		RTE_LOG(ERR, VHOST_CONFIG,
>   				"Invalid file descriptor for slave channel (%d)\n",
>   				fd);
> -		return -1;
> +		return VH_RESULT_ERR;
>   	}
>   
>   	dev->slave_req_fd = fd;
>   
> -	return 0;
> +	return VH_RESULT_OK;
>   }
>   
>   static int
> @@ -1393,7 +1434,7 @@ vhost_user_iotlb_msg(struct virtio_net **pdev, VhostUserMsg *msg)
>   		len = imsg->size;
>   		vva = qva_to_vva(dev, imsg->uaddr, &len);
>   		if (!vva)
> -			return -1;
> +			return VH_RESULT_ERR;
>   
>   		for (i = 0; i < dev->nr_vring; i++) {
>   			struct vhost_virtqueue *vq = dev->virtqueue[i];
> @@ -1419,10 +1460,10 @@ vhost_user_iotlb_msg(struct virtio_net **pdev, VhostUserMsg *msg)
>   	default:
>   		RTE_LOG(ERR, VHOST_CONFIG, "Invalid IOTLB message type (%d)\n",
>   				imsg->type);
> -		return -1;
> +		return VH_RESULT_ERR;
>   	}
>   
> -	return 0;
> +	return VH_RESULT_OK;
>   }
>   
>   /* return bytes# of read on success or negative val on failure. */
> @@ -1671,28 +1712,28 @@ vhost_user_msg_handler(int vid, int fd)
>   
>   	switch (msg.request.master) {
>   	case VHOST_USER_GET_FEATURES:
> -		vhost_user_get_features(dev, &msg);
> +		ret = vhost_user_get_features(&dev, &msg);
>   		send_vhost_reply(fd, &msg);
>   		break;
>   	case VHOST_USER_SET_FEATURES:
> -		ret = vhost_user_set_features(dev, msg.payload.u64);
> +		ret = vhost_user_set_features(&dev, &msg);
>   		if (ret)
>   			return -1;
>   		break;
>   
>   	case VHOST_USER_GET_PROTOCOL_FEATURES:
> -		vhost_user_get_protocol_features(dev, &msg);
> +		ret = vhost_user_get_protocol_features(&dev, &msg);
>   		send_vhost_reply(fd, &msg);
>   		break;
>   	case VHOST_USER_SET_PROTOCOL_FEATURES:
> -		vhost_user_set_protocol_features(dev, msg.payload.u64);
> +		ret = vhost_user_set_protocol_features(&dev, &msg);
>   		break;
>   
>   	case VHOST_USER_SET_OWNER:
> -		vhost_user_set_owner();
> +		ret = vhost_user_set_owner(&dev, &msg);
>   		break;
>   	case VHOST_USER_RESET_OWNER:
> -		vhost_user_reset_owner(dev);
> +		ret = vhost_user_reset_owner(&dev, &msg);
>   		break;
>   
>   	case VHOST_USER_SET_MEM_TABLE:
> @@ -1700,57 +1741,57 @@ vhost_user_msg_handler(int vid, int fd)
>   		break;
>   
>   	case VHOST_USER_SET_LOG_BASE:
> -		vhost_user_set_log_base(dev, &msg);
> +		ret = vhost_user_set_log_base(&dev, &msg);
>   		send_vhost_reply(fd, &msg);
>   		break;
>   	case VHOST_USER_SET_LOG_FD:
> -		vhost_user_set_log_fd(dev, &msg);
> +		ret = vhost_user_set_log_fd(&dev, &msg);
>   		break;
>   
>   	case VHOST_USER_SET_VRING_NUM:
> -		vhost_user_set_vring_num(dev, &msg);
> +		ret = vhost_user_set_vring_num(&dev, &msg);
>   		break;
>   	case VHOST_USER_SET_VRING_ADDR:
> -		vhost_user_set_vring_addr(&dev, &msg);
> +		ret = vhost_user_set_vring_addr(&dev, &msg);
>   		break;
>   	case VHOST_USER_SET_VRING_BASE:
> -		vhost_user_set_vring_base(dev, &msg);
> +		ret = vhost_user_set_vring_base(&dev, &msg);
>   		break;
>   
>   	case VHOST_USER_GET_VRING_BASE:
> -		vhost_user_get_vring_base(dev, &msg);
> +		ret = vhost_user_get_vring_base(&dev, &msg);
>   		send_vhost_reply(fd, &msg);
>   		break;
>   
>   	case VHOST_USER_SET_VRING_KICK:
> -		vhost_user_set_vring_kick(&dev, &msg);
> +		ret = vhost_user_set_vring_kick(&dev, &msg);
>   		break;
>   	case VHOST_USER_SET_VRING_CALL:
> -		vhost_user_set_vring_call(dev, &msg);
> +		ret = vhost_user_set_vring_call(&dev, &msg);
>   		break;
>   
>   	case VHOST_USER_SET_VRING_ERR:
> -		vhost_user_set_vring_err(dev, &msg);
> +		ret = vhost_user_set_vring_err(&dev, &msg);
>   		break;
>   
>   	case VHOST_USER_GET_QUEUE_NUM:
> -		vhost_user_get_queue_num(dev, &msg);
> +		ret = vhost_user_get_queue_num(&dev, &msg);
>   		send_vhost_reply(fd, &msg);
>   		break;
>   
>   	case VHOST_USER_SET_VRING_ENABLE:
> -		vhost_user_set_vring_enable(dev, &msg);
> +		ret = vhost_user_set_vring_enable(&dev, &msg);
>   		break;
>   	case VHOST_USER_SEND_RARP:
> -		vhost_user_send_rarp(dev, &msg);
> +		ret = vhost_user_send_rarp(&dev, &msg);
>   		break;
>   
>   	case VHOST_USER_NET_SET_MTU:
> -		ret = vhost_user_net_set_mtu(dev, &msg);
> +		ret = vhost_user_net_set_mtu(&dev, &msg);
>   		break;
>   
>   	case VHOST_USER_SET_SLAVE_REQ_FD:
> -		ret = vhost_user_set_req_fd(dev, &msg);
> +		ret = vhost_user_set_req_fd(&dev, &msg);
>   		break;
>   
>   	case VHOST_USER_IOTLB_MSG:
> 

[dpdk-dev] [PATCH v2 5/5] vhost: message handling implemented as a callback array

[Nikolay Nikolaev]

Introduce vhost_message_handlers, which maps the message request
type to the message handler. Then replace the switch construct
with a map and call.

Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
---
 lib/librte_vhost/vhost_user.c |  143 +++++++++++++++--------------------------
 1 file changed, 54 insertions(+), 89 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 6b39d1e30..1b164df27 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -1466,6 +1466,34 @@ vhost_user_iotlb_msg(struct virtio_net **pdev, VhostUserMsg *msg)
 	return VH_RESULT_OK;
 }
 
+typedef int (*vhost_message_handler_t)(struct virtio_net **pdev, VhostUserMsg * msg);
+static vhost_message_handler_t vhost_message_handlers[VHOST_USER_MAX] = {
+	[VHOST_USER_NONE] = NULL,
+	[VHOST_USER_GET_FEATURES] = vhost_user_get_features,
+	[VHOST_USER_SET_FEATURES] = vhost_user_set_features,
+	[VHOST_USER_SET_OWNER] = vhost_user_set_owner,
+	[VHOST_USER_RESET_OWNER] = vhost_user_reset_owner,
+	[VHOST_USER_SET_MEM_TABLE] = vhost_user_set_mem_table,
+	[VHOST_USER_SET_LOG_BASE] = vhost_user_set_log_base,
+	[VHOST_USER_SET_LOG_FD] = vhost_user_set_log_fd,
+	[VHOST_USER_SET_VRING_NUM] = vhost_user_set_vring_num,
+	[VHOST_USER_SET_VRING_ADDR] = vhost_user_set_vring_addr,
+	[VHOST_USER_SET_VRING_BASE] = vhost_user_set_vring_base,
+	[VHOST_USER_GET_VRING_BASE] = vhost_user_get_vring_base,
+	[VHOST_USER_SET_VRING_KICK] = vhost_user_set_vring_kick,
+	[VHOST_USER_SET_VRING_CALL] = vhost_user_set_vring_call,
+	[VHOST_USER_SET_VRING_ERR] = vhost_user_set_vring_err,
+	[VHOST_USER_GET_PROTOCOL_FEATURES] = vhost_user_get_protocol_features,
+	[VHOST_USER_SET_PROTOCOL_FEATURES] = vhost_user_set_protocol_features,
+	[VHOST_USER_GET_QUEUE_NUM] = vhost_user_get_queue_num,
+	[VHOST_USER_SET_VRING_ENABLE] = vhost_user_set_vring_enable,
+	[VHOST_USER_SEND_RARP] = vhost_user_send_rarp,
+	[VHOST_USER_NET_SET_MTU] = vhost_user_net_set_mtu,
+	[VHOST_USER_SET_SLAVE_REQ_FD] = vhost_user_set_req_fd,
+	[VHOST_USER_IOTLB_MSG] = vhost_user_iotlb_msg,
+};
+
+
 /* return bytes# of read on success or negative val on failure. */
 static int
 read_vhost_message(int sockfd, VhostUserMsg *msg)
@@ -1618,6 +1646,7 @@ vhost_user_msg_handler(int vid, int fd)
 	int ret;
 	int unlock_required = 0;
 	uint32_t skip_master = 0;
+	int request;
 
 	dev = get_device(vid);
 	if (dev == NULL)
@@ -1710,97 +1739,33 @@ vhost_user_msg_handler(int vid, int fd)
 			goto skip_to_post_handle;
 	}
 
-	switch (msg.request.master) {
-	case VHOST_USER_GET_FEATURES:
-		ret = vhost_user_get_features(&dev, &msg);
-		send_vhost_reply(fd, &msg);
-		break;
-	case VHOST_USER_SET_FEATURES:
-		ret = vhost_user_set_features(&dev, &msg);
-		if (ret)
-			return -1;
-		break;
-
-	case VHOST_USER_GET_PROTOCOL_FEATURES:
-		ret = vhost_user_get_protocol_features(&dev, &msg);
-		send_vhost_reply(fd, &msg);
-		break;
-	case VHOST_USER_SET_PROTOCOL_FEATURES:
-		ret = vhost_user_set_protocol_features(&dev, &msg);
-		break;
-
-	case VHOST_USER_SET_OWNER:
-		ret = vhost_user_set_owner(&dev, &msg);
-		break;
-	case VHOST_USER_RESET_OWNER:
-		ret = vhost_user_reset_owner(&dev, &msg);
-		break;
-
-	case VHOST_USER_SET_MEM_TABLE:
-		ret = vhost_user_set_mem_table(&dev, &msg);
-		break;
-
-	case VHOST_USER_SET_LOG_BASE:
-		ret = vhost_user_set_log_base(&dev, &msg);
-		send_vhost_reply(fd, &msg);
-		break;
-	case VHOST_USER_SET_LOG_FD:
-		ret = vhost_user_set_log_fd(&dev, &msg);
-		break;
-
-	case VHOST_USER_SET_VRING_NUM:
-		ret = vhost_user_set_vring_num(&dev, &msg);
-		break;
-	case VHOST_USER_SET_VRING_ADDR:
-		ret = vhost_user_set_vring_addr(&dev, &msg);
-		break;
-	case VHOST_USER_SET_VRING_BASE:
-		ret = vhost_user_set_vring_base(&dev, &msg);
-		break;
-
-	case VHOST_USER_GET_VRING_BASE:
-		ret = vhost_user_get_vring_base(&dev, &msg);
-		send_vhost_reply(fd, &msg);
-		break;
-
-	case VHOST_USER_SET_VRING_KICK:
-		ret = vhost_user_set_vring_kick(&dev, &msg);
-		break;
-	case VHOST_USER_SET_VRING_CALL:
-		ret = vhost_user_set_vring_call(&dev, &msg);
-		break;
-
-	case VHOST_USER_SET_VRING_ERR:
-		ret = vhost_user_set_vring_err(&dev, &msg);
-		break;
-
-	case VHOST_USER_GET_QUEUE_NUM:
-		ret = vhost_user_get_queue_num(&dev, &msg);
-		send_vhost_reply(fd, &msg);
-		break;
-
-	case VHOST_USER_SET_VRING_ENABLE:
-		ret = vhost_user_set_vring_enable(&dev, &msg);
-		break;
-	case VHOST_USER_SEND_RARP:
-		ret = vhost_user_send_rarp(&dev, &msg);
-		break;
-
-	case VHOST_USER_NET_SET_MTU:
-		ret = vhost_user_net_set_mtu(&dev, &msg);
-		break;
-
-	case VHOST_USER_SET_SLAVE_REQ_FD:
-		ret = vhost_user_set_req_fd(&dev, &msg);
-		break;
-
-	case VHOST_USER_IOTLB_MSG:
-		ret = vhost_user_iotlb_msg(&dev, &msg);
-		break;
+	request = msg.request.master;
+	if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) {
+		if (!vhost_message_handlers[request])
+			goto skip_to_post_handle;
+		ret = vhost_message_handlers[request](&dev, &msg);
 
-	default:
+		switch (ret) {
+		case VH_RESULT_ERR:
+			RTE_LOG(ERR, VHOST_CONFIG,
+				"Processing %s failed.\n",
+				vhost_message_str[request]);
+		case VH_RESULT_OK:
+			RTE_LOG(DEBUG, VHOST_CONFIG,
+				"Processing %s succeeded.\n",
+				vhost_message_str[request]);
+			break;
+		case VH_RESULT_REPLY:
+			RTE_LOG(INFO, VHOST_CONFIG,
+				"Processing %s succeeded and needs reply.\n",
+				vhost_message_str[request]);
+			send_vhost_reply(fd, &msg);
+			break;
+		}
+	} else {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"Requested invalid message type %d.\n", request);
 		ret = -1;
-		break;
 	}
 
 skip_to_post_handle:

Re: [dpdk-dev] [PATCH v2 5/5] vhost: message handling implemented as a callback array

[Maxime Coquelin]

On 07/19/2018 09:13 PM, Nikolay Nikolaev wrote:
> Introduce vhost_message_handlers, which maps the message request
> type to the message handler. Then replace the switch construct
> with a map and call.
> 
> Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
> ---
>   lib/librte_vhost/vhost_user.c |  143 +++++++++++++++--------------------------
>   1 file changed, 54 insertions(+), 89 deletions(-)

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 6b39d1e30..1b164df27 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -1466,6 +1466,34 @@ vhost_user_iotlb_msg(struct virtio_net **pdev, VhostUserMsg *msg)
>   	return VH_RESULT_OK;
>   }
>   
> +typedef int (*vhost_message_handler_t)(struct virtio_net **pdev, VhostUserMsg * msg);
> +static vhost_message_handler_t vhost_message_handlers[VHOST_USER_MAX] = {
> +	[VHOST_USER_NONE] = NULL,
> +	[VHOST_USER_GET_FEATURES] = vhost_user_get_features,
> +	[VHOST_USER_SET_FEATURES] = vhost_user_set_features,
> +	[VHOST_USER_SET_OWNER] = vhost_user_set_owner,
> +	[VHOST_USER_RESET_OWNER] = vhost_user_reset_owner,
> +	[VHOST_USER_SET_MEM_TABLE] = vhost_user_set_mem_table,
> +	[VHOST_USER_SET_LOG_BASE] = vhost_user_set_log_base,
> +	[VHOST_USER_SET_LOG_FD] = vhost_user_set_log_fd,
> +	[VHOST_USER_SET_VRING_NUM] = vhost_user_set_vring_num,
> +	[VHOST_USER_SET_VRING_ADDR] = vhost_user_set_vring_addr,
> +	[VHOST_USER_SET_VRING_BASE] = vhost_user_set_vring_base,
> +	[VHOST_USER_GET_VRING_BASE] = vhost_user_get_vring_base,
> +	[VHOST_USER_SET_VRING_KICK] = vhost_user_set_vring_kick,
> +	[VHOST_USER_SET_VRING_CALL] = vhost_user_set_vring_call,
> +	[VHOST_USER_SET_VRING_ERR] = vhost_user_set_vring_err,
> +	[VHOST_USER_GET_PROTOCOL_FEATURES] = vhost_user_get_protocol_features,
> +	[VHOST_USER_SET_PROTOCOL_FEATURES] = vhost_user_set_protocol_features,
> +	[VHOST_USER_GET_QUEUE_NUM] = vhost_user_get_queue_num,
> +	[VHOST_USER_SET_VRING_ENABLE] = vhost_user_set_vring_enable,
> +	[VHOST_USER_SEND_RARP] = vhost_user_send_rarp,
> +	[VHOST_USER_NET_SET_MTU] = vhost_user_net_set_mtu,
> +	[VHOST_USER_SET_SLAVE_REQ_FD] = vhost_user_set_req_fd,
> +	[VHOST_USER_IOTLB_MSG] = vhost_user_iotlb_msg,
> +};
> +
> +
>   /* return bytes# of read on success or negative val on failure. */
>   static int
>   read_vhost_message(int sockfd, VhostUserMsg *msg)
> @@ -1618,6 +1646,7 @@ vhost_user_msg_handler(int vid, int fd)
>   	int ret;
>   	int unlock_required = 0;
>   	uint32_t skip_master = 0;
> +	int request;
>   
>   	dev = get_device(vid);
>   	if (dev == NULL)
> @@ -1710,97 +1739,33 @@ vhost_user_msg_handler(int vid, int fd)
>   			goto skip_to_post_handle;
>   	}
>   
> -	switch (msg.request.master) {
> -	case VHOST_USER_GET_FEATURES:
> -		ret = vhost_user_get_features(&dev, &msg);
> -		send_vhost_reply(fd, &msg);
> -		break;
> -	case VHOST_USER_SET_FEATURES:
> -		ret = vhost_user_set_features(&dev, &msg);
> -		if (ret)
> -			return -1;
> -		break;
> -
> -	case VHOST_USER_GET_PROTOCOL_FEATURES:
> -		ret = vhost_user_get_protocol_features(&dev, &msg);
> -		send_vhost_reply(fd, &msg);
> -		break;
> -	case VHOST_USER_SET_PROTOCOL_FEATURES:
> -		ret = vhost_user_set_protocol_features(&dev, &msg);
> -		break;
> -
> -	case VHOST_USER_SET_OWNER:
> -		ret = vhost_user_set_owner(&dev, &msg);
> -		break;
> -	case VHOST_USER_RESET_OWNER:
> -		ret = vhost_user_reset_owner(&dev, &msg);
> -		break;
> -
> -	case VHOST_USER_SET_MEM_TABLE:
> -		ret = vhost_user_set_mem_table(&dev, &msg);
> -		break;
> -
> -	case VHOST_USER_SET_LOG_BASE:
> -		ret = vhost_user_set_log_base(&dev, &msg);
> -		send_vhost_reply(fd, &msg);
> -		break;
> -	case VHOST_USER_SET_LOG_FD:
> -		ret = vhost_user_set_log_fd(&dev, &msg);
> -		break;
> -
> -	case VHOST_USER_SET_VRING_NUM:
> -		ret = vhost_user_set_vring_num(&dev, &msg);
> -		break;
> -	case VHOST_USER_SET_VRING_ADDR:
> -		ret = vhost_user_set_vring_addr(&dev, &msg);
> -		break;
> -	case VHOST_USER_SET_VRING_BASE:
> -		ret = vhost_user_set_vring_base(&dev, &msg);
> -		break;
> -
> -	case VHOST_USER_GET_VRING_BASE:
> -		ret = vhost_user_get_vring_base(&dev, &msg);
> -		send_vhost_reply(fd, &msg);
> -		break;
> -
> -	case VHOST_USER_SET_VRING_KICK:
> -		ret = vhost_user_set_vring_kick(&dev, &msg);
> -		break;
> -	case VHOST_USER_SET_VRING_CALL:
> -		ret = vhost_user_set_vring_call(&dev, &msg);
> -		break;
> -
> -	case VHOST_USER_SET_VRING_ERR:
> -		ret = vhost_user_set_vring_err(&dev, &msg);
> -		break;
> -
> -	case VHOST_USER_GET_QUEUE_NUM:
> -		ret = vhost_user_get_queue_num(&dev, &msg);
> -		send_vhost_reply(fd, &msg);
> -		break;
> -
> -	case VHOST_USER_SET_VRING_ENABLE:
> -		ret = vhost_user_set_vring_enable(&dev, &msg);
> -		break;
> -	case VHOST_USER_SEND_RARP:
> -		ret = vhost_user_send_rarp(&dev, &msg);
> -		break;
> -
> -	case VHOST_USER_NET_SET_MTU:
> -		ret = vhost_user_net_set_mtu(&dev, &msg);
> -		break;
> -
> -	case VHOST_USER_SET_SLAVE_REQ_FD:
> -		ret = vhost_user_set_req_fd(&dev, &msg);
> -		break;
> -
> -	case VHOST_USER_IOTLB_MSG:
> -		ret = vhost_user_iotlb_msg(&dev, &msg);
> -		break;
> +	request = msg.request.master;
> +	if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) {
> +		if (!vhost_message_handlers[request])
> +			goto skip_to_post_handle;
> +		ret = vhost_message_handlers[request](&dev, &msg);
>   
> -	default:
> +		switch (ret) {
> +		case VH_RESULT_ERR:
> +			RTE_LOG(ERR, VHOST_CONFIG,
> +				"Processing %s failed.\n",
> +				vhost_message_str[request]);
> +		case VH_RESULT_OK:
> +			RTE_LOG(DEBUG, VHOST_CONFIG,
> +				"Processing %s succeeded.\n",
> +				vhost_message_str[request]);
> +			break;
> +		case VH_RESULT_REPLY:
> +			RTE_LOG(INFO, VHOST_CONFIG,
> +				"Processing %s succeeded and needs reply.\n",
> +				vhost_message_str[request]);
> +			send_vhost_reply(fd, &msg);
> +			break;
> +		}
> +	} else {
> +		RTE_LOG(ERR, VHOST_CONFIG,
> +			"Requested invalid message type %d.\n", request);
>   		ret = -1;
> -		break;
>   	}
>   
>   skip_to_post_handle:
> 

Re: [dpdk-dev] [PATCH v2 5/5] vhost: message handling implemented as a callback array

[Ilya Maximets]

Hi Maxime,
Thanks for pointing to this patch set. I missed it somehow.

This patch could not replace mine [1], because it does not improve
the message handling from the error handling point of view. But
it completely changes the code, so we need to negotiate the order
in which they will be applied or combine them somehow.

So, what's the plan? What do you think?

Few comments inline.

[1] http://patchwork.dpdk.org/patch/44168/

Best regards, Ilya Maximets.

On 10.09.2018 18:32, Maxime Coquelin wrote:
> 
> 
> On 07/19/2018 09:13 PM, Nikolay Nikolaev wrote:
>> Introduce vhost_message_handlers, which maps the message request
>> type to the message handler. Then replace the switch construct
>> with a map and call.
>>
>> Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
>> ---
>>   lib/librte_vhost/vhost_user.c |  143 +++++++++++++++--------------------------
>>   1 file changed, 54 insertions(+), 89 deletions(-)
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>> index 6b39d1e30..1b164df27 100644
>> --- a/lib/librte_vhost/vhost_user.c
>> +++ b/lib/librte_vhost/vhost_user.c
>> @@ -1466,6 +1466,34 @@ vhost_user_iotlb_msg(struct virtio_net **pdev, VhostUserMsg *msg)
>>       return VH_RESULT_OK;
>>   }
>>   +typedef int (*vhost_message_handler_t)(struct virtio_net **pdev, VhostUserMsg * msg);
>> +static vhost_message_handler_t vhost_message_handlers[VHOST_USER_MAX] = {
>> +    [VHOST_USER_NONE] = NULL,
>> +    [VHOST_USER_GET_FEATURES] = vhost_user_get_features,
>> +    [VHOST_USER_SET_FEATURES] = vhost_user_set_features,
>> +    [VHOST_USER_SET_OWNER] = vhost_user_set_owner,
>> +    [VHOST_USER_RESET_OWNER] = vhost_user_reset_owner,
>> +    [VHOST_USER_SET_MEM_TABLE] = vhost_user_set_mem_table,
>> +    [VHOST_USER_SET_LOG_BASE] = vhost_user_set_log_base,
>> +    [VHOST_USER_SET_LOG_FD] = vhost_user_set_log_fd,
>> +    [VHOST_USER_SET_VRING_NUM] = vhost_user_set_vring_num,
>> +    [VHOST_USER_SET_VRING_ADDR] = vhost_user_set_vring_addr,
>> +    [VHOST_USER_SET_VRING_BASE] = vhost_user_set_vring_base,
>> +    [VHOST_USER_GET_VRING_BASE] = vhost_user_get_vring_base,
>> +    [VHOST_USER_SET_VRING_KICK] = vhost_user_set_vring_kick,
>> +    [VHOST_USER_SET_VRING_CALL] = vhost_user_set_vring_call,
>> +    [VHOST_USER_SET_VRING_ERR] = vhost_user_set_vring_err,
>> +    [VHOST_USER_GET_PROTOCOL_FEATURES] = vhost_user_get_protocol_features,
>> +    [VHOST_USER_SET_PROTOCOL_FEATURES] = vhost_user_set_protocol_features,
>> +    [VHOST_USER_GET_QUEUE_NUM] = vhost_user_get_queue_num,
>> +    [VHOST_USER_SET_VRING_ENABLE] = vhost_user_set_vring_enable,
>> +    [VHOST_USER_SEND_RARP] = vhost_user_send_rarp,
>> +    [VHOST_USER_NET_SET_MTU] = vhost_user_net_set_mtu,
>> +    [VHOST_USER_SET_SLAVE_REQ_FD] = vhost_user_set_req_fd,
>> +    [VHOST_USER_IOTLB_MSG] = vhost_user_iotlb_msg,
>> +};
>> +
>> +
>>   /* return bytes# of read on success or negative val on failure. */
>>   static int
>>   read_vhost_message(int sockfd, VhostUserMsg *msg)
>> @@ -1618,6 +1646,7 @@ vhost_user_msg_handler(int vid, int fd)
>>       int ret;
>>       int unlock_required = 0;
>>       uint32_t skip_master = 0;
>> +    int request;
>>         dev = get_device(vid);
>>       if (dev == NULL)
>> @@ -1710,97 +1739,33 @@ vhost_user_msg_handler(int vid, int fd)
>>               goto skip_to_post_handle;
>>       }
>>   -    switch (msg.request.master) {
>> -    case VHOST_USER_GET_FEATURES:
>> -        ret = vhost_user_get_features(&dev, &msg);
>> -        send_vhost_reply(fd, &msg);
>> -        break;
>> -    case VHOST_USER_SET_FEATURES:
>> -        ret = vhost_user_set_features(&dev, &msg);
>> -        if (ret)
>> -            return -1;

You can not just remove this. Disconnection was triggered here
because the error is unrecoverable.
See 59fe5e17d930 ("vhost: propagate set features handling error")
for details.

>> -        break;
>> -
>> -    case VHOST_USER_GET_PROTOCOL_FEATURES:
>> -        ret = vhost_user_get_protocol_features(&dev, &msg);
>> -        send_vhost_reply(fd, &msg);
>> -        break;
>> -    case VHOST_USER_SET_PROTOCOL_FEATURES:
>> -        ret = vhost_user_set_protocol_features(&dev, &msg);
>> -        break;
>> -
>> -    case VHOST_USER_SET_OWNER:
>> -        ret = vhost_user_set_owner(&dev, &msg);
>> -        break;
>> -    case VHOST_USER_RESET_OWNER:
>> -        ret = vhost_user_reset_owner(&dev, &msg);
>> -        break;
>> -
>> -    case VHOST_USER_SET_MEM_TABLE:
>> -        ret = vhost_user_set_mem_table(&dev, &msg);
>> -        break;
>> -
>> -    case VHOST_USER_SET_LOG_BASE:
>> -        ret = vhost_user_set_log_base(&dev, &msg);
>> -        send_vhost_reply(fd, &msg);
>> -        break;
>> -    case VHOST_USER_SET_LOG_FD:
>> -        ret = vhost_user_set_log_fd(&dev, &msg);
>> -        break;
>> -
>> -    case VHOST_USER_SET_VRING_NUM:
>> -        ret = vhost_user_set_vring_num(&dev, &msg);
>> -        break;
>> -    case VHOST_USER_SET_VRING_ADDR:
>> -        ret = vhost_user_set_vring_addr(&dev, &msg);
>> -        break;
>> -    case VHOST_USER_SET_VRING_BASE:
>> -        ret = vhost_user_set_vring_base(&dev, &msg);
>> -        break;
>> -
>> -    case VHOST_USER_GET_VRING_BASE:
>> -        ret = vhost_user_get_vring_base(&dev, &msg);
>> -        send_vhost_reply(fd, &msg);
>> -        break;
>> -
>> -    case VHOST_USER_SET_VRING_KICK:
>> -        ret = vhost_user_set_vring_kick(&dev, &msg);
>> -        break;
>> -    case VHOST_USER_SET_VRING_CALL:
>> -        ret = vhost_user_set_vring_call(&dev, &msg);
>> -        break;
>> -
>> -    case VHOST_USER_SET_VRING_ERR:
>> -        ret = vhost_user_set_vring_err(&dev, &msg);
>> -        break;
>> -
>> -    case VHOST_USER_GET_QUEUE_NUM:
>> -        ret = vhost_user_get_queue_num(&dev, &msg);
>> -        send_vhost_reply(fd, &msg);
>> -        break;
>> -
>> -    case VHOST_USER_SET_VRING_ENABLE:
>> -        ret = vhost_user_set_vring_enable(&dev, &msg);
>> -        break;
>> -    case VHOST_USER_SEND_RARP:
>> -        ret = vhost_user_send_rarp(&dev, &msg);
>> -        break;
>> -
>> -    case VHOST_USER_NET_SET_MTU:
>> -        ret = vhost_user_net_set_mtu(&dev, &msg);
>> -        break;
>> -
>> -    case VHOST_USER_SET_SLAVE_REQ_FD:
>> -        ret = vhost_user_set_req_fd(&dev, &msg);
>> -        break;
>> -
>> -    case VHOST_USER_IOTLB_MSG:
>> -        ret = vhost_user_iotlb_msg(&dev, &msg);
>> -        break;
>> +    request = msg.request.master;
>> +    if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) {
>> +        if (!vhost_message_handlers[request])
>> +            goto skip_to_post_handle;
>> +        ret = vhost_message_handlers[request](&dev, &msg);
>>   -    default:
>> +        switch (ret) {
>> +        case VH_RESULT_ERR:
>> +            RTE_LOG(ERR, VHOST_CONFIG,
>> +                "Processing %s failed.\n",
>> +                vhost_message_str[request]);

I guess 'break' is missing here. Isn't it?

>> +        case VH_RESULT_OK:
>> +            RTE_LOG(DEBUG, VHOST_CONFIG,
>> +                "Processing %s succeeded.\n",
>> +                vhost_message_str[request]);
>> +            break;
>> +        case VH_RESULT_REPLY:
>> +            RTE_LOG(INFO, VHOST_CONFIG,
>> +                "Processing %s succeeded and needs reply.\n",
>> +                vhost_message_str[request]);
>> +            send_vhost_reply(fd, &msg);
>> +            break;
>> +        }
>> +    } else {
>> +        RTE_LOG(ERR, VHOST_CONFIG,
>> +            "Requested invalid message type %d.\n", request);
>>           ret = -1;
>> -        break;
>>       }
>>     skip_to_post_handle:
>>
> 
> 

Re: [dpdk-dev] [PATCH v2 5/5] vhost: message handling implemented as a callback array

[Maxime Coquelin]

Hi Ilya,

On 09/10/2018 06:09 PM, Ilya Maximets wrote:
> Hi Maxime,
> Thanks for pointing to this patch set. I missed it somehow.
> 
> This patch could not replace mine [1], because it does not improve
> the message handling from the error handling point of view. But
> it completely changes the code, so we need to negotiate the order
> in which they will be applied or combine them somehow.

By complementary I meant both of your patches add similar things, like
vhost_user_set_vring_kick() becoming an int (even if Nikolay's version
doesn't return an error for now).

> 
> So, what's the plan? What do you think?

On one hand Nikolay's series was posted first, and it might be easier to
just apply your patch on top. On the other hand, your patch fixes an
issue, so it will be easier to backport it if it is applied first.

I guess we should go for your patch first to ease backporting.

I'd like Tiwei's feedback before applying anything, as he reviewed the
first versions of both yours and Nikolay's patches.

> Few comments inline.
> 
> [1] http://patchwork.dpdk.org/patch/44168/
> 
> Best regards, Ilya Maximets.
> 
> On 10.09.2018 18:32, Maxime Coquelin wrote:
>>
>>
>> On 07/19/2018 09:13 PM, Nikolay Nikolaev wrote:
>>> Introduce vhost_message_handlers, which maps the message request
>>> type to the message handler. Then replace the switch construct
>>> with a map and call.
>>>
>>> Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
>>> ---
>>>    lib/librte_vhost/vhost_user.c |  143 +++++++++++++++--------------------------
>>>    1 file changed, 54 insertions(+), 89 deletions(-)
>>
>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>
>>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>>> index 6b39d1e30..1b164df27 100644
>>> --- a/lib/librte_vhost/vhost_user.c
>>> +++ b/lib/librte_vhost/vhost_user.c
>>> @@ -1466,6 +1466,34 @@ vhost_user_iotlb_msg(struct virtio_net **pdev, VhostUserMsg *msg)
>>>        return VH_RESULT_OK;
>>>    }
>>>    +typedef int (*vhost_message_handler_t)(struct virtio_net **pdev, VhostUserMsg * msg);
>>> +static vhost_message_handler_t vhost_message_handlers[VHOST_USER_MAX] = {
>>> +    [VHOST_USER_NONE] = NULL,
>>> +    [VHOST_USER_GET_FEATURES] = vhost_user_get_features,
>>> +    [VHOST_USER_SET_FEATURES] = vhost_user_set_features,
>>> +    [VHOST_USER_SET_OWNER] = vhost_user_set_owner,
>>> +    [VHOST_USER_RESET_OWNER] = vhost_user_reset_owner,
>>> +    [VHOST_USER_SET_MEM_TABLE] = vhost_user_set_mem_table,
>>> +    [VHOST_USER_SET_LOG_BASE] = vhost_user_set_log_base,
>>> +    [VHOST_USER_SET_LOG_FD] = vhost_user_set_log_fd,
>>> +    [VHOST_USER_SET_VRING_NUM] = vhost_user_set_vring_num,
>>> +    [VHOST_USER_SET_VRING_ADDR] = vhost_user_set_vring_addr,
>>> +    [VHOST_USER_SET_VRING_BASE] = vhost_user_set_vring_base,
>>> +    [VHOST_USER_GET_VRING_BASE] = vhost_user_get_vring_base,
>>> +    [VHOST_USER_SET_VRING_KICK] = vhost_user_set_vring_kick,
>>> +    [VHOST_USER_SET_VRING_CALL] = vhost_user_set_vring_call,
>>> +    [VHOST_USER_SET_VRING_ERR] = vhost_user_set_vring_err,
>>> +    [VHOST_USER_GET_PROTOCOL_FEATURES] = vhost_user_get_protocol_features,
>>> +    [VHOST_USER_SET_PROTOCOL_FEATURES] = vhost_user_set_protocol_features,
>>> +    [VHOST_USER_GET_QUEUE_NUM] = vhost_user_get_queue_num,
>>> +    [VHOST_USER_SET_VRING_ENABLE] = vhost_user_set_vring_enable,
>>> +    [VHOST_USER_SEND_RARP] = vhost_user_send_rarp,
>>> +    [VHOST_USER_NET_SET_MTU] = vhost_user_net_set_mtu,
>>> +    [VHOST_USER_SET_SLAVE_REQ_FD] = vhost_user_set_req_fd,
>>> +    [VHOST_USER_IOTLB_MSG] = vhost_user_iotlb_msg,
>>> +};
>>> +
>>> +
>>>    /* return bytes# of read on success or negative val on failure. */
>>>    static int
>>>    read_vhost_message(int sockfd, VhostUserMsg *msg)
>>> @@ -1618,6 +1646,7 @@ vhost_user_msg_handler(int vid, int fd)
>>>        int ret;
>>>        int unlock_required = 0;
>>>        uint32_t skip_master = 0;
>>> +    int request;
>>>          dev = get_device(vid);
>>>        if (dev == NULL)
>>> @@ -1710,97 +1739,33 @@ vhost_user_msg_handler(int vid, int fd)
>>>                goto skip_to_post_handle;
>>>        }
>>>    -    switch (msg.request.master) {
>>> -    case VHOST_USER_GET_FEATURES:
>>> -        ret = vhost_user_get_features(&dev, &msg);
>>> -        send_vhost_reply(fd, &msg);
>>> -        break;
>>> -    case VHOST_USER_SET_FEATURES:
>>> -        ret = vhost_user_set_features(&dev, &msg);
>>> -        if (ret)
>>> -            return -1;
> 
> You can not just remove this. Disconnection was triggered here
> because the error is unrecoverable.
> See 59fe5e17d930 ("vhost: propagate set features handling error")
> for details.

Right, maybe adding another enum for unrecoverable errors would do the
trick.

>>> -        break;
>>> -
>>> -    case VHOST_USER_GET_PROTOCOL_FEATURES:
>>> -        ret = vhost_user_get_protocol_features(&dev, &msg);
>>> -        send_vhost_reply(fd, &msg);
>>> -        break;
>>> -    case VHOST_USER_SET_PROTOCOL_FEATURES:
>>> -        ret = vhost_user_set_protocol_features(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_SET_OWNER:
>>> -        ret = vhost_user_set_owner(&dev, &msg);
>>> -        break;
>>> -    case VHOST_USER_RESET_OWNER:
>>> -        ret = vhost_user_reset_owner(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_SET_MEM_TABLE:
>>> -        ret = vhost_user_set_mem_table(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_SET_LOG_BASE:
>>> -        ret = vhost_user_set_log_base(&dev, &msg);
>>> -        send_vhost_reply(fd, &msg);
>>> -        break;
>>> -    case VHOST_USER_SET_LOG_FD:
>>> -        ret = vhost_user_set_log_fd(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_SET_VRING_NUM:
>>> -        ret = vhost_user_set_vring_num(&dev, &msg);
>>> -        break;
>>> -    case VHOST_USER_SET_VRING_ADDR:
>>> -        ret = vhost_user_set_vring_addr(&dev, &msg);
>>> -        break;
>>> -    case VHOST_USER_SET_VRING_BASE:
>>> -        ret = vhost_user_set_vring_base(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_GET_VRING_BASE:
>>> -        ret = vhost_user_get_vring_base(&dev, &msg);
>>> -        send_vhost_reply(fd, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_SET_VRING_KICK:
>>> -        ret = vhost_user_set_vring_kick(&dev, &msg);
>>> -        break;
>>> -    case VHOST_USER_SET_VRING_CALL:
>>> -        ret = vhost_user_set_vring_call(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_SET_VRING_ERR:
>>> -        ret = vhost_user_set_vring_err(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_GET_QUEUE_NUM:
>>> -        ret = vhost_user_get_queue_num(&dev, &msg);
>>> -        send_vhost_reply(fd, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_SET_VRING_ENABLE:
>>> -        ret = vhost_user_set_vring_enable(&dev, &msg);
>>> -        break;
>>> -    case VHOST_USER_SEND_RARP:
>>> -        ret = vhost_user_send_rarp(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_NET_SET_MTU:
>>> -        ret = vhost_user_net_set_mtu(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_SET_SLAVE_REQ_FD:
>>> -        ret = vhost_user_set_req_fd(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_IOTLB_MSG:
>>> -        ret = vhost_user_iotlb_msg(&dev, &msg);
>>> -        break;
>>> +    request = msg.request.master;
>>> +    if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) {
>>> +        if (!vhost_message_handlers[request])
>>> +            goto skip_to_post_handle;
>>> +        ret = vhost_message_handlers[request](&dev, &msg);
>>>    -    default:
>>> +        switch (ret) {
>>> +        case VH_RESULT_ERR:
>>> +            RTE_LOG(ERR, VHOST_CONFIG,
>>> +                "Processing %s failed.\n",
>>> +                vhost_message_str[request]);
> 
> I guess 'break' is missing here. Isn't it?

Good catch.

> 
>>> +        case VH_RESULT_OK:
>>> +            RTE_LOG(DEBUG, VHOST_CONFIG,
>>> +                "Processing %s succeeded.\n",
>>> +                vhost_message_str[request]);
>>> +            break;
>>> +        case VH_RESULT_REPLY:
>>> +            RTE_LOG(INFO, VHOST_CONFIG,
>>> +                "Processing %s succeeded and needs reply.\n",
>>> +                vhost_message_str[request]);
>>> +            send_vhost_reply(fd, &msg);
>>> +            break;
>>> +        }
>>> +    } else {
>>> +        RTE_LOG(ERR, VHOST_CONFIG,
>>> +            "Requested invalid message type %d.\n", request);
>>>            ret = -1;
>>> -        break;
>>>        }
>>>      skip_to_post_handle:
>>>
>>
>>

Re: [dpdk-dev] [PATCH v2 5/5] vhost: message handling implemented as a callback array

[Tiwei Bie]

On Mon, Sep 10, 2018 at 06:43:17PM +0200, Maxime Coquelin wrote:
> Hi Ilya,
> 
> On 09/10/2018 06:09 PM, Ilya Maximets wrote:
> > Hi Maxime,
> > Thanks for pointing to this patch set. I missed it somehow.
> > 
> > This patch could not replace mine [1], because it does not improve
> > the message handling from the error handling point of view. But
> > it completely changes the code, so we need to negotiate the order
> > in which they will be applied or combine them somehow.
> 
> By complementary I meant both of your patches add similar things, like
> vhost_user_set_vring_kick() becoming an int (even if Nikolay's version
> doesn't return an error for now).
> 
> > 
> > So, what's the plan? What do you think?
> 
> On one hand Nikolay's series was posted first, and it might be easier to
> just apply your patch on top. On the other hand, your patch fixes an
> issue, so it will be easier to backport it if it is applied first.
> 
> I guess we should go for your patch first to ease backporting.
> 
> I'd like Tiwei's feedback before applying anything, as he reviewed the
> first versions of both yours and Nikolay's patches.

I also incline to merge the fix patch first, as it needs
to be backported.

> 
> > Few comments inline.
> > 
> > [1] http://patchwork.dpdk.org/patch/44168/
[...]

Re: [dpdk-dev] [PATCH v2 0/5] vhost_user.c code cleanup

[Maxime Coquelin]

Hi Nikolay,

On 07/19/2018 09:13 PM, Nikolay Nikolaev wrote:
> vhost: vhost_user.c code cleanup
> 
> This patchesries introduce a set of code redesigns in vhost_user.c.
> 
> The goal is to unify and simplify vhost-user message handling. The
> patches do not intend to introduce any functional changes.
> 
> v2 changes:
>   - Fix the comments by Tiwei Bie
>   - Keep the old behavior
>     - Fall through when the callback returns VH_RESULT_ERR
>     - Fall through if the request is out of range
> 
> ---
> 
> Nikolay Nikolaev (5):
>        vhost: unify VhostUserMsg usage
>        vhost: make message handling functions prepare the reply
>        vhost: handle unsupported message types in functions
>        vhost: unify message handling function signature
>        vhost: message handling implemented as a callback array
> 
> 
>   lib/librte_vhost/vhost_user.c |  392 ++++++++++++++++++++++-------------------
>   1 file changed, 208 insertions(+), 184 deletions(-)
> 
> --
> Signature
> 

Could you please rebase and fix the series (see Iliya comments) on top
of git://dpdk.org/next/dpdk-next-virtio master branch?

Thanks,
Maxime

Re: [dpdk-dev] [PATCH v2 0/5] vhost_user.c code cleanup

[Nikolay Nikolaev]

Hello Maxime,

I'll rebase and fix Ilya's comments. Thanks for the reviews.

regards,
Nikolay Nikolaev

On Tue, Sep 11, 2018 at 12:38 PM Maxime Coquelin <maxime.coquelin@redhat.com>
wrote:

> Hi Nikolay,
>
> On 07/19/2018 09:13 PM, Nikolay Nikolaev wrote:
> > vhost: vhost_user.c code cleanup
> >
> > This patchesries introduce a set of code redesigns in vhost_user.c.
> >
> > The goal is to unify and simplify vhost-user message handling. The
> > patches do not intend to introduce any functional changes.
> >
> > v2 changes:
> >   - Fix the comments by Tiwei Bie
> >   - Keep the old behavior
> >     - Fall through when the callback returns VH_RESULT_ERR
> >     - Fall through if the request is out of range
> >
> > ---
> >
> > Nikolay Nikolaev (5):
> >        vhost: unify VhostUserMsg usage
> >        vhost: make message handling functions prepare the reply
> >        vhost: handle unsupported message types in functions
> >        vhost: unify message handling function signature
> >        vhost: message handling implemented as a callback array
> >
> >
> >   lib/librte_vhost/vhost_user.c |  392
> ++++++++++++++++++++++-------------------
> >   1 file changed, 208 insertions(+), 184 deletions(-)
> >
> > --
> > Signature
> >
>
> Could you please rebase and fix the series (see Iliya comments) on top
> of git://dpdk.org/next/dpdk-next-virtio master branch?
>
> Thanks,
> Maxime
>