Paths

Table of Contentst

ipfw: use typed pointers to access network protocols headers where possible
ClosedPublic
Actions

Authored by glebius on Fri, Sep 4, 11:10 PM.

Details

Reviewers

ae
lytboris_gmail.com

Group Reviewers

network

Commits

rG08f0a1bdc091: ipfw: use typed pointers to access network protocols headers where possible

Summary

The 'void *ulp' is still in action, but where possible prefer a typed
pointer. Get rid of associated pre-processor macros.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 76555
Build 73438: arc lint + arc unit

Event Timeline

glebius created this revision.Fri, Sep 4, 11:10 PM

Herald added subscribers: vegeta_tuxpowered.net, donner, melifaro, imp. · View Herald TranscriptFri, Sep 4, 11:10 PM

Harbormaster completed remote builds in B76555: Diff 185901.Fri, Sep 4, 11:10 PM

glebius requested review of this revision.Fri, Sep 4, 11:10 PM

glebius added a parent revision: D59426: ipfw: refactor macros around m_pullup().

Treat this as a suggestion. IMHO, this is more readable than the old version. If everybody disagrees with me, I'm fine with abandoning this.

The diff may not look great but I like the spelling out of types rather than hiding casts behind macros.

I was going to say I'd start the block under the case and not at the end of the line but it seems the more dominant style indeed is what you chose, so ignore this one.

lytboris_gmail.com added inline comments.Sun, Sep 6, 2:22 PM

sys/netpfil/ipfw/ip_fw2.c
1569	We have insntoc/insntod macros with similar goal, but for ipfw insn structure. Not sure if we need a stack variables scattered across the code instead of something similar to `ulptod(ulp, icmp6)`. Yep, this sounds much like reformatting `ICMP6(ulp)` et al. into a flexible form.

glebius added inline comments.Mon, Sep 7, 8:23 PM

sys/netpfil/ipfw/ip_fw2.c
1569	IMHO, all these macros that hide C basics are beneficial when you work with a single project for a long time and both your eyes and fingers have the muscle memory to immediately recognize them and type them. The downside is that the code is not very welcoming to a newcomer. The `struct ip`, `struct tcp`, `struct icmp6_hdr` and etc are well known to any network stack programmer and adoption is even wider than just FreeBSD. So, for a newcomer the suggested code is much more readable without jumping and checking wtf is `ICMP6()` or `ulptod()`. Even a returning co-author after years of working in different areas of the network stack will stumble upon this micro-obfuscations.

lytboris_gmail.com added inline comments.Tue, Sep 8, 10:41 AM

sys/netpfil/ipfw/ip_fw2.c
1569	A perfectly valid point indeed. But we're talking about hot path and on-stack variables may require an additional assignment in contrast to type casting.

glebius added inline comments.Wed, Sep 9, 8:24 AM

sys/netpfil/ipfw/ip_fw2.c
1569	Compiler will produce same code for both variants. Maybe only with -O0 could be different.

This revision was not accepted when it landed; it landed in state Needs Review.Fri, Sep 18, 10:50 PM

Closed by commit rG08f0a1bdc091: ipfw: use typed pointers to access network protocols headers where possible (authored by glebius). · Explain Why

This revision was automatically updated to reflect the committed changes.

glebius added a commit: rG08f0a1bdc091: ipfw: use typed pointers to access network protocols headers where possible.

Revision Contents
Changeset List

Path

Size

sys/

netpfil/

ipfw/

ip_fw2.c

225 lines

Diff 185901

View Options

ipfw: use typed pointers to access network protocols headers where possibleClosedPublicActions