Re: [PATCH v3 4/4] vhost-user: refactor send_resp code

[Michael S. Tsirkin]

On Wed, May 24, 2023 at 11:13:33AM +0200, Albert Esteve wrote:
> Refactor code to send response message so that
> all common parts both for the common REPLY_ACK
> case, and other data responses, can call it and
> avoid code repetition.
> 
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> ---
>  hw/virtio/vhost-user.c | 52 +++++++++++++++++++-----------------------
>  1 file changed, 24 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 5ac5f0eafd..b888f2c177 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1619,28 +1619,36 @@ static int 
> vhost_user_backend_handle_shared_object(VhostUserShared *object)
>      return 0;
>  }
>  
> -static bool
> -vhost_user_backend_send_dmabuf_fd(QIOChannel *ioc, VhostUserHeader *hdr,
> -                                  VhostUserPayload *payload)
> +static bool vhost_user_send_resp(QIOChannel *ioc, VhostUserHeader *hdr,
> +                                 VhostUserPayload *payload)
>  {
>      Error *local_err = NULL;
>      struct iovec iov[2];


As long as you are refactoring, please add an empty line here
after variable declaration.

Also, can't we initialize it here?
        struct iovec iov[] = {
                { .iov_base = hdr ....},
                { .iov_base = payload }
        };

will also avoid the need for explicit size.


> -    if (hdr->flags & VHOST_USER_NEED_REPLY_MASK) {
> -        hdr->flags &= ~VHOST_USER_NEED_REPLY_MASK;
> -        hdr->flags |= VHOST_USER_REPLY_MASK;
> +    hdr->flags &= ~VHOST_USER_NEED_REPLY_MASK;
> +    hdr->flags |= VHOST_USER_REPLY_MASK;
>  
> -        hdr->size = sizeof(payload->object);
> +    iov[0].iov_base = hdr;
> +    iov[0].iov_len = VHOST_USER_HDR_SIZE;
> +    iov[1].iov_base = payload;
> +    iov[1].iov_len = hdr->size;
> +
> +    if (qio_channel_writev_all(ioc, iov, ARRAY_SIZE(iov), &local_err)) {
> +        error_report_err(local_err);
> +        return false;
> +    }
>  
> -        iov[0].iov_base = hdr;
> -        iov[0].iov_len = VHOST_USER_HDR_SIZE;
> -        iov[1].iov_base = payload;
> -        iov[1].iov_len = hdr->size;
> +    return true;
> +}
>  
> -        if (qio_channel_writev_all(ioc, iov, ARRAY_SIZE(iov), &local_err)) {
> -            error_report_err(local_err);
> -            return false;
> -        }
> +static bool
> +vhost_user_backend_send_dmabuf_fd(QIOChannel *ioc, VhostUserHeader *hdr,
> +                                  VhostUserPayload *payload)
> +{
> +    if (hdr->flags & VHOST_USER_NEED_REPLY_MASK) {
> +        hdr->size = sizeof(payload->object);
> +        return vhost_user_send_resp(ioc, hdr, payload);
>      }
> +
>      return true;
>  }
>  
> @@ -1717,22 +1725,10 @@ static gboolean slave_read(QIOChannel *ioc, 
> GIOCondition condition,
>       * directly in their request handlers.
>       */
>      if (hdr.flags & VHOST_USER_NEED_REPLY_MASK) {
> -        struct iovec iovec[2];
> -
> -
> -        hdr.flags &= ~VHOST_USER_NEED_REPLY_MASK;
> -        hdr.flags |= VHOST_USER_REPLY_MASK;
> -
>          payload.u64 = !!ret;
>          hdr.size = sizeof(payload.u64);
>  
> -        iovec[0].iov_base = &hdr;
> -        iovec[0].iov_len = VHOST_USER_HDR_SIZE;
> -        iovec[1].iov_base = &payload;
> -        iovec[1].iov_len = hdr.size;
> -
> -        if (qio_channel_writev_all(ioc, iovec, ARRAY_SIZE(iovec), 
> &local_err)) {
> -            error_report_err(local_err);
> +        if (!vhost_user_send_resp(ioc, &hdr, &payload)) {
>              goto err;
>          }
>      }
> -- 
> 2.40.0