Re: [PATCH v3 2/2] virtio-net: introduce a new control to set macaddr

[Amos Kong <akong@redhat.com>]

On Fri, Jan 18, 2013 at 12:00:42PM +0100, Stefan Hajnoczi wrote:
> On Thu, Jan 17, 2013 at 06:40:12PM +0800, akong@redhat.com wrote:
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 395ab4f..837c978 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -802,14 +802,32 @@ static int virtnet_set_mac_address(struct net_device *dev, void *p)
> >  	struct virtnet_info *vi = netdev_priv(dev);
> >  	struct virtio_device *vdev = vi->vdev;
> >  	int ret;
> > +	struct scatterlist sg;
> > +	char save_addr[ETH_ALEN];
> > +	unsigned char save_aatype;
> > +
> > +	memcpy(save_addr, dev->dev_addr, ETH_ALEN);
> > +	save_aatype = dev->addr_assign_type;
> >  
> >  	ret = eth_mac_addr(dev, p);
> >  	if (ret)
> >  		return ret;
> >  
> > -	if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC))
> > +	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) {
> > +		sg_init_one(&sg, dev->dev_addr, dev->addr_len);
> > +		if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
> > +					  VIRTIO_NET_CTRL_MAC_ADDR_SET,
> > +					  &sg, 1, 0)) {
> > +			dev_warn(&vdev->dev,
> > +				 "Failed to set mac address by vq command.\n");
> > +			memcpy(dev->dev_addr, save_addr, ETH_ALEN);
> > +			dev->addr_assign_type = save_aatype;
> > +			return -EINVAL;
> > +		}
> 
> eth_mac_addr() doesn't allow callers to implement error handling nicely.
> Although you didn't duplicate it's code directly, this patch still leaks
> internals of eth_mac_addr().
> 
> How about splitting eth_mac_addr() in a separate patch:

Agree, then we can have nice error handling.
I will send a V4 and include this patch.
Thanks.
 
> int eth_prepare_mac_addr_change(struct net_device *dev, void *p)
> {
>         struct sockaddr *addr = p;
>         if (!(dev->priv_flags & IFF_LIVE_ADDR_CHANGE) && netif_running(dev))
>                 return -EBUSY;
>         if (!is_valid_ether_addr(addr->sa_data))
>                 return -EADDRNOTAVAIL;
> 	return 0;
> }

...

> Now virtio_net.c does:
> 
> ret = eth_prepare_mac_addr_change(dev, p);
> if (ret < 0)
> 	return ret;
> 
> if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) {
> 	sg_init_one(&sg, dev->dev_addr, dev->addr_len);

         trivial issue, %s/dev->dev_addr/addr->sa_data

> 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
> 				  VIRTIO_NET_CTRL_MAC_ADDR_SET,
> 				  &sg, 1, 0)) {
> 		dev_warn(&vdev->dev,
> 			 "Failed to set mac address by vq command.\n");
> 		return -EINVAL;
> 	}
> } ...
> 
> eth_commit_mac_addr_change(dev, p);
> return 0;
> 
> Stefan