Re: [PATCH v2 5/7] vhost-user: hoist "write_sync", "get_features", "get_u64"

[Laszlo Ersek <lersek@redhat.com>]

On 8/30/23 15:40, Laszlo Ersek wrote:
> In order to avoid a forward-declaration for "vhost_user_write_sync" in a
> subsequent patch, hoist "vhost_user_write_sync" ->
> "vhost_user_get_features" -> "vhost_user_get_u64" just above
> "vhost_set_vring".
> 
> This is purely code movement -- no observable change.
> 
> Cc: "Michael S. Tsirkin" <mst@redhat.com> (supporter:vhost)
> Cc: Eugenio Perez Martin <eperezma@redhat.com>
> Cc: German Maglione <gmaglione@redhat.com>
> Cc: Liu Jiang <gerry@linux.alibaba.com>
> Cc: Sergio Lopez Pascual <slp@redhat.com>
> Cc: Stefano Garzarella <sgarzare@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> 
> Notes:
>     v2:
>     
>     - pick up R-b from Stefano
>     
>     - rename "vhost_user_write_msg" to "vhost_user_write_sync" (in code and
>       commit message) [Stefano]
> 
>  hw/virtio/vhost-user.c | 170 ++++++++++----------
>  1 file changed, 85 insertions(+), 85 deletions(-)

Phil reviewed v1:

http://mid.mail-archive.com/98150923-39ef-7581-6144-8d0ad8d4dd52@linaro.org

and I would've kept his R-b (similar to Stefano's) across the
vhost_user_write_msg->vhost_user_write_sync rename in v2; so I'm copying
it here:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Hope that's OK.

Laszlo

> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 4129ba72e408..c79b6f77cdca 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1083,6 +1083,91 @@ static int vhost_user_set_vring_endian(struct vhost_dev *dev,
>      return vhost_user_write(dev, &msg, NULL, 0);
>  }
>  
> +static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t *u64)
> +{
> +    int ret;
> +    VhostUserMsg msg = {
> +        .hdr.request = request,
> +        .hdr.flags = VHOST_USER_VERSION,
> +    };
> +
> +    if (vhost_user_per_device_request(request) && dev->vq_index != 0) {
> +        return 0;
> +    }
> +
> +    ret = vhost_user_write(dev, &msg, NULL, 0);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    ret = vhost_user_read(dev, &msg);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    if (msg.hdr.request != request) {
> +        error_report("Received unexpected msg type. Expected %d received %d",
> +                     request, msg.hdr.request);
> +        return -EPROTO;
> +    }
> +
> +    if (msg.hdr.size != sizeof(msg.payload.u64)) {
> +        error_report("Received bad msg size.");
> +        return -EPROTO;
> +    }
> +
> +    *u64 = msg.payload.u64;
> +
> +    return 0;
> +}
> +
> +static int vhost_user_get_features(struct vhost_dev *dev, uint64_t *features)
> +{
> +    if (vhost_user_get_u64(dev, VHOST_USER_GET_FEATURES, features) < 0) {
> +        return -EPROTO;
> +    }
> +
> +    return 0;
> +}
> +
> +/* Note: "msg->hdr.flags" may be modified. */
> +static int vhost_user_write_sync(struct vhost_dev *dev, VhostUserMsg *msg,
> +                                 bool wait_for_reply)
> +{
> +    int ret;
> +
> +    if (wait_for_reply) {
> +        bool reply_supported = virtio_has_feature(dev->protocol_features,
> +                                          VHOST_USER_PROTOCOL_F_REPLY_ACK);
> +        if (reply_supported) {
> +            msg->hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
> +        }
> +    }
> +
> +    ret = vhost_user_write(dev, msg, NULL, 0);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    if (wait_for_reply) {
> +        uint64_t dummy;
> +
> +        if (msg->hdr.flags & VHOST_USER_NEED_REPLY_MASK) {
> +            return process_message_reply(dev, msg);
> +        }
> +
> +       /*
> +        * We need to wait for a reply but the backend does not
> +        * support replies for the command we just sent.
> +        * Send VHOST_USER_GET_FEATURES which makes all backends
> +        * send a reply.
> +        */
> +        return vhost_user_get_features(dev, &dummy);
> +    }
> +
> +    return 0;
> +}
> +
>  static int vhost_set_vring(struct vhost_dev *dev,
>                             unsigned long int request,
>                             struct vhost_vring_state *ring)
> @@ -1255,91 +1340,6 @@ static int vhost_user_set_vring_err(struct vhost_dev *dev,
>      return vhost_set_vring_file(dev, VHOST_USER_SET_VRING_ERR, file);
>  }
>  
> -static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t *u64)
> -{
> -    int ret;
> -    VhostUserMsg msg = {
> -        .hdr.request = request,
> -        .hdr.flags = VHOST_USER_VERSION,
> -    };
> -
> -    if (vhost_user_per_device_request(request) && dev->vq_index != 0) {
> -        return 0;
> -    }
> -
> -    ret = vhost_user_write(dev, &msg, NULL, 0);
> -    if (ret < 0) {
> -        return ret;
> -    }
> -
> -    ret = vhost_user_read(dev, &msg);
> -    if (ret < 0) {
> -        return ret;
> -    }
> -
> -    if (msg.hdr.request != request) {
> -        error_report("Received unexpected msg type. Expected %d received %d",
> -                     request, msg.hdr.request);
> -        return -EPROTO;
> -    }
> -
> -    if (msg.hdr.size != sizeof(msg.payload.u64)) {
> -        error_report("Received bad msg size.");
> -        return -EPROTO;
> -    }
> -
> -    *u64 = msg.payload.u64;
> -
> -    return 0;
> -}
> -
> -static int vhost_user_get_features(struct vhost_dev *dev, uint64_t *features)
> -{
> -    if (vhost_user_get_u64(dev, VHOST_USER_GET_FEATURES, features) < 0) {
> -        return -EPROTO;
> -    }
> -
> -    return 0;
> -}
> -
> -/* Note: "msg->hdr.flags" may be modified. */
> -static int vhost_user_write_sync(struct vhost_dev *dev, VhostUserMsg *msg,
> -                                 bool wait_for_reply)
> -{
> -    int ret;
> -
> -    if (wait_for_reply) {
> -        bool reply_supported = virtio_has_feature(dev->protocol_features,
> -                                          VHOST_USER_PROTOCOL_F_REPLY_ACK);
> -        if (reply_supported) {
> -            msg->hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
> -        }
> -    }
> -
> -    ret = vhost_user_write(dev, msg, NULL, 0);
> -    if (ret < 0) {
> -        return ret;
> -    }
> -
> -    if (wait_for_reply) {
> -        uint64_t dummy;
> -
> -        if (msg->hdr.flags & VHOST_USER_NEED_REPLY_MASK) {
> -            return process_message_reply(dev, msg);
> -        }
> -
> -       /*
> -        * We need to wait for a reply but the backend does not
> -        * support replies for the command we just sent.
> -        * Send VHOST_USER_GET_FEATURES which makes all backends
> -        * send a reply.
> -        */
> -        return vhost_user_get_features(dev, &dummy);
> -    }
> -
> -    return 0;
> -}
> -
>  static int vhost_user_set_vring_addr(struct vhost_dev *dev,
>                                       struct vhost_vring_addr *addr)
>  {
>