[systemd-devel] [PATCH 15/28] dhcp: Add example DHCP client test program

[Lennart Poettering]

On Wed, 13.11.13 23:22, Patrik Flykt (patrik.flykt at linux.intel.com) wrote:

> +static int get_mac(int index, struct ether_addr *mac)
> +{
> +        struct ifreq ifr;
> +        int s, err;
> +
> +        if (index < 0 || !mac)
> +                return -EINVAL;
> +
> +        s = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
> +        if (s < 0)
> +                return -EIO;
> +
> +        memset(&ifr, 0, sizeof(ifr));

Please initialize when declaring with = {} instead.

> +        ifr.ifr_ifindex = index;
> +
> +        if (ioctl(s, SIOCGIFNAME, &ifr) < 0) {
> +                err = -errno;
> +                close(s);
> +                return err;
> +        }
> +
> +        if (ioctl(s, SIOCGIFHWADDR, &ifr) < 0) {
> +                err = -errno;
> +                close(s);
> +                return err;
> +        }
> +
> +        memcpy(&mac->ether_addr_octet[0], &ifr.ifr_hwaddr.sa_data, ETH_ALEN);
> +
> +        return 0;

You forget to close the socket on success.. But this looks like
something where you should use Tom's rtnl stuff, no? I am pretty sure we
should avoid the ioctls wherever netlink equivalents exist.

Also, _cleanup_close_ is your friend!

> +}
> +
> +static int get_index(char *ifname)

const char *ifname, no?

> +{
> +        struct ifreq ifr;
> +        int s;
> +
> +        s = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
> +        if (s < 0)
> +                return -EIO;
> +
> +        memset(&ifr, 0, sizeof(ifr));
> +        strncpy(ifr.ifr_name, ifname, IFNAMSIZ);
> +
> +        if (ioctl(s, SIOCGIFINDEX, &ifr) < 0) {
> +                close(s);
> +                return -EIO;
> +        }
> +
> +        return ifr.ifr_ifindex;
> +}

You forgot to close the fd here, too. _cleanup_close_ is your friend
again... Also initialization. 

Also, rtnl sounds the better option. And if not, then this call appears
to be totally identical to glibc's ifname_to_index(), no? Better use
that then...

Lennart

-- 
Lennart Poettering, Red Hat