Bigger one than the last two:
http://landley.net/hg/toybox/rev/866
In this round, we delete the standalone global iface_list_head and add a
GLOBALS() block with void *iface_list. (We have a pointer to a list,
saying we have a pointer to a list_head is silly. The list head is just
the first node in the list...)
Combine "typedef struct _proc_net_dev_info {...}
WHY_IS_THIS_CAPITALIZED" and "typedef struct _iface_list {...}
CAPITAL_LETTERS_MEAN_MACRO" into a single "struct iface_list" which is
much shorter and not a typedef, plus avoids an extra layer of
indirection with nested structures so you can say "il->transmit_fifo"
instead of "l_ptr->dev_info.transmit_fifo". (Do that a dozen times in
the same function's arguments, it adds up.) The name of the new struct
comes from the old iface_list_head, that went away but we're keeping the
name for the struct it was a list of.
Retain the comment that the netdevice man page has lots of relevant info
on this stuff.
We're inlining or replacing a bunch of stuff this time around. The old
symbols that went away are field_format, iface_flags_str,
omit_whitespace(), print_iface_flags(), print_media(), and clear_list().
I'll mention each again at the point it's was called.
Replace clear_list() with llist_traverse(free). Collate two similar
hex_to_binary() calls (with identical error messages) into a single call
using ? : operator for the single differing argument; no point
duplicating the function call and marshaling those other arguments twice.
(An aside on rough cost estimates: a function call takes roughly
sizeof(long), and marshalling an argument ballparks to sizeof(the
argument). The ? : isn't without cost but we've already _got_ the test
so moving the test into the parameter instead of around the function
call is a wash. If I had to guess I'd say a ? : is maybe as expensive as
a function call with no arguments or return value, but all this stuff
varies per target and compiler version and so on. This is just so I have
some vague idea what's more or less expensive. Mostly, it's not having
the code say the same thing twice. Maybe some optimizers are smart
enough to collate it themselves these days, but I still don't like
redundant code.)
The get_proc_info() version field: the version field is for dealing with
data from different /proc/net/dev versions. How old are these versions,
has any of them been exported since 3.x came out? Let's see:
/proc/net/dev is exported from linux/net/core/net-procfs.c where the
relevant functions are dev_seq_show() (printing the header) and
dev_seq_printf_stats() (printing the numbers). According to git
annotate, that recently (feb 2013) moved from net/core/dev.c (commit
900ff8c63214) so we use the old "git annotate ${VERSION}^1 -- filename"
trick, and _that_ says the part of dev_seq_show() that's printing out
the header hasn't changed since 2003 (so an old version would be 2.4
kernel). So we may be calculating the contents differently, but we
haven't relabeled the fields so we shouldn't have to parse them
differently. Let's confirm though, dev_seq_printf_stats() has several
commits since then. Let's see, 28172739f0a2 was "fix 64 bit counters on
32 bit arches", not a format change. Annotate before _that_, and commit
be1f3c2c027c was "64 bit statistics on 32 bit architectures",
2d13bafeba24 is potentially relevant "ensure there is always a space
between : and the first number" (but we do a strsep on the : so aren't
confused if there isn't), eeda3fd64f75 is "replace dev->stats() with
get_dev_stats()" (different function, same data), 5a1b5898ee9e is just
reindenting this block...
And at that point, the actual printf block dates back to 2002, so again
before 2.6 shipped. So we only care about the most recent version.
Back to get_proc_info() in ifconfig.c: we zap the version argument, and
it's using the new struct iface_list. Zap the memset because the only
caller did an xzalloc() so we know it's zeroed. Inline
omit_whitespace(). Tidy up a block using iface_list and combine _4_
lines of curly brackets into a single "} else thingy;"
Now that version's gone, field_format[] doesn't need to be an array. It
never needed to be a global. It can just be a normal sscanf() inline
format string. And we can collapse the 17 line sscanf() into a 7 line
sscanf(), and eliminate the if(version) after it because the above
exercise proved we should never have to care on a kernel shipped in the
last 10 years.
Convert add_iface_to_list() to struct iface_list, using the TT instance
instead of the standalone global. Same for get_device_info(), no other
changes there.
In get_device_info we have #ifdef SIOCGIFMAP and we go back to the
kernel to see how long that's been there: it's
include/uapi/linux/sockios.h which I'm not even going to annotate but
just log to see when that was moved into uapi... commit 607ca46e97a1, an
using the _big_ linux git tree that goes all the way back to 0.0.1, the
line #definining SIOCGIFMAP is from 1994. So we can safely yank the #ifdef.
get_ifconfig_info(): pretty much a complete rewrite of this function. We
don't need to do version detection, so just make it a single loop
reading the file, skipping the first two lines of header, running each
of the rest through get_proc_info(), add_iface_to_list(), setting it
non-virtual, and calling get-device_info().
print_hw_addr() and print_ip_addr() just convert to struct iface_list,
no other changes.
print_media(): just delete it and remove the call from
display_ifconfig(). The function exists to identify the type of ethernet
cable connection plugged into the card, but the table (ahem, if/else
staircase) only has entries for 10baseT and 100baseT. Does anybody still
care about distinguishing between 10base2 (coax) and 10baseT (cat5)? Or
about whether or not your 100baseT connection is T or TX or FX? Can
anybody tell me what that difference _means_? The man page can't:
media type
Set the physical port or medium type to be used by the device.
Not all devices can change this setting, and those that can vary
in what values they support. Typical values for type are
10base2 (thin Ethernet), 10baseT (twisted-pair 10Mbps Ethernet),
AUI (external transceiver) and so on. The special medium type
of auto can be used to tell the driver to auto-sense the media.
Again, not all drivers can do this.
Note we're not _setting_ it, we're just reading it. I dug around in the
kernel source a bit (since these values are defined in
linux/netdevice.h) and found drivers/net/ethernet/sis/sis900.c going:
/* we switch on the ifmap->port field. I couldn't find anything
* like a definition or standard for the values of that field.
* I think the meaning of those values is device specific. But
* since I would like to change the media type via the ifconfig
* command I use the definition from linux/netdevice.h
* (which seems to be different from the ifport(pcmcia) definition) */
Git annotate says that comment's from 2001, so even _then_ this was
obsolete (overtaken by "auto"). So I'm yanking the function, which never
applied to wireless or gigabit interfaces anyway. (I could have redone
it as a small inline string array, but it doesn't seem worth keeping.)
print_iv6_addr(): convert to struct iface_list, replace stack buf[] with
toybuf, and some curly bracket/whitespace cleanups. Remove a memset
inet_ntop doesn't care about, combine two xprintf() calls into one, and
replace an xprintf() with an xputc().
display_ifconfig(): Convert to struct iface_list. We don't need a local
variable copy of int hw_type now that getting it out of the structure is
less of a pain. Remove the call to print_media because we deleted that
function as useless.
Inline print_iface_flags(), which also sucks in the global
iface_flags_str[]. Instead of calling the function, locally test
ifrflags, declare a string array inside the if {} block, and loop
through it here. This means we can delete print_iface_flags() and
iface_flags_str[] because that was the only user.
The rest of this function looks more changed than it is, it's all just
these changes repeated a lot:
1) replace lptr->dev_info. with il->
2) replace %10s with %10c (and change its argument from " " to ' ')
3) replace local hw_type with il->hw_type.
4) combine consecutive xprintf() calls into a single xprintf() call.
5) whitespace (remove newline after if() when it'll fit on one line)
The %s to %c padding change means we don't need an empty string
constant, marshalling int instead of pointer is slightly cheaper on 64
bit platforms, and it saves a dereference. (A rounding error "while I
was there" cleanup.)
readconf(): The bit with the "Escape duplicate values" is a classic loop
simplification: we don't need a match_found variable if our iterator
variable will be null if it hit the end of the list and won't if it
broke out early. So as long as I had to convert IFACE_LIST to struct
iface_list anyway, I went ahead and turned:
IFACE_LIST *list_ptr;
int match_found = 0;
for(list_ptr = iface_list_head; list_ptr != NULL; list_ptr =
list_ptr->next) {
//if interface already in the list then donot add it in the list.
if(!strcmp(ifre->ifr_name, list_ptr->dev_info.ifrname)) {
match_found = 1;
break;
}
}
if(!match_found) {
into:
struct iface_list *il;
for(il = TT.iface_list; il; il = il->next)
if(!strcmp(ifre->ifr_name, il->ifrname)) break;
if(!il) {
10 lines into 4 lines, and one of those 4 is blank. Yes I removed a
comment, but this whole thing is right after the "Escape duplicate
values" comment.
show_iface(): more IFACE_LIST -> struct iface_list conversion, and
another loop that doesn't need is_dev_found when if (!il) tells us
whether or not we hit the end.
The "error getting device info" message just becomes a perror_msg() with
the interface name and the errno string. "Interface" had "problem". Yes,
maybe ENODEV won't have quite the error string we want, but it'll be
slightly wrong in the correctly internationalized language.
Moving struct iface_list up a block level means it can replace two local
IFACE_LIST variables. Their scopes don't overlap, so they can reuse the
same variable safely.
And at the end of the patch, clear_list() went away as mentioned earlier.
_______________________________________________
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net
