[ovs-dev] [L3 In-Band 7/9] netdev: Add netdev_get_next_hop call

Ben Pfaff blp at nicira.com
Tue Sep 1 00:13:45 UTC 2009


Justin Pettit <jpettit at nicira.com> writes:

> +/* Looks up the next hop for 'ip'.  If it can be found, the address is

For 'host'?

> + * stored in 'next_hop', the name of the device to reach it is stored in 
> + * 'netdev_name', and 0 is returned.  If a gateway is not required to 
> + * reach 'ip', the value of 'ip' is stored in 'next_hop'.  If a route 
> + * could not be determined, a positive errno is returned. */

The comment here doesn't say how much space the caller needs to
allocate for 'netdev_name'.  %16s, I think, implies that 17 bytes
are required.  That's long enough for any Linux network device
name.  But that will only work for the citrix branch: master
supports more general network devices and we can't (don't,
anyway) assume there that their names will fit in 16 characters.
So it would be better to make netdev_name a char ** argument and
have the caller free() it.

In the error case, would you mind making netdev_get_next_hop()
set next_hop->s_addr to some predictable value (perhaps 0)?  In
my experience this kind of consistency can make programs easier
to debug.

> +int
> +netdev_get_next_hop(const struct in_addr *host, struct in_addr *next_hop, 
> +                    char *netdev_name) {

Please put the { on a line of its own.

> +    static const char fn[] = "/proc/net/route";
> +    FILE *stream;
> +    char line[256];
> +    int ln;
> +
> +    stream = fopen(fn, "r");
> +    if (stream == NULL) {
> +        VLOG_WARN_RL(&rl, "%s: open failed: %s", fn, strerror(errno));
> +        return errno;
> +    }
> +
> +    ln = 0;
> +    while (fgets(line, sizeof line, stream)) {
> +        if (++ln >= 2) {
> +            uint32_t dest, gateway, mask;
> +            int refcnt, metric, mtu;
> +            unsigned int flags, use, window, irtt;
> +
> +            if (sscanf(line,
> +                       "%16s %08X %08X %04X %d %u %d %08X %d %u %u\n",
> +                       netdev_name, &dest, &gateway, &flags, &refcnt,
> +                       &use, &metric, &mask, &mtu, &window, &irtt) != 11) {

For uint32_t the proper scanf specifier is "%"SCNx32.  Also a
leading 0 doesn't mean anything for scanf specifiers (although it
doesn't hurt anything either).

> +
> +                VLOG_WARN_RL(&rl, "%s: could not parse output", fn);

It would be nice to include the line number and possibly the
contents of the line that failed to parse here.  (Also, is
"output" the right word?)

> +                continue;
> +            }
> +            if (!(flags & RTF_UP)) {
> +                /* Skip routes that aren't up. */
> +                continue;
> +            }
> +
> +            if ((dest & mask) == (host->s_addr & mask)) {

I thought there was a byte order mixup here, but when I looked, I
was really surprised to find out that /proc/net/route represents
'dest' and 'mask' in network byte order (as hex strings!).  Maybe
a comment would be in order?

> +                if (!gateway) {
> +                    /* The host is directly reachable. */
> +                    next_hop->s_addr = host->s_addr;
> +                } else {
> +                    /* To reach the host, we must go through a gateway. */
> +                    next_hop->s_addr = gateway;
> +                }
> +                fclose(stream);
> +                return 0;
> +            }
> +        }
> +    }
> +
> +    fclose(stream);
> +    return ENXIO;
> +}
> +




More information about the dev mailing list