[BusyBox 0001130]: arp applet

bugs at busybox.net bugs at busybox.net
Sat Jan 6 22:18:03 UTC 2007


A NOTE has been added to this issue. 
====================================================================== 
http://busybox.net/bugs/view.php?id=1130 
====================================================================== 
Reported By:                espakman
Assigned To:                BusyBox
====================================================================== 
Project:                    BusyBox
Issue ID:                   1130
Category:                   New Features
Reproducibility:            N/A
Severity:                   feature
Priority:                   normal
Status:                     assigned
====================================================================== 
Date Submitted:             12-23-2006 12:35 PST
Last Modified:              01-06-2007 14:18 PST
====================================================================== 
Summary:                    arp applet
Description: 
Busybox currently doesn't contain an arp applet. Someone wrote an arp
applet a long time ago. I rediffed this version against bb 1.3.0 and it
applies cleanly against current SVN.
====================================================================== 

---------------------------------------------------------------------- 
 vda - 12-23-06 23:44  
---------------------------------------------------------------------- 
Unused #define:

+
+#define APPLET_NAME "arp"
+

Inconsistent style: 3-space indent AND 4-space indent in the same file:

+    (void) fclose(fp);
+
+    return 0;
+}
+
+int arp_main(int argc, char **argv)
+{
+   int what, i;
+
+   /* Initialize variables... */
+   if ((hw = get_hwtype(DFLT_HW)) == NULL) {
+      bb_error_msg("%s: hardware type not supported!\n", DFLT_HW);
+      return -1;
+   }

Please reformat to bbox style (indents by 1 tab).

 

---------------------------------------------------------------------- 
 espakman - 12-24-06 09:07  
---------------------------------------------------------------------- 
Second try, new patch attached. 

---------------------------------------------------------------------- 
 espakman - 12-24-06 10:42  
---------------------------------------------------------------------- 
Now run through indent, to be fully bbox style complient. 

---------------------------------------------------------------------- 
 vda - 12-25-06 20:10  
---------------------------------------------------------------------- 
Reviewed the patch.

+/* This structure defines protocol families and their handlers. */
+struct aftype {
+        char *name;
+        char *title;
+        int af;
+        int alen;

Use tabs for indentation.

+#define arp_trivial_usage \
+        "[-evn] [-H type] [-i if] -a [hostname]\n" \
+        "\tarp [-v] [-i if] -d hostname [pub]\n" \
+        "\tarp [-v] [-H type] [-i if] -s hostname hw_addr [temp]\n" \

Follow usage.h rules which are written in the comment on top of that
file.

+#include <sys/ioctl.h>
+#include <sys/signal.h>
+#include <sys/time.h>
+
+#include <errno.h>
+#include <netdb.h>
+#include <stdlib.h>
+#include <string.h>
+#include <ctype.h>
+#include <unistd.h>
+
+#include <arpa/inet.h>
+#include <net/if.h>
+#include <net/if_arp.h>
+#include <netinet/ether.h>
+#include <netpacket/packet.h>
+
+#include "busybox.h"

Most of these standard includes are already included by #include
"busybox.h"

+extern struct aftype *get_aftype(const char *name);
+extern struct hwtype *get_hwntype(int type);
+extern struct hwtype *get_hwtype(const char *name);
+
+int opt_n = 0;
+int opt_v = 0;

These functions and variables should be static.

+               /* Skip trailing whitespace. */
+               if (*ptr != '\0') {
+                       while (*ptr == ' ' || *ptr == '\t')
+                               ptr++;
+               }

We have skip_whitespace, use that.

+       memcpy((char *) &req.arp_pa, (char *) &sa, sizeof(struct
sockaddr));

Casts are superfluous here.

+                       bb_error_msg("args=%s\n", *args);

bb_[ph]error_msgXXX print trailing '\n' by themself.

+               perror("SIOCSARP");

use bb_perror_msg

+                       ("Address                  HWtype  HWaddress      
    Flags Mask            Iface\n");

Maybe use tabs?

+static int arp_show(char *name)
+{
+       char host[100];
+       struct sockaddr sa;
+       char ip[100];
+       char hwa[100];
+       char mask[100];
+       char line[200];
+       char dev[100];
+       int type, flags;
+       FILE *fp;
+       char *hostname;
+       int num, entries = 0, showed = 0;
+
....
+       /* Bypass header -- read until newline */
+       if (fgets(line, sizeof(line), fp) != (char *) NULL) {

use xmalloc_fget{s,line} instead of fixed-sized buffers. It saves memory.

+       (void) fclose(fp);

Don't think we need this cast

+                               bb_error_msg("%s: unknown address
family.\n", optarg);

Trailing period (and '\n') is not needed 

---------------------------------------------------------------------- 
 espakman - 12-26-06 05:02  
---------------------------------------------------------------------- 
> +/* This structure defines protocol families and their handlers. */
> +struct aftype {
> + char *name;
> + char *title;
> + int af;
> + int alen;
>
> Use tabs for indentation.

Fixed

> +#define arp_trivial_usage \
> + "[-evn] [-H type] [-i if] -a [hostname]\n" \
> + "\tarp [-v] [-i if] -d hostname [pub]\n" \
> + "\tarp [-v] [-H type] [-i if] -s hostname hw_addr [temp]\n" \
> 
> Follow usage.h rules which are written in the comment on top of that
file.

Fixed

> +#include <sys/ioctl.h>
> +#include <sys/signal.h>
> ....
> +
> +#include "busybox.h"
> 
> Most of these standard includes are already included by #include
"busybox.h"

Fixed

> +extern struct aftype *get_aftype(const char *name);
> +extern struct hwtype *get_hwntype(int type);
> +extern struct hwtype *get_hwtype(const char *name);
> +
> +int opt_n = 0;
> +int opt_v = 0;
> 
> These functions and variables should be static.
>
> + /* Skip trailing whitespace. */
> + if (*ptr != '\0') {
> + while (*ptr == ' ' || *ptr == '\t')
> + ptr++;
> + }
> 
> We have skip_whitespace, use that.
> 
> + memcpy((char *) &req.arp_pa, (char *) &sa, sizeof(struct sockaddr));
> 
> Casts are superfluous here.

I'm more of a user than a programmer, this goes beyond my capabilities to
change or comment on...

> + bb_error_msg("args=%s\n", *args);
> 
> bb_[ph]error_msgXXX print trailing '\n' by themself.
>
> + perror("SIOCSARP");
> 
> use bb_perror_msg

Fixed

> + ("Address HWtype HWaddress Flags Mask Iface\n");
> 
> Maybe use tabs?

That breaks formatting unfortuanatly 

> +static int arp_show(char *name)
> +{
> + char host[100];
> + struct sockaddr sa;
> + char ip[100];
> + char hwa[100];
> + char mask[100];
> + char line[200];
> + char dev[100];
> + int type, flags;
> + FILE *fp;
> + char *hostname;
> + int num, entries = 0, showed = 0;
> +
> ....
> + /* Bypass header -- read until newline */
> + if (fgets(line, sizeof(line), fp) != (char *) NULL) {
> 
> use xmalloc_fget{s,line} instead of fixed-sized buffers. It saves
memory.

This also goes beyond my capabilities, I need a bit of help on this one
too.

> + (void) fclose(fp);
> 
> Don't think we need this cast

Removed both instances

> + bb_error_msg("%s: unknown address family.\n", optarg);
> 
> Trailing period (and '\n') is not needed

Fixed 

---------------------------------------------------------------------- 
 espakman - 12-26-06 06:55  
---------------------------------------------------------------------- 
A few more fixes:

> + memcpy((char *) &req.arp_pa, (char *) &sa, sizeof(struct sockaddr));
>
> Casts are superfluous here.

Fixed on all instances 

---------------------------------------------------------------------- 
 vda - 12-26-06 10:05  
---------------------------------------------------------------------- 
Thanks for your fixes. I feel it still is a bit rough on edges, so

svn-arp5 is arp5 diff rediffed against current svn.
svn-arp6 is next revision of it with my changes.
arp5-arp6 is diff between arp5 and arp6.

Review notes (examine arp5-arp6.patch to see):

Never place extern decls in .c file. You had a bug there (functions were
returning const pointers but your extern decl in arp.c did not have
const).

Use static for local data/functions: _static_ int opt_n;

getargs() is too big (use skip_whitespace() to trim it down) and I think
it has bug in handling of \", \' - not fixed in arp6.

Many superfluous casts.


-               if (!(xhw = get_hwntype(ifr.ifr_hwaddr.sa_family))
-                       || (xhw->print == 0)) {
+               xhw = get_hwntype(ifr.ifr_hwaddr.sa_family);
+               if (!xhw || !xhw->print) {

Do not use assignment-inside-if - it is hard to read (for no gain in code
size at all).


while ((i = getopt(argc, argv, "A:H:adfp:nsei:t:vh?DNV")) != EOF)

Replace with getopt32. There are many examples in the tree (say,
od_bloaty.c)


-               if ((hw = get_hwtype(DFLT_HW)) == NULL) {
-                       bb_error_msg("%s: hardware type not supported!",
DFLT_HW);
-                       return (-1);
-               }
+               hw = get_hwtype(DFLT_HW);
+               if (!hw)
+                       bb_error_msg_and_die("%s: hardware type not
supported", DFLT_HW);

Die on errors, do not bother returning/closing files/freeing memory if
error is fatal anyway. 

---------------------------------------------------------------------- 
 espakman - 12-27-06 14:05  
---------------------------------------------------------------------- 
Thanks for your fixes and comments.

I decided to remove the /etc/ethers handling all together for a few
reasons:
both getargs() and arp_file() add considerable size.
I think that most users, if any, won't use or need /etc/ethers with arp
anyway.
It doesn't seem to work reliable, at least not in my tests.

The arp.c file still needs getopt -> getopt32, but as non-programmer it
will cost me a bit more time...

New patch (against latest SVN) attached 

---------------------------------------------------------------------- 
 espakman - 01-04-07 13:23  
---------------------------------------------------------------------- 
With the help of a friend getopt32 is implemented. Also some more
cleanups.
New patch (busybox-arp8.patch) attached. 

---------------------------------------------------------------------- 
 bernhardf - 01-04-07 14:36  
---------------------------------------------------------------------- 
It's still way to huge, imo.

$ size arp.o.08a arp.o.09b 
   text	   data	    bss	    dec	    hex	filename
   3940	      0	     48	   3988	    f94	arp.o.08a
   3876	      0	     36	   3912	    f48	arp.o.09b 

---------------------------------------------------------------------- 
 bernhardf - 01-04-07 14:41  
---------------------------------------------------------------------- 
wth are all these -v stuff ment to do in there?
Please explain.. 

---------------------------------------------------------------------- 
 bernhardf - 01-04-07 14:45  
---------------------------------------------------------------------- 
$ diff -u arp.c.09b arp.c
--- arp.c.09b	2007-01-04 23:35:02.000000000 +0100
+++ arp.c	2007-01-04 23:44:56.000000000 +0100
@@ -36,7 +36,11 @@
 #define	ARP_OPT_i (0x10)
 #define	ARP_OPT_a (0x20)
 #define	ARP_OPT_d (0x40)
+#if ENABLE_DEBUG
 #define	ARP_OPT_v (0x80)	/* debugging output flag        */
+#else
+#define	ARP_OPT_v (0)	/* debugging output flag        */
+#endif
 #define	ARP_OPT_n (0x100)	/* do not resolve addresses     */
 #define	ARP_OPT_D (0x200)	/* HW-address is devicename     */
 #define	ARP_OPT_s (0x400)
$ size arp.o.09b arp.o
   text	   data	    bss	    dec	    hex	filename
   3876	      0	     36	   3912	    f48	arp.o.09b
   3524	      0	     36	   3560	    de8	arp.o


So this wastes 350 bytes to achieve what? ;) 

---------------------------------------------------------------------- 
 bernhardf - 01-04-07 15:04  
---------------------------------------------------------------------- 
TODOs:
1) use compare_string_array().
see e.g. ip for examples.
candidates for compare_string_array:

arp_del()
arp_set()
arp_disp()

2) host[128]
has to die. See RESERVE_CONFIG_BUFFER

3) if (ap->input(0, host, &sa) < 0) {
is used way too often and sounds like it is (or should be) in libbb in one
form or another.

etc, etc. 

---------------------------------------------------------------------- 
 vda - 01-04-07 16:21  
---------------------------------------------------------------------- 
Attaching applet_arp9-10.diff (apply on top of 09b).
Can you test it and maybe address "use switch(index_in_str_array)" TODO?

Some highlights:


-static unsigned opt;

many applets were doing this, and now we just use common
global variable - option_mask32.

-#define	ARP_OPT_v (0x80)	/* debugging output flag        */
-#define	ARP_OPT_n (0x100)	/* do not resolve addresses     */
-#define	ARP_OPT_D (0x200)	/* HW-address is devicename     */
-#define	ARP_OPT_s (0x400)
-#define	ARP_OPT_h (0x800)
-
+#define	ARP_OPT_n (0x80)	/* do not resolve addresses     */
+#define	ARP_OPT_D (0x100)	/* HW-address is devicename     */
+#define	ARP_OPT_s (0x200)
+#define	ARP_OPT_v (0x400 * DEBUG)	/* debugging output flag        */

Kill -h (we have --help, and -<unknown option> shows help anyway)
Make -v available only if DEBUG=1, else optimize away all relevant code.

-static char device[16];	/* current device               */
+static char *device;		/* current device               */

We can actually use a pointer, no need to have buffer for this.

 /* Delete an entry from the ARP cache. */
+/* Called only from main, once */
 static int arp_del(char **args)

... and therefore use error_mag_and_die instead of error_msg(); return
-1;...

+//use switch(index_in_str_array) (see mount.c)
 		if (!strcmp(*args, "pub")) {
 			flags |= 1;
 			args++;

Enough said...


-	strcpy(req.arp_dev, device);
+	strncpy(req.arp_dev, device, sizeof(req.arp_dev));

This is safe(r)

-			bb_perror_msg("SIOCDARP(priv)");
-			return -1;
+			bb_perror_msg_and_die("SIOCDARP(priv)");

Yeah, baby. Die plenty.

-static int
+static void
 arp_getdevhw(char *ifname, struct sockaddr *sa, const struct hwtype
*hwt)
...
-	if (opt & ARP_OPT_D) {
-		if (arp_getdevhw(*args++, &req.arp_ha, hw_set ? hw : NULL) < 0)
-			return -1;
+	if (option_mask32 & ARP_OPT_D) {
+		arp_getdevhw(*args++, &req.arp_ha, hw_set ? hw : NULL);

Code review shows that we die immediately if this fails.
Why return -1 then, we can die without returning as well.
Then function can return void as well.

-		strcpy(mask, "-");
-		strcpy(dev, "-");
+		mask[0] = '-'; mask[1] = '\0';
+		dev[0] = '-'; dev[1] = '\0';

Hehe :)

-			if (host[0] && strcmp(ip, host))
+			if (host[0] && strcmp(ip, host) != 0)

Visually hints on "not equal"


Finally, massive main() bloat hunt:

 int arp_main(int argc, char **argv)
 {
-	int what;
-	char *iface_name;
 	char *hw_type;
 	char *protocol;
 
 	/* Initialize variables... */
-	hw = get_hwtype(DFLT_HW);
-	if (!hw)
-		bb_error_msg_and_die("%s: hardware type not supported", DFLT_HW);
 	ap = get_aftype(DFLT_AF);
 	if (!ap)
-		bb_error_msg_and_die("%s: address family not supported", DFLT_AF);
-	what = 0;
-
-	opt =
-		getopt32(argc, argv, "A:p:H:t:i:advnDsh", &protocol, &protocol,
-				 &hw_type, &hw_type, &iface_name);
+		bb_error_msg_and_die("%s: %s not supported", DFLT_AF, "address
family");
 
-	if (opt & ARP_OPT_A || opt & ARP_OPT_p) {
+	getopt32(argc, argv, "A:p:H:t:i:adnDsv", &protocol, &protocol,
+				 &hw_type, &hw_type, &device);
+	argv += optind;
+	if (option_mask32 & ARP_OPT_A || option_mask32 & ARP_OPT_p) {
 		ap = get_aftype(protocol);
 		if (ap == NULL)
-			bb_error_msg_and_die("%s: unknown address family", protocol);
+			bb_error_msg_and_die("%s: unknown %s", protocol, "address family");
 	}
-	if (opt & ARP_OPT_A || opt & ARP_OPT_p) {
+	if (option_mask32 & ARP_OPT_A || option_mask32 & ARP_OPT_p) {
 		hw = get_hwtype(hw_type);
 		if (hw == NULL)
-			bb_error_msg_and_die("%s: unknown hardware type", hw_type);
+			bb_error_msg_and_die("%s: unknown %s", hw_type, "hardware type");
 		hw_set = 1;
 	}
-	if (opt & ARP_OPT_i) {
-		safe_strncpy(device, iface_name, sizeof(device));
-	}
-
-	/* show set or delete something, if not, show usage! */
-	if (opt & ARP_OPT_a) {
-		what = 1;
-	} else if (opt & ARP_OPT_d) {
-		what = 2;
-	} else if (opt & ARP_OPT_s) {
-		what = 3;
-	}
-
-	if (opt & ARP_OPT_h) {
-		bb_show_usage();
-	}
+	//if (option_mask32 & ARP_OPT_i)... -i
 
 	if (ap->af != AF_INET) {
 		bb_error_msg_and_die("%s: kernel only supports 'inet'", ap->name);
 	}
 
 	/* If not hw type specified get default */
-	if (hw_set == 0) {
+	if (!hw) {
 		hw = get_hwtype(DFLT_HW);
 		if (!hw)
-			bb_error_msg_and_die("%s: hardware type not supported", DFLT_HW);
+			bb_error_msg_and_die("%s: %s not supported", DFLT_HW, "hardware
type");
 	}
 
 	if (hw->alen <= 0) {
-		bb_error_msg_and_die("%s: hardware type without ARP support",
-							 hw->name);
+		bb_error_msg_and_die("%s: %s without ARP support",
+							 hw->name, "hardware type");
 	}
 	sockfd = xsocket(AF_INET, SOCK_DGRAM, 0);
+
 	/* Now see what we have to do here... */
-	switch (what) {
-	case 0:
-	case 1:			/* show an ARP entry in the cache */
-		what = arp_show(argv[optind]);
-		break;
-
-	case 2:			/* delete an ARP entry from the cache */
-		what = arp_del(&argv[optind]);
-		break;
-
-	case 3:			/* set an ARP entry in the cache */
-		what = arp_set(&argv[optind]);
-		break;
 
-	default:
-		bb_show_usage();
+	if (option_mask32 & ARP_OPT_a)
+		return arp_show(argv[0]);
+
+	if (argv[0] == NULL) {
+		bb_error_msg_and_die("need host name");
 	}
 
-	return what;
+	if (option_mask32 & ARP_OPT_d)
+		return arp_del(argv);
+	if (option_mask32 & ARP_OPT_s)
+		return arp_set(argv);
+	bb_show_usage();
 } 

---------------------------------------------------------------------- 
 vda - 01-04-07 16:30  
---------------------------------------------------------------------- 
"size arp.o" of 09b and 10:

   text    data     bss     dec     hex
   3762       0      36    3798     ed6 - 09b
   3169       0      17    3186     c72 - 10 

---------------------------------------------------------------------- 
 espakman - 01-05-07 16:01  
---------------------------------------------------------------------- 
Unfortunately the latest patch (10) doesn't fully work anymore. Some
examples:
# ./busybox arp
arp: need host name

Without an argument it should default to -a

# ./busybox arp -d firewall
Segmentation fault

The correct output should be (in testing case):
SIOCDARP(priv): Network is unreachable

But this does work:
# ./busybox arp -d -i eth0 firewall 

#  ./busybox arp -s firewall 12:34:32:53:65:45
Segmentation fault

The correct output should be (in testing case):
SIOCSARP: Network is unreachable

# ./busybox arp -Ds firewall eth0
Segmentation fault

The correct output shoulde be (in testing case):
SIOCSARP: Network is unreachable

This does work:
# ./busybox arp -s firewall 12:34:32:53:65:45 -i eth0 

---------------------------------------------------------------------- 
 vda - 01-05-07 16:58  
---------------------------------------------------------------------- 
Hmm, I'm not sure anymore who is trying to submit a patch to whom ;)

Anyway. Fixing "-a" to be a default is pretty trivial:

diff -urpN busybox.9/networking/arp.c busybox.a/networking/arp.c
--- busybox.9/networking/arp.c  2007-01-06 01:53:15.000000000 +0100
+++ busybox.a/networking/arp.c  2007-01-06 01:47:05.000000000 +0100
@@ -489,17 +489,13 @@ int arp_main(int argc, char **argv)
        sockfd = xsocket(AF_INET, SOCK_DGRAM, 0);

        /* Now see what we have to do here... */
-
-       if (option_mask32 & ARP_OPT_a)
-               return arp_show(argv[0]);
-
-       if (argv[0] == NULL) {
-               bb_error_msg_and_die("need host name");
-       }
-
-       if (option_mask32 & ARP_OPT_d)
+       if (option_mask32 & (ARP_OPT_d|ARP_OPT_s)) {
+               if (argv[0] == NULL)
+                       bb_error_msg_and_die("need host name");
+               if (option_mask32 & ARP_OPT_s)
+                       return arp_set(argv);
                return arp_del(argv);
-       if (option_mask32 & ARP_OPT_s)
-               return arp_set(argv);
-       bb_show_usage();
+       }
+       //if (option_mask32 & ARP_OPT_a) - default
+       return arp_show(argv[0]);
 }

All other test commands did not segfault for me.

Updated arp9-arp10 patch is attached. Please test and maybe attack a TODO
if you have time... 

---------------------------------------------------------------------- 
 espakman - 01-06-07 14:18  
---------------------------------------------------------------------- 
LOL 

Sorry, like I said I'm no programmer and even the most trivial changes are
a struggle to me ;-)
Anyway, index_in_str_array() and host[128] -> RESERVE_CONFIG_BUFFER() are
addressed. Using arp -s or -d without -i still segfaults for me though...
When I undo device[16] -> *device it works again. 

Issue History 
Date Modified   Username       Field                    Change               
====================================================================== 
12-23-06 12:35  espakman       New Issue                                    
12-23-06 12:35  espakman       Status                   new => assigned     
12-23-06 12:35  espakman       Assigned To               => BusyBox         
12-23-06 12:35  espakman       File Added: busybox-arp.patch                    
12-23-06 23:44  vda            Note Added: 0001931                          
12-23-06 23:44  vda            Note Edited: 0001931                         
12-24-06 09:06  espakman       File Added: busybox-arp2.patch                   

12-24-06 09:07  espakman       Note Added: 0001932                          
12-24-06 10:41  espakman       File Added: busybox-arp3.patch                   

12-24-06 10:42  espakman       Note Added: 0001933                          
12-25-06 20:10  vda            Note Added: 0001934                          
12-26-06 05:02  espakman       Note Added: 0001935                          
12-26-06 05:02  espakman       File Added: busybox-arp4.patch                   

12-26-06 06:54  espakman       File Added: busybox-arp5.patch                   

12-26-06 06:55  espakman       Note Added: 0001936                          
12-26-06 09:52  vda            File Added: svn-arp5.patch                    
12-26-06 09:52  vda            File Added: svn-arp6.patch                    
12-26-06 09:52  vda            File Added: arp5-arp6.patch                    
12-26-06 10:05  vda            Note Added: 0001937                          
12-27-06 13:54  espakman       File Added: busybox-arp7.patch                   

12-27-06 14:05  espakman       Note Added: 0001940                          
01-04-07 13:23  espakman       File Added: busybox-arp8.patch                   

01-04-07 13:23  espakman       Note Added: 0001957                          
01-04-07 14:36  bernhardf      File Added: busybox.applet-arp.09b.diff          
         
01-04-07 14:36  bernhardf      Note Added: 0001958                          
01-04-07 14:41  bernhardf      Note Added: 0001959                          
01-04-07 14:45  bernhardf      Note Added: 0001960                          
01-04-07 15:04  bernhardf      Note Added: 0001961                          
01-04-07 16:21  vda            Note Added: 0001962                          
01-04-07 16:21  vda            File Added: applet_arp9-10.diff                  
 
01-04-07 16:30  vda            Note Added: 0001963                          
01-05-07 16:01  espakman       Note Added: 0001964                          
01-05-07 16:56  vda            File Added: applet_arp9-10fixed.diff             
      
01-05-07 16:58  vda            Note Added: 0001965                          
01-06-07 14:18  espakman       Note Added: 0001966                          
======================================================================




More information about the busybox-cvs mailing list