From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id D9A673B5 for ; Tue, 28 Mar 2017 05:40:18 +0200 (CEST) Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga105.fm.intel.com with ESMTP; 27 Mar 2017 20:40:17 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,235,1486454400"; d="scan'208";a="71149507" Received: from shwdeisgchi083.ccr.corp.intel.com (HELO [10.239.67.140]) ([10.239.67.140]) by orsmga004.jf.intel.com with ESMTP; 27 Mar 2017 20:40:16 -0700 To: Wenfeng Liu , yuanhan.liu@linux.intel.com, maxime.coquelin@redhat.com References: <1489246618-16898-1-git-send-email-liuwf@arraynetworks.com.cn> <004401d2a76f$cd209a00$6761ce00$@com.cn> Cc: dev@dpdk.org From: "Tan, Jianfeng" Message-ID: Date: Tue, 28 Mar 2017 11:40:15 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <004401d2a76f$cd209a00$6761ce00$@com.cn> Content-Type: text/plain; charset=gbk; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] =?utf-8?b?562U5aSNOiAgW1BBVENIXSBuZXQvdmlydGlvLXVzZXI6?= =?utf-8?q?_support_changing_tap_interface_name?= X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 28 Mar 2017 03:40:19 -0000 On 3/28/2017 11:02 AM, Wenfeng Liu wrote: > Hi Jianfeng, > > At 2017-03-28 10:05:11, "Tan, Jianfeng" wrote: >> Hi Wenfeng, >> >> Thank you for implementing this. >> >> On 3/11/2017 11:36 PM, Wenfeng Liu wrote: >>> This patch adds a new option 'iface' to change the interface name of >>> tap device with vhost-kernel as backend. >>> >>> Signed-off-by: Wenfeng Liu >>> --- >>> drivers/net/virtio/virtio_user/virtio_user_dev.c | 12 ++++++++---- >>> drivers/net/virtio/virtio_user/virtio_user_dev.h | 2 +- >>> drivers/net/virtio/virtio_user_ethdev.c | 24 > +++++++++++++++++++++--- >>> 3 files changed, 30 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c > b/drivers/net/virtio/virtio_user/virtio_user_dev.c >>> index 21ed00d..e7fd65f 100644 >>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c >>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c >>> @@ -193,9 +193,6 @@ int virtio_user_stop_device(struct virtio_user_dev > *dev) >>> for (i = 0; i < dev->max_queue_pairs; ++i) >>> dev->ops->enable_qp(dev, i, 0); >>> >>> - free(dev->ifname); >>> - dev->ifname = NULL; >> If we do not free it here, we need to free it at virtio_user_pmd_remove(). > Yes, I put the free in virtio_user_dev_uninit(), which will be called by > virtio_user_pmd_remove(). > >>> - >>> return 0; >>> } >>> >>> @@ -268,7 +265,7 @@ int virtio_user_stop_device(struct virtio_user_dev > *dev) >>> >>> int >>> virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int > queues, >>> - int cq, int queue_size, const char *mac) >>> + int cq, int queue_size, const char *mac, char **ifname) >>> { >>> snprintf(dev->path, PATH_MAX, "%s", path); >>> dev->max_queue_pairs = queues; >>> @@ -277,6 +274,11 @@ int virtio_user_stop_device(struct virtio_user_dev > *dev) >>> dev->mac_specified = 0; >>> parse_mac(dev, mac); >>> >>> + if (*ifname) { >>> + dev->ifname = *ifname; >>> + *ifname = NULL; >>> + } >>> + >>> if (virtio_user_dev_setup(dev) < 0) { >>> PMD_INIT_LOG(ERR, "backend set up fails"); >>> return -1; >>> @@ -327,6 +329,8 @@ int virtio_user_stop_device(struct virtio_user_dev > *dev) >>> free(dev->vhostfds); >>> free(dev->tapfds); >>> } >>> + >>> + free(dev->ifname); > Here is the free in virtio_user_dev_uninit(). Oops, I missed that. > >>> } >>> >>> static uint8_t >>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h > b/drivers/net/virtio/virtio_user/virtio_user_dev.h >>> index 0d39f40..6ecb91e 100644 >>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h >>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h >>> @@ -69,7 +69,7 @@ struct virtio_user_dev { >>> int virtio_user_start_device(struct virtio_user_dev *dev); >>> int virtio_user_stop_device(struct virtio_user_dev *dev); >>> int virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int > queues, >>> - int cq, int queue_size, const char *mac); >>> + int cq, int queue_size, const char *mac, char > **ifname); >>> void virtio_user_dev_uninit(struct virtio_user_dev *dev); >>> void virtio_user_handle_cq(struct virtio_user_dev *dev, uint16_t > queue_idx); >>> #endif >>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c > b/drivers/net/virtio/virtio_user_ethdev.c >>> index 0b226ac..16d1526 100644 >>> --- a/drivers/net/virtio/virtio_user_ethdev.c >>> +++ b/drivers/net/virtio/virtio_user_ethdev.c >>> @@ -243,6 +243,8 @@ >>> VIRTIO_USER_ARG_PATH, >>> #define VIRTIO_USER_ARG_QUEUE_SIZE "queue_size" >>> VIRTIO_USER_ARG_QUEUE_SIZE, >>> +#define VIRTIO_USER_ARG_INTERFACE_NAME "iface" >>> + VIRTIO_USER_ARG_INTERFACE_NAME, >>> NULL >>> }; >>> >>> @@ -259,6 +261,9 @@ >>> >>> *(char **)extra_args = strdup(value); >>> >>> + if (!*(char **)extra_args) >>> + return -ENOMEM; >>> + >>> return 0; >>> } >>> >>> @@ -347,6 +352,7 @@ >>> uint64_t cq = VIRTIO_USER_DEF_CQ_EN; >>> uint64_t queue_size = VIRTIO_USER_DEF_Q_SZ; >>> char *path = NULL; >>> + char *ifname = NULL; >>> char *mac_addr = NULL; >>> int ret = -1; >>> >>> @@ -375,6 +381,15 @@ >>> goto end; >>> } >>> >>> + if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_INTERFACE_NAME) == 1) { >>> + if (rte_kvargs_process(kvlist, > VIRTIO_USER_ARG_INTERFACE_NAME, >>> + &get_string_arg, &ifname) < 0) { >>> + PMD_INIT_LOG(ERR, "error to parse %s", >>> + VIRTIO_USER_ARG_INTERFACE_NAME); >>> + goto end; >>> + } >>> + } >>> + >>> if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_MAC) == 1) { >>> if (rte_kvargs_process(kvlist, VIRTIO_USER_ARG_MAC, >>> &get_string_arg, &mac_addr) < 0) { >>> @@ -413,7 +428,7 @@ >>> cq = 1; >>> } >>> >>> - if (queues > 1 && cq == 0) { >>> + if (queues > 1 && cq == VIRTIO_USER_DEF_CQ_EN) { >> Any specific reason for above change? What if we set >> VIRTIO_USER_DEF_CQ_EN=1 in future? I think zero is more clear. > Because cq was initialized to VIRTIO_USER_DEF_CQ_EN in this function, I > thought this comparison was intend to tell whether cq had been given a > different value, so I used > VIRTIO_USER_DEF_CQ_EN instead the magic number 0. Did I understand this > correctly? If not, I am happy to change it back. No, our purpose is not to check if cq is assigned a different value. We are checking if cq is enabled or not as mq requires cq. Besides that, it looks good to me. Reviewed-by: Jianfeng Tan