* Alexey Suslikov <alexey.susli...@gmail.com> [2014-04-21 12:38]:
> Henning Brauer <lists-openbsdtech <at> bsws.de> writes:
>
> > > #if NVLAN > 0
> > > if (ifp->if_type == IFT_L2VLAN)
> > > return vlan_encap(ifp, m);
> > > #endif
> >
> > I don't think so, really. We're talking about 25 lines of code, in a
> > function that either prepends a regular ethernet header or a
> > vlan-ethernet header. Having both cases next to each other makes it
> > much more obvious what's going on imho.
>
> If I would be writing such a code, I wouldn't be breaking Single
> Responsibility Pattern and obfuscate ether_addheader with VLAN
> logic, but add small dedicated methods instead.
>
> Something like:
>
> ...
>
> #if NVLAN > 0
> //void ether_addvlanhdr1...
> //void ether_addvlanhdr2...
> #endif
>
> ...
>
> #if NVLAN > 0
> if (ifp->if_type == IFT_L2VLAN) {
> if ((p->if_capabilities & IFCAP_VLAN_HWTAGGING) &&
> (ifv->ifv_type == ETHERTYPE_VLAN)) {
> //ether_addvlanhdr1();
> } else {
> //ether_addvlanhdr2();
> return (0);
> }
> }
> #endif
congratulations, that is close to unauditable.
i put the vlan and the !vlan case next to each other ON PURPOSE. both
cases add an ethernet header, one with a few extra fields, one
without. Having that next to each other makes it f***ing obvious both
cases are identical, except the vlan case filling the 3 extra fields.
--
Henning Brauer, h...@bsws.de, henn...@openbsd.org
BS Web Services GmbH, http://bsws.de, Full-Service ISP
Secure Hosting, Mail and DNS Services. Dedicated Servers, Root to Fully Managed
Henning Brauer Consulting, http://henningbrauer.com/
