[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