Return to Answer

• 145.5k
• 22
• 190
• 478

• 145.5k
• 22
• 190
• 478

Concept

I don't believe that there is a cross-platform way to read the ARP cache. From the use of /proc filesystem, I deduce that you are targeting Linux. In Linux, you could read /proc/net/arp as you have done, or run the command ip neigh, which does something similar to your program. (On OS X, you could run arp -a -n instead.)

A comment worth a thousand words

You should include a comment like this in your program:

/**
 * /proc/net/arp looks like this:
 *
 * IP address HW type Flags HW address Mask Device
 * 192.168.12.31 0x1 0x2 00:09:6b:00:02:03 * eth0
 * 192.168.12.70 0x1 0x2 00:01:02:38:4c:85 * eth0
 */

That picture tells me everything I need to know about what you are trying to accomplish — you need hardly explain further. Conversely, not having that picture makes me work a lot harder to reverse-engineer your code.

Minutiae

1. &Buffer[0] is usually written as Buffer.

2. You repeat the readLine() call — once before entering the loop, once at the end of the loop. This is a good opportunity to make use of C's support for side-effects.

    while (0 == (Ret = readLine(Fd, Buffer))) {
    ...
    }
   

3. Since ARP_CACHE is a compile-time constant string, you can just write

    fprintf(stdout, "Arp Cache: Failed to open file \"" ARP_CACHE "\"\n");
   

   instead of doing a %s substitution at runtime.

4. Reading one byte at a time is wasteful.

5. Print errors to standard error; don't contaminate standard output.

6. I suggest naming your variables to be consistent with the /proc/net/arp header fields. For example, device instead of IfaceStr. Also, be consistent with capitalization: count starts with lowercase, which is more common.

Big-picture issues

1. The function int readLine(int Fd, char *Buffer) is kind of susceptible to buffer overflow. I can deduce that fact from the function signature: you pass a pointer to a buffer without also passing the size, so it seems unlikely that readLine would know how to stop when the buffer fills up. You could hard-code it to use ARP_BUFFER_LEN as a limit, but it would be unfortunate to cripple an otherwise generic function like that. It would be better to pass in the buffer size explicitly. That's a general pattern you'll see in C APIs: a pointer to a buffer is frequently accompanied by the buffer's size.

   In practice, /proc/net/arp should never contain a line long enough to overflow a 1024-byte buffer, so you're safe. Still, you should follow idiomatic C conventions.

2. Once you modify readLine() to take a size parameter, you'll find that you've just reinvented fgets().

3. In C, passing strings from a function to its caller is usually avoided, since it involves malloc(), which introduces the potential for memory leaks. Rather, have the caller pass a buffer and size, like how fgets() and scanf() work.

   The only good reason to return a string that was allocated using malloc() would be to support arbitrary-length results. At first glance, your getField() accomplishes that, as it allocates a buffer as large as strlen(Line_Arg). (You forgot to add a byte to accommodate the terminating NUL character, by the way.) But, that turns out not to be the case, since Line_Arg itself is not a string of arbitrary length. It's either less than ARP_BUFFER_LEN bytes long (if you got "lucky") or a buffer overflow (as discussed in (1) above).

4. You seem to be working very hard at I/O and string processing. Why not just use fscanf()?

The whole program, then, can be simplified to

#include <stdio.h>
/**
 * Macros to turn a numeric macro into a string literal. See
 * https://gcc.gnu.org/onlinedocs/cpp/Stringification.html
 */
#define xstr(s) str(s)
#define str(s) #s
#define ARP_CACHE "/proc/net/arp"
#define ARP_BUFFER_LEN 1024
/* Format for fscanf() to read the 1st, 4th, and 6th space-delimited fields */
#define ARP_LINE_FORMAT "%" xstr(ARP_BUFFER_LEN) "s %*s %*s " \
 "%" xstr(ARP_BUFFER_LEN) "s %*s " \
 "%" xstr(ARP_BUFFER_LEN) "s"
int main()
{
 FILE *arpCache = fopen(ARP_CACHE, "r");
 if (!arpCache)
 {
 perror("Arp Cache: Failed to open file \"" ARP_CACHE "\"");
 return 1;
 }
 /* Ignore the first line, which contains the header */
 char header[ARP_BUFFER_LEN];
 if (!fgets(header, sizeof(header), arpCache))
 {
 return 1;
 }
 char ipAddr[ARP_BUFFER_LEN], hwAddr[ARP_BUFFER_LEN], device[ARP_BUFFER_LEN];
 int count = 0;
 while (3 == fscanf(arpCache, ARP_LINE_FORMAT, ipAddr, hwAddr, device))
 {
 printf("%03d: Mac Address of [%s] on [%s] is \"%s\"\n",
 ++count, ipAddr, device, hwAddr);
 }
 fclose(arpCache);
 return 0;
}