Re: [PATCH v6 3/3] vhost-user: add shared_object msg

[Philippe Mathieu-Daudé]

Hi Albert,

On 6/9/23 13:15, Albert Esteve wrote:
> Add three new vhost-user protocol
> `VHOST_USER_BACKEND_SHARED_OBJECT_* messages`.
> These new messages are sent from vhost-user
> back-ends to interact with the virtio-dmabuf
> table in order to add or remove themselves as
> virtio exporters, or lookup for virtio dma-buf
> shared objects.
>
> The action taken in the front-end depends
> on the type stored in the virtio shared
> object hash table.
>
> When the table holds a pointer to a vhost
> backend for a given UUID, the front-end sends
> a VHOST_USER_GET_SHARED_OBJECT to the
> backend holding the shared object.
>
> In the libvhost-user library we need to add
> helper functions to allow sending messages to
> interact with the virtio shared objects
> hash table.
>
> The messages can only be sent after successfully
> negotiating a new VHOST_USER_PROTOCOL_F_SHARED_OBJECT
> vhost-user protocol feature bit.
>
> Finally, 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>
> ---
>   docs/interop/vhost-user.rst               |  57 +++++++
>   hw/virtio/vhost-user.c                    | 174 ++++++++++++++++++++--
>   include/hw/virtio/vhost-backend.h         |   3 +
>   subprojects/libvhost-user/libvhost-user.c | 118 +++++++++++++++
>   subprojects/libvhost-user/libvhost-user.h |  55 ++++++-
>   5 files changed, 393 insertions(+), 14 deletions(-)

Almost 400 lines of changes is too much for me to review in a
single patch. Looking at the names, can't we split virtio VS
libvhost-user?

> +static bool vhost_user_send_resp(QIOChannel *ioc, VhostUserHeader *hdr,
> +                                 VhostUserPayload *payload)
> +{
> +    Error *local_err = NULL;
> +    struct iovec iov[] = {
> +        { .iov_base = hdr,      .iov_len = VHOST_USER_HDR_SIZE },
> +        { .iov_base = payload,  .iov_len = hdr->size },
> +    };
> +
> +    hdr->flags &= ~VHOST_USER_NEED_REPLY_MASK;
> +    hdr->flags |= VHOST_USER_REPLY_MASK;
> +
> +    if (qio_channel_writev_all(ioc, iov, ARRAY_SIZE(iov), &local_err)) {
> +        error_report_err(local_err);
> +        return false;
> +    }
> +
> +    return true;
> +}

I note you  ignored my comment regarding adding a 'Error **' argument in
the previous version:
911eef0c-d04f-2fcf-e78b-2475cd7af8f0@linaro.org/">https://lore.kernel.org/qemu-devel/911eef0c-d04f-2fcf-e78b-2475cd7af8f0@linaro.org/