Message ID | 1606575020-2973-1-git-send-email-17826875952@163.com (mailing list archive) |
---|---|
State | Superseded, archived |
Delegated to: | Maxime Coquelin |
Headers | show |
Series | net/virtio-user: fix error run close(0) | expand |
Context | Check | Description |
---|---|---|
ci/travis-robot | success | Travis build: passed |
ci/iol-mellanox-Performance | success | Performance Testing PASS |
ci/iol-intel-Performance | success | Performance Testing PASS |
ci/iol-intel-Functional | success | Functional Testing PASS |
ci/iol-testing | success | Testing PASS |
ci/iol-broadcom-Functional | success | Functional Testing PASS |
ci/iol-broadcom-Performance | success | Performance Testing PASS |
ci/Intel-compilation | success | Compilation OK |
ci/checkpatch | success | coding style OK |
Hi Jiawei, Thanks for catching this! Comments inline. > -----Original Message----- > From: Jiawei Zhu <17826875952@163.com> > Sent: Saturday, November 28, 2020 10:50 PM > To: dev@dpdk.org > Cc: liweifeng2@huawei.com; zhujiawei12@huawei.com; maxime.coquelin@redhat.com; > Xia, Chenbo <chenbo.xia@intel.com> > Subject: [PATCH] net/virtio-user: fix error run close(0) > > From: Jiawei Zhu <zhujiawei12@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: stable@dpdk.org > > Signed-off-by: Jiawei Zhu <zhujiawei12@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? 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
On 12/9/20 12:31 PM, Xia, Chenbo wrote: > Hi Jiawei, > > Thanks for catching this! > Comments inline. > >> -----Original Message----- >> From: Jiawei Zhu <17826875952@163.com> >> Sent: Saturday, November 28, 2020 10:50 PM >> To: dev@dpdk.org >> Cc: liweifeng2@huawei.com; zhujiawei12@huawei.com; maxime.coquelin@redhat.com; >> Xia, Chenbo <chenbo.xia@intel.com> >> Subject: [PATCH] net/virtio-user: fix error run close(0) >> >> From: Jiawei Zhu <zhujiawei12@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: stable@dpdk.org >> >> Signed-off-by: Jiawei Zhu <zhujiawei12@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? +1, I noticed the same discussing the patch with David. > 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/) I personally don't care about the --in-reply-to, but I agree for the versioning. Thanks, Maxime > Thanks! > Chenbo > >> close(dev->callfds[j]); >> close(dev->kickfds[j]); >> } >> -- >> 1.8.3.1 >
Hi Jiawei, >From: 17826875952 <17826875952@163.com> >Sent: Friday, December 11, 2020 1:31 AM >To: Xia, Chenbo <chenbo.xia@intel.com> >Cc: maxime.coquelin@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@intel.com> wrote: >>Hi Jiawei, >> >>Thanks for catching this! >>Comments inline. >> >>> -----Original Message----- >>> From: Jiawei Zhu <mailto:17826875952@163.com> >>> Sent: Saturday, November 28, 2020 10:50 PM >>> To: mailto:dev@dpdk.org >>> Cc: mailto:liweifeng2@huawei.com; mailto:zhujiawei12@huawei.com; mailto:maxime.coquelin@redhat.com; >>> Xia, Chenbo <mailto:chenbo.xia@intel.com> >>> Subject: [PATCH] net/virtio-user: fix error run close(0) >>> >>> From: Jiawei Zhu <mailto:zhujiawei12@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@dpdk.org >>> >>> Signed-off-by: Jiawei Zhu <mailto:zhujiawei12@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@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
Hi Chenbo, Thanks for your advices! But When I use 'git send-email --to dev@dpdk.org --in-reply-to 1607703293-6121-1-git-send-email-17826875952@163.com --suppress-cc=all 0001-net-virtio-user-fix-run-close-0-and-close-callfd.patch', it still send new same one in patchwork.😭.Please help me delete the other,Tks! And the new version patch is: https://patches.dpdk.org/patch/85019/ If this patch is ok, please give me a reply! Thanks! Jiawei | | zhujw 邮箱:zhujw@zju.edu.cn | On 12/11/2020 09:58, Xia, Chenbo wrote: Hi Jiawei, >From: 17826875952 <17826875952@163.com> >Sent: Friday, December 11, 2020 1:31 AM >To: Xia, Chenbo <chenbo.xia@intel.com> >Cc: maxime.coquelin@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@intel.com> wrote: >>Hi Jiawei, >> >>Thanks for catching this! >>Comments inline. >> >>> -----Original Message----- >>> From: Jiawei Zhu <mailto:17826875952@163.com> >>> Sent: Saturday, November 28, 2020 10:50 PM >>> To: mailto:dev@dpdk.org >>> Cc: mailto:liweifeng2@huawei.com; mailto:zhujiawei12@huawei.com; mailto:maxime.coquelin@redhat.com; >>> Xia, Chenbo <mailto:chenbo.xia@intel.com> >>> Subject: [PATCH] net/virtio-user: fix error run close(0) >>> >>> From: Jiawei Zhu <mailto:zhujiawei12@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@dpdk.org >>> >>> Signed-off-by: Jiawei Zhu <mailto:zhujiawei12@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@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
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) { close(dev->callfds[j]); close(dev->kickfds[j]); }