Re: [dpdk-dev] [PATCH] net/virtio-user: specify MAC address for tap port

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

Hi Ning,

On Thu, Dec 14, 2017 at 07:38:14PM +0800, Ning Li wrote:
> When use virtio_user as exception path, we need to specify a MAC
> address for the tap port.

Is this a fix? Did you meet any issue? If so, please describe
the issue and add a fixline.

> 
> Signed-off-by: Ning Li <muziding001@163.com>
> ---
>  drivers/net/virtio/virtio_user/vhost_kernel.c     |  3 ++-
>  drivers/net/virtio/virtio_user/vhost_kernel_tap.c |  8 ++++++++
>  drivers/net/virtio/virtio_user/vhost_kernel_tap.h | 12 +++++++++++-
>  3 files changed, 21 insertions(+), 2 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);
>  	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..756fde2 100644
> --- a/drivers/net/virtio/virtio_user/vhost_kernel_tap.c
> +++ b/drivers/net/virtio/virtio_user/vhost_kernel_tap.c
> @@ -123,6 +123,14 @@

You forgot to add the mac param for vhost_kernel_open_tap().
The patch isn't buildable on my machine.

>  		PMD_DRV_LOG(ERR, "TUNSETOFFLOAD ioctl() failed: %s",
>  			   strerror(errno));
>  
> +	memset(&ifr, 0, sizeof(ifr));
> +	ifr.ifr_hwaddr.sa_family = ARPHRD_ETHER;

ARPHRD_ETHER? Please explain.

> +	memcpy(ifr.ifr_hwaddr.sa_data, mac, ETH_ALEN);

You can use ETHER_ADDR_LEN.
There is no need to define ETH_ALEN.

> +	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..f59b1ac 100644
> --- a/drivers/net/virtio/virtio_user/vhost_kernel_tap.h
> +++ b/drivers/net/virtio/virtio_user/vhost_kernel_tap.h
> @@ -64,4 +64,14 @@
>  /* Constants */
>  #define PATH_NET_TUN	"/dev/net/tun"
>  
> -int vhost_kernel_open_tap(char **p_ifname, int hdr_size, int req_mq);
> +/* Socket configuration controls. */
> +#define SIOCSIFHWADDR   0x8924          /* set hardware address */

There is no need to define this.

> +
> +/* ARP protocol HARDWARE identifiers. */
> +#define ARPHRD_ETHER    1               /* Ethernet             */
> +
> +/* Length of Ethernet address. */
> +#define ETH_ALEN  6
> +

There is no need to define this.

Thanks,
Tiwei

> +int vhost_kernel_open_tap(char **p_ifname, int hdr_size, int req_mq,
> +			 const char *mac);
> -- 
> 1.8.3.1
> 
>