<div dir="ltr"><div dir="ltr"><div><div dir="ltr" class="gmail_signature"><div dir="ltr"><br></div></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jul 10, 2023 at 6:44 PM Alex Bennée <<a href="mailto:alex.bennee@linaro.org">alex.bennee@linaro.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Instead of requiring all the information up front allow the<br>
vhost_dev_init to complete and then see what information we have from<br>
the backend.<br>
<br>
This does change the order around somewhat.<br>
<br>
Signed-off-by: Alex Bennée <<a href="mailto:alex.bennee@linaro.org" target="_blank">alex.bennee@linaro.org</a>><br>
---<br>
 hw/virtio/vhost-user-device.c | 45 +++++++++++++++++++++++++----------<br>
 1 file changed, 32 insertions(+), 13 deletions(-)<br>
<br>
diff --git a/hw/virtio/vhost-user-device.c b/hw/virtio/vhost-user-device.c<br>
index 0109d4829d..b30b6265fb 100644<br>
--- a/hw/virtio/vhost-user-device.c<br>
+++ b/hw/virtio/vhost-user-device.c<br>
@@ -243,7 +243,6 @@ static void vub_device_realize(DeviceState *dev, Error **errp)<br>
 {<br>
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);<br>
     VHostUserBase *vub = VHOST_USER_BASE(dev);<br>
-    int ret;<br>
<br>
     if (!vub->chardev.chr) {<br>
         error_setg(errp, "vhost-user-device: missing chardev");<br>
@@ -254,13 +253,43 @@ static void vub_device_realize(DeviceState *dev, Error **errp)<br>
         return;<br>
     }<br>
<br>
+    if (vhost_dev_init(&vub->vhost_dev, &vub->vhost_user,<br>
+                       VHOST_BACKEND_TYPE_USER, 0, errp)!=0) {<br>
+        error_setg(errp, "vhost-user-device: unable to start connection");<br>
+        return;<br>
+    }<br>
+<br>
+    if (vub->vhost_dev.specs.device_id) {<br>
+        if (vub->virtio_id && vub->virtio_id != vub->vhost_dev.specs.device_id) {<br>
+            error_setg(errp, "vhost-user-device: backend id %d doesn't match cli %d",<br>
+                       vub->vhost_dev.specs.device_id, vub->virtio_id);<br>
+            return;<br>
+        }<br>
+        vub->virtio_id = vub->vhost_dev.specs.device_id;<br>
+    }<br>
+<br>
     if (!vub->virtio_id) {<br>
-        error_setg(errp, "vhost-user-device: need to define device id");<br>
+        error_setg(errp, "vhost-user-device: need to define or be told device id");<br>
         return;<br>
     }<br>
<br>
+    if (vub->vhost_dev.specs.min_vqs) {<br>
+        if (vub->num_vqs) {<br>
+            if (vub->num_vqs < vub->vhost_dev.specs.min_vqs ||<br>
+                vub->num_vqs > vub->vhost_dev.specs.max_vqs) {<br>
+                error_setg(errp,<br>
+                           "vhost-user-device: selected nvqs (%d) out of bounds (%d->%d)",<br>
+                           vub->num_vqs,<br>
+                           vub->vhost_dev.specs.min_vqs, vub->vhost_dev.specs.max_vqs);<br>
+                return;<br>
+            }<br>
+        } else {<br>
+            vub->num_vqs = vub->vhost_dev.specs.min_vqs;<br>
+        }<br>
+    }<br>
+<br>
     if (!vub->num_vqs) {<br>
-        vub->num_vqs = 1; /* reasonable default? */<br>
+        error_setg(errp, "vhost-user-device: need to define number of vqs");<br>
     }<br>
<br>
     /*<br>
@@ -287,16 +316,6 @@ static void vub_device_realize(DeviceState *dev, Error **errp)<br>
                         virtio_add_queue(vdev, 4, vub_handle_output));<br>
     }<br>
<br>
-    vub->vhost_dev.nvqs = vub->num_vqs;<br></blockquote><div><br></div><div>Who sets `vub->vhost_dev.nvqs` after removing this line?</div><div>Why having `vub->num_vqs` in the first place? In vub_start for example we still</div><div>use `vub->vhost_dev.nvqs`, and we pass `vhost_dev` to other functions that</div><div>use its `nvqs`, so `num_vqs` is redundant and requires a logic to copy/initialise `vhost_dev.nvqs`.</div><div><br></div><div>Maybe it would be better to initialse `nvqs` through a function, in the device file, instead of doing:</div><div>`vub->num_vqs = 2;`</div><div>We could have:<br></div><div>`vub_set_nvqs(vub, 2);`</div><div>Or something along those lines. And the function will have all the internal logic in this commit, i.e.,<br></div><div>checking the boundaries, setting the `vhost_dev.nvqs` value, printing the error, etc.</div><div>So we can save the extra variable, and the logic to copy the value to the device.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
-<br>
-    /* connect to backend */<br>
-    ret = vhost_dev_init(&vub->vhost_dev, &vub->vhost_user,<br>
-                         VHOST_BACKEND_TYPE_USER, 0, errp);<br>
-<br>
-    if (ret < 0) {<br>
-        do_vhost_user_cleanup(vdev, vub);<br>
-    }<br>
-<br>
     qemu_chr_fe_set_handlers(&vub->chardev, NULL, NULL, vub_event, NULL,<br>
                              dev, NULL, true);<br>
 }<br>
-- <br>
2.39.2<br>
<br>
<br>
</blockquote></div></div>