On Fri, Jan 18, 2013 at 12:00:42PM +0100, Stefan Hajnoczi wrote:
> On Thu, Jan 17, 2013 at 06:40:12PM +0800, akong@xxxxxxxxxx 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
