From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Hemminger Subject: Re: [PATCH 3/3] net/virtio_user: fix dev not freed after init error Date: Fri, 5 Aug 2016 09:34:13 -0700 Message-ID: <20160805093413.431ba323@xeon-e3> References: <1470397003-5782-1-git-send-email-jianfeng.tan@intel.com> <1470397003-5782-4-git-send-email-jianfeng.tan@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org, yuanhan.liu@linux.intel.com, zhihong.wang@intel.com, lining18@jd.com To: Jianfeng Tan Return-path: Received: from mail-pf0-f174.google.com (mail-pf0-f174.google.com [209.85.192.174]) by dpdk.org (Postfix) with ESMTP id 417232B9B for ; Fri, 5 Aug 2016 18:34:05 +0200 (CEST) Received: by mail-pf0-f174.google.com with SMTP id x72so98753288pfd.2 for ; Fri, 05 Aug 2016 09:34:05 -0700 (PDT) In-Reply-To: <1470397003-5782-4-git-send-email-jianfeng.tan@intel.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Fri, 5 Aug 2016 11:36:43 +0000 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; > }