Re: [PATCH 4/4] libvhost-user: add write_msg cb to dev struct

[Marc-André Lureau]

Hi

On Tue, May 9, 2023 at 3:17 PM Albert Esteve <aesteve@redhat.com> wrote:

Hi!

On Tue, May 9, 2023 at 12:12 PM Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

Hi

On Wed, May 3, 2023 at 12:21 PM Albert Esteve <aesteve@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 <aesteve@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