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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 6/9/23 08:04, Philippe Mathieu-Daudé wrote:
> On 2/8/23 11:08, 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.
>>
>> Signed-off-by: Albert Esteve <aesteve@redhat.com>
>> ---
>>   docs/interop/vhost-user.rst               |  57 ++++++++
>>   hw/virtio/vhost-user.c                    | 166 ++++++++++++++++++++++
>>   include/hw/virtio/vhost-backend.h         |   3 +
>>   subprojects/libvhost-user/libvhost-user.c | 118 +++++++++++++++
>>   subprojects/libvhost-user/libvhost-user.h |  55 ++++++-
>>   5 files changed, 398 insertions(+), 1 deletion(-)
> 
> 
>> +static bool
>> +vhost_user_backend_send_dmabuf_fd(QIOChannel *ioc, VhostUserHeader *hdr,
>> +                                  VhostUserPayload *payload)
>> +{
>> +    Error *local_err = NULL;
>> +    struct iovec iov[2];
>> +
>> +    if (hdr->flags & VHOST_USER_NEED_REPLY_MASK) {
>> +        hdr->flags &= ~VHOST_USER_NEED_REPLY_MASK;
>> +    }
>> +    hdr->flags |= VHOST_USER_REPLY_MASK;
>> +
>> +    hdr->size = sizeof(payload->u64);
>> +
>> +    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);
> 
> This function could have a 'Error **errp' parameter to propagate
> the error to the caller.
> 
>> +        return false;
>> +    }
>> +    return true;
>> +}
>> +
>> +static bool
>> +vhost_user_backend_send_dmabuf_fd(QIOChannel *ioc, VhostUserHeader *hdr,
>> +                                  VhostUserPayload *payload)
>> +{
>> +    hdr->size = sizeof(payload->u64);
>> +    return vhost_user_send_resp(ioc, hdr, payload);
>> +}
> 
> I'm confused by having two vhost_user_backend_send_dmabuf_fd() functions
> with different body...

This patch doesn't compile:

../../hw/virtio/vhost-user.c:1662:1: error: redefinition of 
‘vhost_user_backend_send_dmabuf_fd’
  1662 | vhost_user_backend_send_dmabuf_fd(QIOChannel *ioc, 
VhostUserHeader *hdr,
       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../hw/virtio/vhost-user.c:1636:1: note: previous definition of 
‘vhost_user_backend_send_dmabuf_fd’ with type ‘_Bool(QIOChannel *, 
VhostUserHeader *, VhostUserPayload *)’
  1636 | vhost_user_backend_send_dmabuf_fd(QIOChannel *ioc, 
VhostUserHeader *hdr,
       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../hw/virtio/vhost-user.c: In function 
‘vhost_user_backend_send_dmabuf_fd’:
../../hw/virtio/vhost-user.c:1666:12: error: implicit declaration of 
function ‘vhost_user_send_resp’; did you mean ‘vhost_user_set_u64’? 
[-Werror=implicit-function-declaration]
  1666 |     return vhost_user_send_resp(ioc, hdr, payload);
       |            ^~~~~~~~~~~~~~~~~~~~
       |            vhost_user_set_u64
../../hw/virtio/vhost-user.c:1666:12: error: nested extern declaration 
of ‘vhost_user_send_resp’ [-Werror=nested-externs]
At top level:
../../hw/virtio/vhost-user.c:1636:1: error: 
‘vhost_user_backend_send_dmabuf_fd’ defined but not used 
[-Werror=unused-function]
  1636 | vhost_user_backend_send_dmabuf_fd(QIOChannel *ioc, 
VhostUserHeader *hdr,
       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors