[dpdk-dev] [PATCH] net/virtio-user: fix error run close(0)

[Xia, Chenbo]

Hi Jiawei,

>From: 17826875952 <17826875952 at 163.com> 
>Sent: Friday, December 11, 2020 1:31 AM
>To: Xia, Chenbo <chenbo.xia at intel.com>
>Cc: maxime.coquelin at redhat.com
>Subject: Re:RE: [PATCH] net/virtio-user: fix error run close(0)
>
>
>Hi Chenbo,
>Thanks for you comment！
>
>At 2020-12-09 19:31:19, "Xia, Chenbo" <mailto:chenbo.xia at intel.com> wrote:
>>Hi Jiawei,
>>
>>Thanks for catching this!
>>Comments inline.
>>
>>> -----Original Message-----
>>> From: Jiawei Zhu <mailto:17826875952 at 163.com>
>>> Sent: Saturday, November 28, 2020 10:50 PM
>>> To: mailto:dev at dpdk.org
>>> Cc: mailto:liweifeng2 at huawei.com; mailto:zhujiawei12 at huawei.com; mailto:maxime.coquelin at redhat.com;
>>> Xia, Chenbo <mailto:chenbo.xia at intel.com>
>>> Subject: [PATCH] net/virtio-user: fix error run close(0)
>>> 
>>> From: Jiawei Zhu <mailto:zhujiawei12 at huawei.com>
>>> 
>>> When i < VIRTIO_MAX_VIRTQUEUES and j == i,
>>> dev->callfds[i] and dev->kickfds[i] are default 0.
>>> So it will close(0), close the standard input (stdin).
>>> 
>>> Fixes: e6e7ad8b3024 ("net/virtio-user: move eventfd open/close into
>>> init/uninit")
>>> Cc: mailto:stable at dpdk.org
>>> 
>>> Signed-off-by: Jiawei Zhu <mailto:zhujiawei12 at huawei.com>
>>> ---
>>>  drivers/net/virtio/virtio_user/virtio_user_dev.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> index 053f026..1bfd223 100644
>>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> @@ -284,7 +284,7 @@ int virtio_user_stop_device(struct virtio_user_dev *dev)
>>>  	}
>>> 
>>>  	if (i < VIRTIO_MAX_VIRTQUEUES) {
>>> -		for (j = 0; j <= i; ++j) {
>>> +		for (j = 0; j < i; ++j) {
>>
>>With the help of your patch, I notice another defect that if the code fails in kickfd
>>creation, we will leave one callfd not closed. Since you are here, could you help solve
>>this too? A potential solution could be doing 'dev->callfds[i] = callfd' just after callfd
>>creation, keeping 'j <= i' and adding checks before close(). What do you think?
>
>This solution is ok to solve this，but I think the checks ars superfluous for  'j < i'.
>So I think adding ‘close(callfd)’ before break when fails in kickfd creation and keeping 'j < i'.
>What do you think?

Yes, that's also a viable solution. Please go ahead with this 😊.

Btw, next time you reply to patch email, please:
1. Better use plain text rather than HTML.
2. cc to dev at dpdk.org to make our discussion open to community.

And since you will send new version now, please add v2 as patch prefix. Otherwise maintainers will
be confused.

Thanks!
Chenbo

>
>>
>>Btw, I noticed that you have sent multiple patches that have same content. If you want to
>>send new version. Please --in-reply-to this patch as this is the one that shows in patchwork.
>>(http://patchwork.dpdk.org/patch/84626/)
>>
>>Thanks!
>>Chenbo
>>
>>>  			close(dev->callfds[j]);
>>>  			close(dev->kickfds[j]);
>>>  		}
>>> --
>>> 1.8.3.1
>>
>Thanks!
>Jiawei