On Sun, Aug 27, 2023 at 08:29:30PM +0200, Laszlo Ersek wrote:
The last patch in the series states and fixes the problem; prior patches
only refactor the code.
Thanks for the fix and great cleanup!
I fully reviewed the series and LGTM.
An additional step that we can take (not in this series) crossed my
mind, though. In some places we repeat the following pattern:
vhost_user_write(dev, , NULL, 0);
...
if (reply_supported) {
return process_message_reply(dev, );
}
So what about extending the vhost_user_write_msg() added in this series
to support also this cases and remove some code.
Or maybe integrate vhost_user_write_msg() in vhost_user_write().
I mean something like this (untested):
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 01e0ca90c5..9ee2a78afa 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1130,13 +1130,19 @@ static int vhost_user_get_features(struct vhost_dev
*dev, uint
64_t *features)
return 0;
}
+typedef enum {
+NO_REPLY,
+REPLY_IF_SUPPORTED,
+REPLY_FORCED,
+} VhostUserReply;
+
/* Note: "msg->hdr.flags" may be modified. */
static int vhost_user_write_msg(struct vhost_dev *dev, VhostUserMsg *msg,
-bool wait_for_reply)
+VhostUserReply reply)
{
int ret;
-if (wait_for_reply) {
+if (reply != NO_REPLY) {
bool reply_supported = virtio_has_feature(dev->protocol_features,
VHOST_USER_PROTOCOL_F_REPLY_ACK);
if (reply_supported) {
@@ -1149,7 +1155,7 @@ static int vhost_user_write_msg(struct vhost_dev *dev,
VhostUserMsg *msg,
return ret;
}
-if (wait_for_reply) {
+if (reply != NO_REPLY) {
uint64_t dummy;
if (msg->hdr.flags & VHOST_USER_NEED_REPLY_MASK) {
@@ -1162,7 +1168,9 @@ static int vhost_user_write_msg(struct vhost_dev
*dev, VhostUserMsg *msg,
* Send VHOST_USER_GET_FEATURES which makes all backends
* send a reply.
*/
-return vhost_user_get_features(dev, );
+if (reply == REPLY_FORCED) {
+return vhost_user_get_features(dev, );
+}
}
return 0;
@@ -2207,9 +2228,6 @@ static bool vhost_user_can_merge(struct vhost_dev *dev,
static int vhost_user_net_set_mtu(struct vhost_dev *dev, uint16_t mtu)
{
VhostUserMsg msg;
-bool reply_supported = virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_REPLY_ACK);
-int ret;
if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_NET_MTU))) {
return 0;
@@ -2219,21 +2237,9 @@ static int vhost_user_net_set_mtu(struct vhost_dev *dev,
uint16_t mtu)
msg.payload.u64 = mtu;
msg.hdr.size = sizeof(msg.payload.u64);
msg.hdr.flags = VHOST_USER_VERSION;
-if (reply_supported) {
-msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
-}
-
-ret = vhost_user_write(dev, , NULL, 0);
-if (ret < 0) {
-return ret;
-}
/* If reply_ack supported, backend has to ack specified MTU is valid */
-if (reply_supported) {
-return process_message_reply(dev, );
-}
-
-return 0;
+return vhost_user_write_msg(dev, , REPLY_IF_SUPPORTED);
}
static int vhost_user_send_device_iotlb_msg(struct vhost_dev *dev,
@@ -2313,10 +2319,7 @@ static int vhost_user_get_config(struct vhost_dev *dev,
uint8_t *config,
static int vhost_user_set_config(struct vhost_dev *dev, const uint8_t *data,
uint32_t offset, uint32_t size, uint32_t
flags)
{
-int ret;
uint8_t *p;
-bool reply_supported = virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_REPLY_ACK);
VhostUserMsg msg = {
.hdr.request = VHOST_USER_SET_CONFIG,
@@ -2329,10 +2332,6 @@ static int vhost_user_set_config(struct vhost_dev *dev,
const uint8_t *data,
return -ENOTSUP;
}
-if (reply_supported) {
-msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
-}
-
if (size > VHOST_USER_MAX_CONFIG_SIZE) {
return -EINVAL;
}
@@ -2343,16 +2342,7 @@ static int vhost_user_set_config(struct vhost_dev *dev,
const uint8_t *data,
p = msg.payload.config.region;
memcpy(p, data, size);
-ret = vhost_user_write(dev, , NULL, 0);
-if (ret < 0) {
-return ret;
-}
-
-if (reply_supported) {
-return process_message_reply(dev, );
-}
-
-return 0;
+return vhost_user_write_msg(dev, , REPLY_IF_SUPPORTED);
}
Thanks,
Stefano
