> On Jul 31, 2023, at 7:38 AM, Li Feng <fen...@smartx.com> wrote:
>
>
>
>> 2023年7月31日 06:13，Raphael Norwitz <raphael.norw...@nutanix.com> 写道：
>>
>>>
>>> On Jul 28, 2023, at 3:49 AM, Li Feng <fen...@smartx.com> wrote:
>>>
>>>
>>>
>>>> 2023年7月28日 下午2:04，Michael S. Tsirkin <m...@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 <fen...@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.
>
Makes sense.
>>>>> + 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.
>
On re-review I agree it’s fine since vac-inflight isn’t set.
>>
>>>>> - 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
