Hi
On Tue, May 9, 2023 at 3:17 PM Albert Esteve <aest...@redhat.com> wrote:
>
> Hi!
>
> On Tue, May 9, 2023 at 12:12 PM Marc-André Lureau <
> marcandre.lur...@gmail.com> wrote:
>
>> Hi
>>
>> On Wed, May 3, 2023 at 12:21 PM Albert Esteve <aest...@redhat.com> wrote:
>>
>>> Add vu_write_msg_cb type as a member of the VuDev
>>> struct.
>>>
>>> In order to interact with the virtio-dmabuf
>>> API, vhost-user backends have available a special
>>> message type that can be sent to the frontend
>>> in Qemu, in order to add, lookup, or remove
>>> entries.
>>>
>>> To send these messages and avoid code replication,
>>> backends will need the write_msg method to be exposed
>>> to them, similarly to how the read_msg is for
>>> receiving messages.
>>>
>>
>> I think read_msg was introduced to blend libvhost-user IO to qemu
>> mainloop & coroutine. Is that what you have in mind for write_msg?
>>
>
> Uhm, after grep'ing, it seems that read_msg is only used within
> libvhost-user source, so I guess it is mainly used to
> allow backends to provide custom methods? Maybe I am misunderstanding.
>
> But my idea for adding `write_msg` is exposing the write method (i.e.,
> vu_message_write) to the backends,
> without having the function signature in the header. This way, vhost-user
> backends that want to write a message,
> can just use `dev->write_msg(args...)`. Which would be equivalent to
> `vu_message_write(args...)` if this
> was visible for others.
>
Imho it's better to introduce a normal function in that case, that is
simply export vu_message_write_default().
> Another option could be to have a specific public method sending the
> requests to the frontend, that
> internally, would use `vu_message_write`. But since we introduce three new
> message types that
> backends can send, I thought adding different methods would be a bit too
> verbose.
>
Actually, exposing higher-level methods to send specific messages is more
correct imho.
>>
>>> Signed-off-by: Albert Esteve <aest...@redhat.com>
>>> ---
>>> subprojects/libvhost-user/libvhost-user.c | 1 +
>>> subprojects/libvhost-user/libvhost-user.h | 16 ++++++++++++++++
>>> 2 files changed, 17 insertions(+)
>>>
>>> diff --git a/subprojects/libvhost-user/libvhost-user.c
>>> b/subprojects/libvhost-user/libvhost-user.c
>>> index 6b4b721225..c50b353915 100644
>>> --- a/subprojects/libvhost-user/libvhost-user.c
>>> +++ b/subprojects/libvhost-user/libvhost-user.c
>>> @@ -2115,6 +2115,7 @@ vu_init(VuDev *dev,
>>> dev->sock = socket;
>>> dev->panic = panic;
>>> dev->read_msg = read_msg ? read_msg : vu_message_read_default;
>>> + dev->write_msg = vu_message_write;
>>>
>>
>> You are not making it customizable? And the callback is not used.
>>
>
> Making it customizable would require changing the signature of `vu_init`,
> and I did not see
> the need for this usecase. I just want to expose the static method to the
> backends.
>
>
ok
> The callback is not used because there is still no virtio device to use
> it. But this whole
> infrastructure will be nice to have for the next device that would require
> it (e.g., virtio-video).
>
> In that regard, this commit could be skipped from the PATCH and just
> change it once there
> is a virtio device that needs to send a `VHOST_USER_BACKEND_SHARED_OBJECT`
> message. Basically, I needed it for testing (just had a dummy vhost-user
> backend that I used for
> sending messages), and then decided to keep it. But maybe having a simpler
> patch is better.
>
>
>>
>>
>>> dev->set_watch = set_watch;
>>> dev->remove_watch = remove_watch;
>>> dev->iface = iface;
>>> diff --git a/subprojects/libvhost-user/libvhost-user.h
>>> b/subprojects/libvhost-user/libvhost-user.h
>>> index 784db65f7c..f5d7162886 100644
>>> --- a/subprojects/libvhost-user/libvhost-user.h
>>> +++ b/subprojects/libvhost-user/libvhost-user.h
>>> @@ -242,6 +242,7 @@ typedef void (*vu_set_features_cb) (VuDev *dev,
>>> uint64_t features);
>>> typedef int (*vu_process_msg_cb) (VuDev *dev, VhostUserMsg *vmsg,
>>> int *do_reply);
>>> typedef bool (*vu_read_msg_cb) (VuDev *dev, int sock, VhostUserMsg
>>> *vmsg);
>>> +typedef bool (*vu_write_msg_cb) (VuDev *dev, int sock, VhostUserMsg
>>> *vmsg);
>>> typedef void (*vu_queue_set_started_cb) (VuDev *dev, int qidx, bool
>>> started);
>>> typedef bool (*vu_queue_is_processed_in_order_cb) (VuDev *dev, int
>>> qidx);
>>> typedef int (*vu_get_config_cb) (VuDev *dev, uint8_t *config, uint32_t
>>> len);
>>> @@ -429,6 +430,21 @@ struct VuDev {
>>> */
>>> vu_read_msg_cb read_msg;
>>>
>>> + /*
>>> + * @write_msg: custom method to write vhost-user message
>>> + *
>>> + * Write data to vhost_user socket fd from the passed
>>> + * VhostUserMsg *vmsg struct.
>>> + *
>>> + * For the details, please refer to vu_message_write in
>>> libvhost-user.c
>>> + * which will be used by default when calling vu_unit.
>>> + * No custom method is allowed.
>>>
>>
>> "No custom method is allowed"?
>>
>
> I meant that I am not making it customizable (from your previous point),
> as opposed to the `read_msg` method.
>
>
>>
>>
>>> + *
>>> + * Returns: true if vhost-user message successfully sent, false
>>> otherwise.
>>> + *
>>> + */
>>> + vu_write_msg_cb write_msg;
>>> +
>>>
>>
>>
>> --
>> Marc-André Lureau
>>
>
--
Marc-André Lureau
