Hi Rusty,
On Tue, 2009-01-27 at 14:22 +1030, Rusty Russell wrote:
> > + struct virtnet_info *vi = netdev_priv(dev);
> > + u16 id = vid;
> > +
> > + if (virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
> > + VIRTIO_NET_CTRL_VLAN_ADD, &id, sizeof(id)))
> > + printk(KERN_WARNING "%s: Failed to add VLAN ID %d.\n",
> > + dev->name, id);
> > +}
>
> The temporary var seems strange here, and in kill_vid.
It does stand out a bit. I can't think of a valid reason for why I had
this, so I'll drop it.
> > - .ndo_poll_controller = virtnet_netpoll,
> > + .ndo_poll_controller = virtnet_netpoll,
>
> Pretty lining up is a pet peeve of mine. Just indent them one more and
> be happy :)
Noted ;^)
>
> > + /* Enable VLAN filtering if supported by the backend */
> > + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
> > + VIRTIO_NET_CTRL_VLAN_ENABLE,
> > + &vlan_filter, sizeof(vlan_filter))) {
> > + printk(KERN_DEBUG "virtio_net: VLAN filter enabled\n");
> > + dev->features |= NETIF_F_HW_VLAN_FILTER;
> > + }
>
> OK, several things here. Should probably be a feature bit. What is the
> vlan_filter arg for? Finally, using ! for the "success" case is YA API
> nastiness for me; if we were leaving virtnet_send_command as is I'd
> make it return bool, if it really returned a sensible -errno I'd
> compare against 0 here.
The VLAN_ENABLE command can either enable or disable the VLAN filter, so
the vlan_filter arg is setting enable to "on". A feature bit might be
more efficient, but since this happens at device init and the enable
will fail if it's not supported by the backend, it seemed redundant.
I agree the success/failure logic would be more clear w/o the !, I'll
switch this to a == 0, or something better depending on what we decide
to do with send_command. Thanks for the comments,
Alex
--
Alex Williamson HP Open Source & Linux Org.
--
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
