[dpdk-dev] [PATCH 3/3] net/virtio_user: fix dev not freed after init error
Hi Stephen,
> -Original Message-
> From: Stephen Hemminger [mailto:stephen at networkplumber.org]
> Sent: Saturday, August 6, 2016 12:34 AM
> To: Tan, Jianfeng
> Cc: dev at dpdk.org; yuanhan.liu at linux.intel.com; Wang, Zhihong;
> lining18 at jd.com
> Subject: Re: [dpdk-dev] [PATCH 3/3] net/virtio_user: fix dev not freed after
> init error
>
> On Fri, 5 Aug 2016 11:36:43 +
> Jianfeng Tan wrote:
>
> > diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> b/drivers/net/virtio/virtio_user_ethdev.c
> > index daef09b..62ccb0b 100644
> > --- a/drivers/net/virtio/virtio_user_ethdev.c
> > +++ b/drivers/net/virtio/virtio_user_ethdev.c
> > @@ -313,6 +313,17 @@ virtio_user_eth_dev_alloc(const char *name)
> > return eth_dev;
> > }
> >
> > +static void
> > +virtio_user_eth_dev_free(struct rte_eth_dev *eth_dev)
> > +{
> > + struct rte_eth_dev_data *data = eth_dev->data;
> > + struct virtio_hw *hw = data->dev_private;
> > +
> > + rte_free(hw->virtio_user_dev);
> > + rte_free(hw);
> > + rte_eth_dev_release_port(eth_dev);
> > +}
> > +
> > /* Dev initialization routine. Invoked once for each virtio vdev at
> > * EAL init time, see rte_eal_dev_init().
> > * Returns 0 on success.
> > @@ -328,7 +339,7 @@ virtio_user_pmd_devinit(const char *name, const
> char *params)
> > uint64_t queue_size = VIRTIO_USER_DEF_Q_SZ;
> > char *path = NULL;
> > char *mac_addr = NULL;
> > - int ret = -1;
> > + int result = -1, ret;
>
> It is not clear why two return value variables are needed?
My purpose was to make "ret" to record the return value of intermediate
functions, and "result" to record that of current method. Any convention to do
that?
If it introduces confusions, I'll change to use only one return value variables.
> >
> > if (!params || params[0] == '\0') {
> > PMD_INIT_LOG(ERR, "arg %s is mandatory for virtio_user",
> > @@ -411,15 +422,19 @@ virtio_user_pmd_devinit(const char *name,
> const char *params)
> >
> > hw = eth_dev->data->dev_private;
> > if (virtio_user_dev_init(hw->virtio_user_dev, path, queues, cq,
> > -queue_size, mac_addr) < 0)
> > +queue_size, mac_addr) < 0) {
> > + PMD_INIT_LOG(ERR, "virtio_user_dev_init fails");
> > + virtio_user_eth_dev_free(eth_dev);
> > goto end;
> > + }
> >
> > /* previously called by rte_eal_pci_probe() for physical dev */
> > if (eth_virtio_dev_init(eth_dev) < 0) {
> > PMD_INIT_LOG(ERR, "eth_virtio_dev_init fails");
> > + virtio_user_eth_dev_free(eth_dev);
> > goto end;
> > }
> > - ret = 0;
> > + result = 0;
> >
> > end:
> > if (kvlist)
> > @@ -428,7 +443,7 @@ end:
> > free(path);
> > if (mac_addr)
> > free(mac_addr);
>
> Unrelated, but this code could eliminate those if () tests.
Thanks for the hint. Yes, as manual says, "If ptr is NULL, no operation is
performed". "if"s can be eliminated.
Thanks,
Jianfeng
>
> > - return ret;
> > + return result;
> > }
[dpdk-dev] [PATCH 3/3] net/virtio_user: fix dev not freed after init error
Currently, when virtio_user device fails to be started (e.g., vhost
unix socket does not exit), the init function does not return struct
rte_eth_dev (and some other structs) back to ether layer. And what's
more, it does not report the error to upper layer.
The fix is to free those structs and report error when failing to
start virtio_user devices.
Fixes: ce2eabdd43ec ("net/virtio-user: add virtual device")
Signed-off-by: Jianfeng Tan
---
drivers/net/virtio/virtio_user_ethdev.c | 23 +++
1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/net/virtio/virtio_user_ethdev.c
b/drivers/net/virtio/virtio_user_ethdev.c
index daef09b..62ccb0b 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -313,6 +313,17 @@ virtio_user_eth_dev_alloc(const char *name)
return eth_dev;
}
+static void
+virtio_user_eth_dev_free(struct rte_eth_dev *eth_dev)
+{
+ struct rte_eth_dev_data *data = eth_dev->data;
+ struct virtio_hw *hw = data->dev_private;
+
+ rte_free(hw->virtio_user_dev);
+ rte_free(hw);
+ rte_eth_dev_release_port(eth_dev);
+}
+
/* Dev initialization routine. Invoked once for each virtio vdev at
* EAL init time, see rte_eal_dev_init().
* Returns 0 on success.
@@ -328,7 +339,7 @@ virtio_user_pmd_devinit(const char *name, const char
*params)
uint64_t queue_size = VIRTIO_USER_DEF_Q_SZ;
char *path = NULL;
char *mac_addr = NULL;
- int ret = -1;
+ int result = -1, ret;
if (!params || params[0] == '\0') {
PMD_INIT_LOG(ERR, "arg %s is mandatory for virtio_user",
@@ -411,15 +422,19 @@ virtio_user_pmd_devinit(const char *name, const char
*params)
hw = eth_dev->data->dev_private;
if (virtio_user_dev_init(hw->virtio_user_dev, path, queues, cq,
-queue_size, mac_addr) < 0)
+queue_size, mac_addr) < 0) {
+ PMD_INIT_LOG(ERR, "virtio_user_dev_init fails");
+ virtio_user_eth_dev_free(eth_dev);
goto end;
+ }
/* previously called by rte_eal_pci_probe() for physical dev */
if (eth_virtio_dev_init(eth_dev) < 0) {
PMD_INIT_LOG(ERR, "eth_virtio_dev_init fails");
+ virtio_user_eth_dev_free(eth_dev);
goto end;
}
- ret = 0;
+ result = 0;
end:
if (kvlist)
@@ -428,7 +443,7 @@ end:
free(path);
if (mac_addr)
free(mac_addr);
- return ret;
+ return result;
}
/** Called by rte_eth_dev_detach() */
--
2.7.4
[dpdk-dev] [PATCH 3/3] net/virtio_user: fix dev not freed after init error
On Fri, 5 Aug 2016 11:36:43 +
Jianfeng Tan wrote:
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> b/drivers/net/virtio/virtio_user_ethdev.c
> index daef09b..62ccb0b 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -313,6 +313,17 @@ virtio_user_eth_dev_alloc(const char *name)
> return eth_dev;
> }
>
> +static void
> +virtio_user_eth_dev_free(struct rte_eth_dev *eth_dev)
> +{
> + struct rte_eth_dev_data *data = eth_dev->data;
> + struct virtio_hw *hw = data->dev_private;
> +
> + rte_free(hw->virtio_user_dev);
> + rte_free(hw);
> + rte_eth_dev_release_port(eth_dev);
> +}
> +
> /* Dev initialization routine. Invoked once for each virtio vdev at
> * EAL init time, see rte_eal_dev_init().
> * Returns 0 on success.
> @@ -328,7 +339,7 @@ virtio_user_pmd_devinit(const char *name, const char
> *params)
> uint64_t queue_size = VIRTIO_USER_DEF_Q_SZ;
> char *path = NULL;
> char *mac_addr = NULL;
> - int ret = -1;
> + int result = -1, ret;
It is not clear why two return value variables are needed?
>
> if (!params || params[0] == '\0') {
> PMD_INIT_LOG(ERR, "arg %s is mandatory for virtio_user",
> @@ -411,15 +422,19 @@ virtio_user_pmd_devinit(const char *name, const char
> *params)
>
> hw = eth_dev->data->dev_private;
> if (virtio_user_dev_init(hw->virtio_user_dev, path, queues, cq,
> - queue_size, mac_addr) < 0)
> + queue_size, mac_addr) < 0) {
> + PMD_INIT_LOG(ERR, "virtio_user_dev_init fails");
> + virtio_user_eth_dev_free(eth_dev);
> goto end;
> + }
>
> /* previously called by rte_eal_pci_probe() for physical dev */
> if (eth_virtio_dev_init(eth_dev) < 0) {
> PMD_INIT_LOG(ERR, "eth_virtio_dev_init fails");
> + virtio_user_eth_dev_free(eth_dev);
> goto end;
> }
> - ret = 0;
> + result = 0;
>
> end:
> if (kvlist)
> @@ -428,7 +443,7 @@ end:
> free(path);
> if (mac_addr)
> free(mac_addr);
Unrelated, but this code could eliminate those if () tests.
> - return ret;
> + return result;
> }
