Re: [PATCH v2 2/4] vhost-user-common: send get_inflight_fd once

[Li Feng]

2023年7月31日 06:13，Raphael Norwitz <raphael.norwitz@nutanix.com> 写道：

On Jul 28, 2023, at 3:49 AM, Li Feng <fengli@smartx.com> wrote:

2023年7月28日 下午2:04，Michael S. Tsirkin <mst@redhat.com> 写道：

On Tue, Jul 25, 2023 at 06:42:45PM +0800, Li Feng wrote:

Get_inflight_fd is sent only once. When reconnecting to the backend,
qemu sent set_inflight_fd to the backend.

I don't understand what you are trying to say here.
Should be:
Currently ABCD. This is wrong/unnecessary because EFG. This patch HIJ.

Thanks, I will reorganize the commit message in v3.

Signed-off-by: Li Feng <fengli@smartx.com>
---
hw/scsi/vhost-scsi-common.c | 37 ++++++++++++++++++-------------------
1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
index a06f01af26..664adb15b4 100644
--- a/hw/scsi/vhost-scsi-common.c
+++ b/hw/scsi/vhost-scsi-common.c
@@ -52,20 +52,28 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)

   vsc->dev.acked_features = vdev->guest_features;

-    assert(vsc->inflight == NULL);
-    vsc->inflight = g_new0(struct vhost_inflight, 1);
-    ret = vhost_dev_get_inflight(&vsc->dev,
-                                 vs->conf.virtqueue_size,
-                                 vsc->inflight);
+    ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
   if (ret < 0) {
-        error_report("Error get inflight: %d", -ret);
+        error_report("Error setting inflight format: %d", -ret);
       goto err_guest_notifiers;
   }

-    ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
-    if (ret < 0) {
-        error_report("Error set inflight: %d", -ret);
-        goto err_guest_notifiers;
+    if (vsc->inflight) {
+        if (!vsc->inflight->addr) {
+            ret = vhost_dev_get_inflight(&vsc->dev,
+                                        vs->conf.virtqueue_size,
+                                        vsc->inflight);
+            if (ret < 0) {
+                error_report("Error get inflight: %d", -ret);

As long as you are fixing this - should be "getting inflight”.

I will fix it in v3.

+                goto err_guest_notifiers;
+            }
+        }
+

Looks like you reworked this a bit so to avoid a potential crash if vsc->inflight is NULL

Should we fix it for vhost-user-blk too?

This check is mainly for the vhost-scsi code, that doesn’t need allocate the inflight memory.

The vhost-user-blk doesn’t need this check, because there isn't a vhost-blk device that reuse the code.

+        ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
+        if (ret < 0) {
+            error_report("Error set inflight: %d", -ret);
+            goto err_guest_notifiers;
+        }
   }

   ret = vhost_dev_start(&vsc->dev, vdev, true);
@@ -85,9 +93,6 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
   return ret;

err_guest_notifiers:
-    g_free(vsc->inflight);
-    vsc->inflight = NULL;
-
   k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
err_host_notifiers:
   vhost_dev_disable_notifiers(&vsc->dev, vdev);
@@ -111,12 +116,6 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
   }
   assert(ret >= 0);

As I said before, I think this introduces a leak.

I have answered in the previous mail.

-    if (vsc->inflight) {
-        vhost_dev_free_inflight(vsc->inflight);
-        g_free(vsc->inflight);
-        vsc->inflight = NULL;
-    }
-
   vhost_dev_disable_notifiers(&vsc->dev, vdev);
}

-- 
2.41.0