ifplugd

[Denys Vlasenko]

On Sunday 19 April 2009 02:04, Maksym Kryzhanovskyy wrote:
> Hi Denis,
> 
> attached is a patch for ifplugd.

+	int iface_disabled;

This flag is "negative". 1 means no and 0 means yes.
Which is hard to read. I am renaming it to iface_enabled.
Hmmm... or rather iface_exists, that's closer to what
it actually means.


static int ifplugd_is_iface_available(void)
{
        struct ifreq ifrequest;

        ifplugd_set_ifreq_to_ifname(&ifrequest);

        if (ifplugd_ioctl(SIOCGIFINDEX, &ifrequest) < 0
         && errno != ENODEV
        ) {
                bb_perror_msg("SIOCGIFINDEX failed");
                return -1;
        }

        return ifrequest.ifr_ifindex < 0;
}

(1) Is it really possible to have ifr_ifindex < 0?
    What does that mean?
(2) If ioctl fails, won't you use uninitialized data
    in ifrequest.ifr_ifindex?


+static int ifplugd_netlink_work(void)
+{
...
+	char ifname[IFNAMSIZ+1] = { 0 };
...
+	while (1) {
+		bytes = recv(netlink_fd, &replybuf, sizeof(replybuf), MSG_DONTWAIT);
+		if (bytes < 0) {
...
+		}
+
+		mhdr = (struct nlmsghdr*)replybuf;
+		while (bytes > 0) {
...
+			if (mhdr->nlmsg_type == RTM_NEWLINK || mhdr->nlmsg_type == RTM_DELLINK) {
...
+				while (RTA_OK(attr, attr_len)) {
+					if(attr->rta_type == IFLA_IFNAME) {
+						int len = RTA_PAYLOAD(attr);
+						if (len > IFNAMSIZ)
+							len = IFNAMSIZ;
+						strncpy(ifname, RTA_DATA(attr), len);
+					}
+					attr = RTA_NEXT(attr, attr_len);
+				}
+
+				if (ifname[0] && strcmp(G.iface, ifname) == 0) {
+					return !(mhdr->nlmsg_type == RTM_NEWLINK);
+				}
+			}
...
+	}

(1) Why do you fill entire ifname with zeroes? ifname[0] = 0 is enough.
(2) You can get ifname for one message but use nlmsg_type from another,
    because you clear ifname in the wrong place. Need to do it
    after if (mhdr->nlmsg_type == RTM_NEWLINK || mhdr->nlmsg_type == RTM_DELLINK)


        logmode |= LOGMODE_NONE;

        iface_status = ifplugd_interface_detect_beat_ethtool();
        if (iface_status != IFSTATUS_ERR) {
                logmode &= ~LOGMODE_NONE;

LOGMODE_NONE is constant 0.


+static int ifplugd_interface_detect_beat_wlan(void)
+{
+	struct iwreq iwrequest;
+	uint8_t mac[ETH_ALEN];
+
+	if (!(option_mask32 & FLAG_NO_AUTO))
+		ifplugd_interface_up();

You repeat ifplugd_interface_up() code in every ifplugd_interface_detect_beat_XXX
function. 5 copies. You can do it just once in ifplugd_detect_beat().


        getopt32(argv, OPTION_STR,
                &G.iface, &G.run, &G.poll_time, &G.delay_up,
                &G.delay_down, &G.api_mode, &G.extra_arg);

G.delay_up/down are time_t's, here you assume they have the same
sizeof as ints. Bad idea.


+		if (poll(netlink_pollfd, (option_mask32 & FLAG_MONITOR) ? 1 : 0,
+		  G.poll_time*1000)
+		) {

Only negative returns from poll are errors. You test for != 0.


ifplugd_netlink_work reports 1 ("interface exists") even if netlink
data does not say whether it exists or not.


I edited the applet a lot and committed it to svn.

Can you test current svn?

Please write up a summary what version of ifplugd it is based on,
what features are implemented compatibly and what is dropped
or changed?

One questionable point of the design is netlink usage:

We have 1 second timeout by default to poll the link status,
it is short enough so that there are no real benefits in
using netlink to get "instantaneous" interface creation/deletion
notifications. We can check for interface existence by just
doing some fast ioctl using its name.

Netlink code then can be just dropped (1k or more?).
--
vda