ifnet: move the new ifnet_event EVENTHANDLER_DECLARE to net/if_var.h
ClosedPublic
Actions

Authored by decui_microsoft.com on Jan 26 2017, 5:22 AM.

Details

Reviewers

kmacy
kib
howard0su_gmail.com
honzhan_microsoft.com
ae
cem
jhb
hselasky
np
delphij
glebius
dab
sepherosa_gmail.com
gnn
royger
rwatson

Commits

rS312916: ifnet: move the new ifnet_event EVENTHANDLER_DECLARE to net/if_var.h

Summary

This is to fix
svn commit: r312687 (ifnet: introduce event handlers for ifup/ifdown events)

Thank glebius for pointing this out:
"The network stuff shall not be added to sys/eventhandler.h"

Diff Detail

Repository

rS FreeBSD src repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

decui_microsoft.com updated this revision to Diff 24465.Jan 26 2017, 5:22 AM

decui_microsoft.com retitled this revision from to ifnet: move the new ifnet_event EVENTHANDLER_DECLAREs to net/if_var.h.

decui_microsoft.com updated this object.

decui_microsoft.com edited the test plan for this revision. (Show Details)

decui_microsoft.com added reviewers: hselasky, sepherosa_gmail.com, cem, np, kmacy, kib, honzhan_microsoft.com, howard0su_gmail.com, jhb, ae, delphij, royger, glebius, gnn, rwatson.

decui_microsoft.com added a subscriber: freebsd-net-list.

decui_microsoft.com retitled this revision from ifnet: move the new ifnet_event EVENTHANDLER_DECLAREs to net/if_var.h to ifnet: move the new ifnet_event EVENTHANDLER_DECLARE to net/if_var.h.Jan 26 2017, 5:23 AM

Just a couple style nits.

sys/net/if_var.h
407 ↗	(On Diff #24465)	I would stick with the previous "ifup/ifdown" or go with "Interface up/down". I think "Interface up/ifdown" reads poorly.
408 ↗	(On Diff #24465)	It would be nice to have the 0/1 values for these two defines line up. In any case, it definitely seems like there is extraneous whitespace between "IFNET_EVENT_UP" and "0".

decui_microsoft.com added inline comments.Jan 26 2017, 2:14 PM

sys/net/if_var.h
407 ↗	(On Diff #24465)	I was trying to keep the consistency with Line 392, 395, 401 and 404. I would tend to leave the patch as it is, if you won't strongly object to it. :-)
408 ↗	(On Diff #24465)	Actually the 2 lines do line up. The visual confusion is due to the nasty Tab issue in the case of a patch. :-) With "set list" in my Vim, the lines of the file net/if_var.h (rather than the patch with a leading + at the beginning of the lines) shows #define IFNET_EVENT_UP^I^I0$ #define IFNET_EVENT_DOWN^I1$ ^I means a Tab. $ means a CR-LF.

dab accepted this revision.Jan 26 2017, 2:31 PM

dab added a reviewer: dab.

dab added inline comments.

sys/net/if_var.h
407 ↗	(On Diff #24465)	No objection to "Interface ..." but then I'd use "Interface up/down event" (the "if" in "ifdown" meaning "interface" is redundant). But, then, this is a nit. I don't strongly object to the patch as-is.
408 ↗	(On Diff #24465)	Hmmm, you are right; Phabricator is deceiving me. They don't line up at all in the diff display it shows me, but you are right that they do in code. Sorry about that. (Just a note that style(9) says to use just a single tab between name and value, which would mean they wouldn't line up. See similar situation at lines 185-186.)

This revision is now accepted and ready to land.Jan 26 2017, 2:31 PM

decui_microsoft.com added inline comments.Jan 26 2017, 3:59 PM

sys/net/if_var.h
407 ↗	(On Diff #24465)	Oh... Sorry, my bad! I failed to understand what you meant -- it's late at night here and obviously my eyes or brain didn't work very good :-)