Revision 17438c2e-18fd-473e-9dd9-0cfdb4b51f3f - Code Review Stack Exchange

## 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 <kbd>NUL</kbd> 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_STRING_LEN 1023
 #define ARP_BUFFER_LEN (ARP_STRING_LEN + 1)
 
 /* Format for fscanf() to read the 1st, 4th, and 6th space-delimited fields */
 #define ARP_LINE_FORMAT "%" xstr(ARP_STRING_LEN) "s %*s %*s " \
 "%" xstr(ARP_STRING_LEN) "s %*s " \
 "%" xstr(ARP_STRING_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;
 }