0001343: arp ethers support - BusyBox Bug and Patch Tracking

(0002384)
vda
05-20-07 06:29

Indeed, "standard" arp has this option. However, I can't apply this patch as-is:

1. -f option is not made conditional (it should be, because this functionality is not essential for every user).

2. If I understand it right, /etc/ethers looks like:
00:00:4b:3e:45:e2 192.168.1.1
00:00:56:3e:36:1f 192.168.1.2
Your parsing code seems to be too complex for such a simple file format. If you handle more complex format, please document example(s) in source code comments.

3. arp_file() body is whitespace damaged; and please avoid assignments in ifs like 'if ((fp = fopen(name, "r"))'

(0002386)
ralf
05-20-07 10:35

vda: i was using an older busybox, now i have the current svn patch

1. True, even if the amount of code is so little...
2. yes, the syntax is like: 00:00:4b:3e:45:e2 192.168.1.1 but wh have to handle "" and \ as the ip could be an hostname. btw, the code was taken from standard net-tools
3: fixed

(0002387)
ralf
05-20-07 10:40

sorry, removed 'int arp_main(int argc, char **argv);' by mistake, fixed in ethers3.diff
Could someone delete older patches? tnx

(0002405)
vda
05-26-07 21:17

Sorry. Still far, far too big. Just one example:

+ if (*ptr != '\0') {
+ while (*ptr == ' ' || *ptr == '\t')
+ ptr++;
+ }

Do you see that if() is completetely useless here and can be removed, leaving only while()?

My suggestion:

+static int arp_file(char *name)
+{
+ char buff[1024];
+ char *sp, *args[32];
+ int linenr, argc;
+ FILE *fp = fopen(name, "r");
+ if(!fp) {
+ bb_error_msg_and_die("arp: cannot open etherfile %s !", name);
+ return (-1);
+ }

Use xfopen.

+ /* Read the lines in the file. */
+ linenr = 0;
+ while(fgets(buff, sizeof(buff), fp) != (char *)NULL) {

Don't use big fixed buffer. Use xmalloc_getline instead.
(Will nicely eliminate the need in "if((sp = strchr(buff, '\n'))..." too)

+ argc = getargs(buff, args);
+ if(argc < 2) {
+ bb_error_msg("arp: format error on line %u of etherfile %s !", linenr, name);
+ continue;
+ }

Completely rewrite getargs(), making it die (with msg) on errors, and handling only [<whitespace>]ether<whitespace>IP_or_hostname[<whitespace>] syntax. This should be simple - and SMALL.

Thus here you won't need argc. You will *know* that there are two arguments.

> btw, the code was taken from standard net-tools

This cannot be used as justification for inclusion. Sorry.

Issue History
Date Modified	Username	Field	Change
05-11-07 07:22	ralf	New Issue
05-11-07 07:22	ralf	Status	new => assigned
05-11-07 07:22	ralf	Assigned To	=> BusyBox
05-11-07 07:22	ralf	File Added: ethers.diff
05-11-07 07:26	ralf	Issue Monitored: ralf
05-20-07 06:29	vda	Note Added: 0002384
05-20-07 10:35	ralf	Note Added: 0002386
05-20-07 10:36	ralf	File Added: ethers2.diff
05-20-07 10:39	ralf	File Added: ethers3.diff
05-20-07 10:40	ralf	Note Added: 0002387
05-26-07 21:17	vda	Note Added: 0002405

Notes
(0002384) vda 05-20-07 06:29	Indeed, "standard" arp has this option. However, I can't apply this patch as-is: 1. -f option is not made conditional (it should be, because this functionality is not essential for every user). 2. If I understand it right, /etc/ethers looks like: 00:00:4b:3e:45:e2 192.168.1.1 00:00:56:3e:36:1f 192.168.1.2 Your parsing code seems to be too complex for such a simple file format. If you handle more complex format, please document example(s) in source code comments. 3. arp_file() body is whitespace damaged; and please avoid assignments in ifs like 'if ((fp = fopen(name, "r"))'

(0002386) ralf 05-20-07 10:35	vda: i was using an older busybox, now i have the current svn patch 1. True, even if the amount of code is so little... 2. yes, the syntax is like: 00:00:4b:3e:45:e2 192.168.1.1 but wh have to handle "" and \ as the ip could be an hostname. btw, the code was taken from standard net-tools 3: fixed

(0002387) ralf 05-20-07 10:40	sorry, removed 'int arp_main(int argc, char **argv);' by mistake, fixed in ethers3.diff Could someone delete older patches? tnx

(0002405) vda 05-26-07 21:17	Sorry. Still far, far too big. Just one example: + if (ptr != '\0') { + while (ptr == ' ' \|\| ptr == '\t') + ptr++; + } Do you see that if() is completetely useless here and can be removed, leaving only while()? My suggestion: +static int arp_file(char name) +{ + char buff[1024]; + char sp, args[32]; + int linenr, argc; + FILE fp = fopen(name, "r"); + if(!fp) { + bb_error_msg_and_die("arp: cannot open etherfile %s !", name); + return (-1); + } Use xfopen. + / Read the lines in the file. / + linenr = 0; + while(fgets(buff, sizeof(buff), fp) != (char )NULL) { Don't use big fixed buffer. Use xmalloc_getline instead. (Will nicely eliminate the need in "if((sp = strchr(buff, '\n'))..." too) + argc = getargs(buff, args); + if(argc < 2) { + bb_error_msg("arp: format error on line %u of etherfile %s !", linenr, name); + continue; + } Completely rewrite getargs(), making it die (with msg) on errors, and handling only [<whitespace>]ether<whitespace>IP_or_hostname[<whitespace>] syntax. This should be simple - and SMALL. Thus here you won't need argc. You will know that there are two arguments. > btw, the code was taken from standard net-tools This cannot be used as justification for inclusion. Sorry.

Relationships