Re: [PATCH] net/virtio-user: specify the MAC of the tap

[Tiwei Bie <tiwei.bie@intel.com>]

Hi Ning,

On Fri, Dec 29, 2017 at 11:38:42AM +0800, Ning Li wrote:
> When using virtio-user with vhost-kernel to exchange
> packet with kernel networking stack, application can
> set the MAC of the tap interface via parameter.
> 
> Signed-off-by: Ning Li <muziding001@163.com>
> ---

Thanks for the new version.

Just FYI, when sending a new version, you also need to
add the version number, e.g. using -v2 when generating
the patch. You can find more details in the "Contribute
by sending patches" section in below link:

http://dpdk.org/dev

>  drivers/net/virtio/virtio_user/vhost_kernel.c     |  3 ++-
>  drivers/net/virtio/virtio_user/vhost_kernel_tap.c | 14 +++++++++++++-
>  drivers/net/virtio/virtio_user/vhost_kernel_tap.h |  3 ++-
>  3 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c b/drivers/net/virtio/virtio_user/vhost_kernel.c
> index 68d28b1..dd24b6b 100644
> --- a/drivers/net/virtio/virtio_user/vhost_kernel.c
> +++ b/drivers/net/virtio/virtio_user/vhost_kernel.c
> @@ -380,7 +380,8 @@ struct vhost_memory_kernel {
>  	else
>  		hdr_size = sizeof(struct virtio_net_hdr);
>  
> -	tapfd = vhost_kernel_open_tap(&dev->ifname, hdr_size, req_mq);
> +	tapfd = vhost_kernel_open_tap(&dev->ifname, hdr_size, req_mq,
> +			 (char *)dev->mac_addr);

I think it's better to add a new device argument for
virtio-user to specify the MAC for the corresponding
tap. But I don't have a very strong opinion on this
for now. So I'd like to hear others' opinions.

>  	if (tapfd < 0) {
>  		PMD_DRV_LOG(ERR, "fail to open tap for vhost kernel");
>  		return -1;
> diff --git a/drivers/net/virtio/virtio_user/vhost_kernel_tap.c b/drivers/net/virtio/virtio_user/vhost_kernel_tap.c
> index 689a5cf..d036428 100644
> --- a/drivers/net/virtio/virtio_user/vhost_kernel_tap.c
> +++ b/drivers/net/virtio/virtio_user/vhost_kernel_tap.c
[...]
> +	memset(&ifr, 0, sizeof(ifr));
> +	ifr.ifr_hwaddr.sa_family = ARPHRD_ETHER;

Just to address others' potential doubts on this, as
I was confused about the macro used here after I saw
the macro used by tap PMD is AF_LOCAL. I checked the
tap related code in Linux, sa_family needs to be the
same with dev->type, otherwise it will return -EINVAL.
For tap, dev->type is initialized to ARPHRD_ETHER in
ether_setup().

Thanks,
Tiwei

> +	memcpy(ifr.ifr_hwaddr.sa_data, mac, ETHER_ADDR_LEN);
> +	if (ioctl(tapfd, SIOCSIFHWADDR, (void *)&ifr) == -1) {
> +		PMD_DRV_LOG(ERR, "SIOCSIFHWADDR failed: %s", strerror(errno));
> +		goto error;
> +	}
> +
>  	if (!(*p_ifname))
>  		*p_ifname = strdup(ifr.ifr_name);
>  
> diff --git a/drivers/net/virtio/virtio_user/vhost_kernel_tap.h b/drivers/net/virtio/virtio_user/vhost_kernel_tap.h
> index eae340c..402f964 100644
> --- a/drivers/net/virtio/virtio_user/vhost_kernel_tap.h
> +++ b/drivers/net/virtio/virtio_user/vhost_kernel_tap.h
> @@ -64,4 +64,5 @@
>  /* Constants */
>  #define PATH_NET_TUN	"/dev/net/tun"
>  
> -int vhost_kernel_open_tap(char **p_ifname, int hdr_size, int req_mq);
> +int vhost_kernel_open_tap(char **p_ifname, int hdr_size, int req_mq,
> +			 const char *mac);
> -- 
> 1.8.3.1
> 
>