| Message ID | 20210119212507.1043636-45-maxime.coquelin@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Maxime Coquelin |
| Headers | show |
| Series | net/virtio: Virtio PMD rework | expand |
| Context | Check | Description |
|---|---|---|
| ci/iol-testing | warning | Testing issues |
| ci/iol-abi-testing | success | Testing PASS |
| ci/iol-mellanox-Functional | success | Functional Testing PASS |
| ci/intel-Testing | success | Testing PASS |
| ci/iol-intel-Functional | success | Functional Testing PASS |
| ci/iol-intel-Performance | success | Performance Testing PASS |
| ci/iol-broadcom-Performance | success | Performance Testing PASS |
| ci/iol-broadcom-Functional | success | Functional Testing PASS |
| ci/Intel-compilation | success | Compilation OK |
| ci/checkpatch | success | coding style OK |
Hi Maxime, > -----Original Message----- > From: Maxime Coquelin <maxime.coquelin@redhat.com> > Sent: Wednesday, January 20, 2021 5:25 AM > To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; olivier.matz@6wind.com; > amorenoz@redhat.com; david.marchand@redhat.com > Cc: Maxime Coquelin <maxime.coquelin@redhat.com> > Subject: [PATCH v2 44/44] net/virtio: handle Virtio-user setup failure > properly > > This patch fixes virtio_user_dev_setup() error path, > by cleaning all resources it allocates. > > Suggested-by: Adrian Moreno <amorenoz@redhat.com> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > --- > drivers/net/virtio/virtio_user/virtio_user_dev.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c > b/drivers/net/virtio/virtio_user/virtio_user_dev.c > index a1e32158bb..2f7dc574b6 100644 > --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c > +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c > @@ -427,15 +427,22 @@ virtio_user_dev_setup(struct virtio_user_dev *dev) > > if (virtio_user_dev_init_notify(dev) < 0) { > PMD_INIT_LOG(ERR, "(%s) Failed to init notifiers\n", dev->path); > - return -1; > + goto destroy; > } > > if (virtio_user_fill_intr_handle(dev) < 0) { > PMD_INIT_LOG(ERR, "(%s) Failed to init interrupt handler\n", dev- > >path); > - return -1; > + goto uninit; > } > > return 0; > + > +uninit: > + virtio_user_dev_uninit(dev); IMHO, it may not be the best choice to call virtio_user_dev_uninit here.. Logically when virtio_user_fill_intr_handle fails, we want to release the resources which is allocated in virtio_user_dev_init_notify (i.e., fds), but virtio_user_dev_uninit does more. For example, unregister the event callback that have not been registered yet and it also leads to destroy callback called twice. Although things done but not needed will not lead to error, but it looks not in the best way. What do you think? Thanks, Chenbo > +destroy: > + dev->ops->destroy(dev); > + > + return -1; > } > > /* Use below macro to filter features from vhost backend */ > -- > 2.29.2
On 1/22/21 10:24 AM, Xia, Chenbo wrote: > Hi Maxime, > >> -----Original Message----- >> From: Maxime Coquelin <maxime.coquelin@redhat.com> >> Sent: Wednesday, January 20, 2021 5:25 AM >> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; olivier.matz@6wind.com; >> amorenoz@redhat.com; david.marchand@redhat.com >> Cc: Maxime Coquelin <maxime.coquelin@redhat.com> >> Subject: [PATCH v2 44/44] net/virtio: handle Virtio-user setup failure >> properly >> >> This patch fixes virtio_user_dev_setup() error path, >> by cleaning all resources it allocates. >> >> Suggested-by: Adrian Moreno <amorenoz@redhat.com> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >> --- >> drivers/net/virtio/virtio_user/virtio_user_dev.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c >> b/drivers/net/virtio/virtio_user/virtio_user_dev.c >> index a1e32158bb..2f7dc574b6 100644 >> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c >> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c >> @@ -427,15 +427,22 @@ virtio_user_dev_setup(struct virtio_user_dev *dev) >> >> if (virtio_user_dev_init_notify(dev) < 0) { >> PMD_INIT_LOG(ERR, "(%s) Failed to init notifiers\n", dev->path); >> - return -1; >> + goto destroy; >> } >> >> if (virtio_user_fill_intr_handle(dev) < 0) { >> PMD_INIT_LOG(ERR, "(%s) Failed to init interrupt handler\n", dev- >>> path); >> - return -1; >> + goto uninit; >> } >> >> return 0; >> + >> +uninit: >> + virtio_user_dev_uninit(dev); > > IMHO, it may not be the best choice to call virtio_user_dev_uninit here.. > > Logically when virtio_user_fill_intr_handle fails, we want to release the resources > which is allocated in virtio_user_dev_init_notify (i.e., fds), but virtio_user_dev_uninit > does more. For example, unregister the event callback that have not been registered yet and > it also leads to destroy callback called twice. Although things done but not needed will > not lead to error, but it looks not in the best way. What do you think? I agree, I'm reworking it for v3. Kick and call FDs will be initialized to -1 at virtio_user_dev_init() time. I introduce a virtio_user_dev_uninit_notify that will close all kick and call FDs is opened and reset their value to -1. Thenb I call this function in the error path of virtio_user_dev_setup() and also in virtio_user_dev_uninit(). Doing that, I can also simplifies virtio_user_dev_init_notify() and only loop for max queue pairs instead of VIRTIO_MAX_VIRTQUEUES and so avoid setting FDs to -1 a second time. Thanks, Maxime > Thanks, > Chenbo > >> +destroy: >> + dev->ops->destroy(dev); >> + >> + return -1; >> } >> >> /* Use below macro to filter features from vhost backend */ >> -- >> 2.29.2 >
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c index a1e32158bb..2f7dc574b6 100644 --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c @@ -427,15 +427,22 @@ virtio_user_dev_setup(struct virtio_user_dev *dev) if (virtio_user_dev_init_notify(dev) < 0) { PMD_INIT_LOG(ERR, "(%s) Failed to init notifiers\n", dev->path); - return -1; + goto destroy; } if (virtio_user_fill_intr_handle(dev) < 0) { PMD_INIT_LOG(ERR, "(%s) Failed to init interrupt handler\n", dev->path); - return -1; + goto uninit; } return 0; + +uninit: + virtio_user_dev_uninit(dev); +destroy: + dev->ops->destroy(dev); + + return -1; } /* Use below macro to filter features from vhost backend */
This patch fixes virtio_user_dev_setup() error path, by cleaning all resources it allocates. Suggested-by: Adrian Moreno <amorenoz@redhat.com> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- drivers/net/virtio/virtio_user/virtio_user_dev.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)