Page MenuHomeFreeBSD

sbin/ifconfig: Use libifconfig to get groups
ClosedPublic

Authored by freqlabs on Feb 27 2021, 8:26 AM.

Details

Summary

More code deduplication.

Test Plan
# ifconfig > /tmp/a
# git stash pop && make -C sbin/ifconfig && make -C sbin/ifconfig install
<snip>
# ifconfig > /tmp/b
# cmp /tmp/a /tmp/b && echo ok
ok

Diff Detail

Repository
R10 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

freqlabs created this revision.
freqlabs added a reviewer: kp.
sbin/ifconfig/ifgroup.c
89

Does that shadow the global name variable that we used to use as input?

I don't understand how this is intended to work.

93–95

Are we changing the input from the name global to the ifr.ifr_name global?

(We really need to clean up the use of globals in ifconfig, it's hard to follow for the small of brain, like me.)

103

That needs to be at the top of the block.

style(9) still has "Declarations may be placed before executable lines at the start of any block."

106

Does the kernel not set .ifgr_len to the correct size for the number of groups it returned?

If not, is that something we should have ifconfig_get_groups() clean up for us? It seems silly for all users of the lib to have to worry about this.

sbin/ifconfig/ifgroup.c
89

Yes I started to catch on to that after getting into a few more of these. I'll remove the local name variables so the global is used for now, and eventually it can be made a parameter to the function along with the handle.

sbin/ifconfig/ifgroup.c
106

Now that I think about it some more, this check is nonsense. The NULL check in the original code here didn't make sense. We were incrementing and checking a pointer we allocated and already checked for NULL ourselves. We're iterating through an array of structs, not an array of pointers.

I'll remove it.

freqlabs added inline comments.
sbin/ifconfig/ifgroup.c
93–95

I agree, I'm hoping to minimize the use of global variables in the long run.

This revision is now accepted and ready to land.Feb 28 2021, 3:59 PM