netdev - Re: [PATCH 5/5] virtio_net: Add support for VLAN filtering in the hypervisor

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-Id: <1233029992.7148.82.camel@2710p.home>
Date:	Mon, 26 Jan 2009 21:19:52 -0700
From:	Alex Williamson <alex.williamson@...com>
To:	Rusty Russell <rusty@...tcorp.com.au>
Cc:	netdev@...r.kernel.org, markmc@...hat.com, kvm@...r.kernel.org
Subject: Re: [PATCH 5/5] virtio_net: Add support for VLAN filtering in the
 hypervisor

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 netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html