On 1/6/21 12:57 PM, Xia, Chenbo wrote:

Hi Maxime,

quoted

-----Original Message-----
From: Maxime Coquelin <redacted>
Sent: Monday, December 21, 2020 5:14 AM
To: dev@dpdk.org; Xia, Chenbo <redacted>; olivier.matz@6wind.com;
amorenoz@redhat.com; david.marchand@redhat.com
Cc: Maxime Coquelin <redacted>
Subject: [PATCH 27/40] net/virtio: add Virtio-user memory tables ops

This patch implements a dedicated callback for
preparing and sending memory table to the backends.

Signed-off-by: Maxime Coquelin <redacted>
---

<snip>

quoted

+static int
+vhost_user_check_reply_ack(struct virtio_user_dev *dev, struct vhost_user_msg
*msg)
+{
+	enum vhost_user_request req = msg->request;
+	int ret;
+
+	if (!(msg->flags & VHOST_USER_NEED_REPLY_MASK))
+		return 0;
+
+	ret = vhost_user_read(dev->vhostfd, msg);
+	if (ret < 0) {
+		PMD_DRV_LOG(ERR, "Failed to read reply-ack");
+		return -1;
+	}
+
+	if (req != msg->request) {
+		PMD_DRV_LOG(ERR, "Unexpected reply-ack request type (%d)", msg-

quoted

request);

+		return -1;
+	}

I think it's better to keep the size check: msg.size should equal sizeof(msg.payload.u64).

quoted

+
+	return msg->payload.u64 ? -1 : 0;

I think it's better to add a log after checking payload's value. Looking back to
vhost_user_set_memory_table, there's no way for user or developer to know what has
failed (vhost_user_write fails or NACK). Maybe it's also better to add error log in
or outside vhost_user_write :)

Indeed, we are lacking logs here, I added both a log when slave replies
NACK and when sendmsg fails.

Thanks,
Maxime

Thanks,
Chenbo