On 3/28/2017 11:02 AM, Wenfeng Liu wrote:
Hi Jianfeng,
At 2017-03-28 10:05:11, "Tan, Jianfeng" <jianfeng....@intel.com> 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 <li...@arraynetworks.com.cn>
---
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 <jianfeng....@intel.com>
