[BusyBox 0001130]: arp applet
bugs at busybox.net
bugs at busybox.net
Sun Jan 7 01:24:51 UTC 2007
The following issue has been CLOSED
======================================================================
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: closed
Resolution: open
Fixed in Version:
======================================================================
Date Submitted: 12-23-2006 12:35 PST
Last Modified: 01-06-2007 17:24 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.
----------------------------------------------------------------------
vda - 01-06-07 16:50
----------------------------------------------------------------------
Wow!
Apart from this small buglet:
case 1: // "priv"
flags |= 2;
args++;
+ break;
case 2: // "temp"
your code looks jut fine
I reproduced segfault, fix is:
-static char *device; /* current device */
+static char *device = ""; /* current device */
# size arp.o
text data bss dec hex filename
3028 4 13 3045 be5 arp.o
We re under 3k. Will apply this to svn now.
Thanks for good work.
----------------------------------------------------------------------
vda - 01-06-07 17:24
----------------------------------------------------------------------
Applied in revision 17177, thanks!
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
01-06-07 14:18 espakman File Added: busybox-arp11.patch
01-06-07 16:50 vda Note Added: 0001968
01-06-07 17:24 vda Status assigned => closed
01-06-07 17:24 vda Note Added: 0001969
======================================================================
More information about the busybox-cvs
mailing list