[libvirt] [PATCH v2 1/4] util: introduce helper to parse /proc/net/arp

[Michal Privoznik]

On 01/24/2018 05:09 PM, Chen Hanxiao wrote:
> From: Chen Hanxiao <chenhanxiao at gmail.com>
> 
> introduce helper to parse /proc/net/arp and
> store it in struct virArpTable.
> 
> Signed-off-by: Chen Hanxiao <chenhanxiao at gmail.com>
> ---
>  src/libvirt_private.syms |  2 ++
>  src/util/virmacaddr.c    | 67 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/util/virmacaddr.h    | 18 +++++++++++++
>  3 files changed, 87 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index bc8cc1fba..26385a2e9 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2133,6 +2133,8 @@ virLogVMessage;
>  
>  
>  # util/virmacaddr.h
> +virArpTableFree;
> +virGetArpTable;
>  virMacAddrCmp;
>  virMacAddrCmpRaw;
>  virMacAddrCompare;

See how these APIs are named? virModuleAction. So in your case it should
be virArpTableGet(), virArpTableFree().

> diff --git a/src/util/virmacaddr.c b/src/util/virmacaddr.c
> index 409fdc34d..540bdbbaa 100644
> --- a/src/util/virmacaddr.c
> +++ b/src/util/virmacaddr.c
> @@ -27,10 +27,15 @@
>  #include <stdio.h>
>  
>  #include "c-ctype.h"
> +#include "viralloc.h"
> +#include "virfile.h"
>  #include "virmacaddr.h"
>  #include "virrandom.h"
> +#include "virstring.h"
>  #include "virutil.h"
>  
> +#define VIR_FROM_THIS VIR_FROM_NONE
> +
>  static const unsigned char virMacAddrBroadcastAddrRaw[VIR_MAC_BUFLEN] =
>      { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
>  
> @@ -257,3 +262,65 @@ virMacAddrIsBroadcastRaw(const unsigned char s[VIR_MAC_BUFLEN])
>  {
>      return memcmp(virMacAddrBroadcastAddrRaw, s, sizeof(*s)) == 0;
>  }
> +
> +int
> +virGetArpTable(virArpTablePtr *table)
> +{
> +#define PROC_NET_ARP    "/proc/net/arp"

Since the string is used at exactly one place this #define feels redundant.

Also, the function could allocate the returned table too. In fact it
could return that instead of int:

virArpTablePtr virArpTableGet(void);

> +    FILE *fp = NULL;
> +    char line[1024];
> +    int num = 0;
> +    int ret = -1;
> +
> +    if (!(fp = fopen(PROC_NET_ARP, "r")))
> +        goto cleanup;

Problem with this is that if fopen() fails no error is reported and thus
caller doesn't know what went wrong. Other functions that you call here
do set error (e.g. VIR_REALLOC_N, VIR_STRDUP).

So what happens if this function is called on FreeBSD where ARP table is
not exposed through /proc/net/arp? I guess we need two versions of this
function: Linux and non-Linux one (which could do one thing - report
ENOSUPP error). Look at virNetDevTapInterfaceStats().

> +
> +    while (fgets(line, sizeof(line), fp)) {
> +        char ip[32], mac[32], dev_name[32], hwtype[32],
> +             flags[32], mask[32], nouse[32];
> +
> +        if (STRPREFIX(line, "IP address"))
> +            continue;
> +
> +        num++;

Small nit, you can do this at the end of the loop body. That way you
don't need to have all those (num - 1). And for realloc go with
VIR_REALLOC_N(,num + 1);
This is actually more correct too because if REALLOC fails, num contains
wrong number of records (not that it causes problems, it's just semantic).

> +        if (VIR_REALLOC_N((*table)->t, num) < 0)
> +            goto cleanup;
> +        (*table)->n = num;
> +        /* /proc/net/arp looks like:
> +         * 172.16.17.254  0x1 0x2  e4:68:a3:8d:ed:d3  *   enp3s0
> +         */
> +        sscanf(line, "%[0-9.]%[ ]%[^ ]%[ ]%[^ ]%[ ]%[^ ]%[ ]%[^ ]%[ ]%[^ \t\n]",
> +               ip, nouse,
> +               hwtype, nouse,
> +               flags, nouse,
> +               mac, nouse,
> +               mask, nouse,
> +               dev_name);
> +
> +
> +        if (VIR_STRDUP((*table)->t[num - 1].ipaddr, ip) < 0)
> +            goto cleanup;
> +        if (VIR_STRDUP((*table)->t[num - 1].mac, mac) < 0)
> +            goto cleanup;
> +        if (VIR_STRDUP((*table)->t[num - 1].dev_name, dev_name) < 0)
> +            goto cleanup;
> +    }
> +
> +    ret = 0;
> +
> + cleanup:
> +    VIR_FORCE_FCLOSE(fp);
> +    return ret;
> +}
> +
> +void
> +virArpTableFree(virArpTablePtr table)
> +{
> +    size_t i;
> +    for (i = 0; i < table->n; i++) {
> +        VIR_FREE(table->t[i].ipaddr);
> +        VIR_FREE(table->t[i].mac);
> +        VIR_FREE(table->t[i].dev_name);
> +    }
> +    VIR_FREE(table);
> +}
> diff --git a/src/util/virmacaddr.h b/src/util/virmacaddr.h
> index ef4285d63..eb18092d1 100644
> --- a/src/util/virmacaddr.h
> +++ b/src/util/virmacaddr.h
> @@ -40,6 +40,22 @@ struct _virMacAddr {
>                         false otherwise. */
>  };
>  
> +typedef struct _virArpTableEntry virArpTableEntry;
> +typedef virArpTableEntry *virArpTableEntryPtr;
> +typedef struct _virArpTable virArpTable;
> +typedef virArpTable *virArpTablePtr;

Don't we want to have these in separate file? I mean, we can have
virarptable.[ch] because these are not really MAC related, are they?

Michal