On Friday 30 January 2009 09:35:12 Alex Williamson wrote:
> +static void virtnet_set_rx_mode(struct net_device *dev)
> +{
> + struct virtnet_info *vi = netdev_priv(dev);
> + u8 promisc, allmulti;
> +
> + if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX))
> + return;
Hmm, this check duplicates the one in virtnet_send_command; after
all your patches, is it ever called with !VIRTIO_NET_F_CTRL_RX? Maybe it should be a BUG_ON in there?
> + promisc = ((dev->flags & IFF_PROMISC) != 0 || dev->uc_count > 0);
> + allmulti = ((dev->flags & IFF_ALLMULTI) != 0 || dev->mc_count > 0);
> +
> + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
> + VIRTIO_NET_CTRL_RX_PROMISC,
> + &promisc, sizeof(promisc)))
> + printk(KERN_WARNING "%s: Failed to %sable promisc mode.\n",
> + dev->name, promisc ? "en" : "dis");
> +
> + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
> + VIRTIO_NET_CTRL_RX_ALLMULTI,
> + &allmulti, sizeof(allmulti)))
> + printk(KERN_WARNING "%s: Failed to %sable allmulti mode.\n",
> + dev->name, allmulti ? "en" : "dis");
> +}
Hmm, we can't do anything with this error. I'd be very tempted to define the API to say "this can't fail". Leave this code in as a sanity check, but have a comment to that effect?
> +#define VIRTIO_NET_CTRL_RX 0
> + #define VIRTIO_NET_CTRL_RX_PROMISC 0
> + #define VIRTIO_NET_CTRL_RX_ALLMULTI 1
Comment above/beside these two perhaps?
/* Supported if VIRTIO_NET_F_CTRL_RX */
Cheers,
Rusty.
--
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
