Hi Denis,
i want to thank for the ifplugd code review! I think I'm one step further with
the busybox's philosophy.
I have written up a short summary explaining the code differences between
implementations. Also can we remove some dead/redundant code from the
'check_existence_through_netlink' function? For example:
//linux/netlink.h
//#define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
// (nlh)->nlmsg_len >= sizeof(struct nlmsghdr) && \
// (nlh)->nlmsg_len <= (len))
if (!NLMSG_OK(mhdr, bytes)
|| bytes < sizeof(struct nlmsghdr) || bytes < mhdr->nlmsg_len // as NLMSG_OK
is declared, with short-cut evaluation its never be performed
) {
...
}
Attached is a patch.
--
max
> > 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
>
> diff --git a/networking/ifplugd.c b/networking/ifplugd.c
index 3739108..38a0324 100644
--- a/networking/ifplugd.c
+++ b/networking/ifplugd.c
@@ -1,6 +1,6 @@
/* vi: set sw=4 ts=4: */
/*
- * ifplugd for busybox
+ * ifplugd for busybox, based on ifplugd 0.28 (written by Lennart Poettering).
*
* Copyright (C) 2009 Maksym Kryzhanovskyy <[email protected]>
*
@@ -22,7 +22,11 @@
#include <linux/wireless.h>
/*
-TODO: describe compat status here.
+From initial port to busybox, removed most of the redundancy by
+converting implementation of a polymorphic interface to the strict
+functional style. The main role is run a script when link state
+changed, other activities like audio signal or detailed reports
+are on the script itself.
One questionable point of the design is netlink usage:
diff --git a/networking/ifplugd.c b/networking/ifplugd.c
index 3739108..f398cca 100644
--- a/networking/ifplugd.c
+++ b/networking/ifplugd.c
@@ -1,6 +1,6 @@
/* vi: set sw=4 ts=4: */
/*
- * ifplugd for busybox
+ * ifplugd for busybox, based on ifplugd 0.28 (written by Lennart Poettering).
*
* Copyright (C) 2009 Maksym Kryzhanovskyy <[email protected]>
*
@@ -22,7 +22,11 @@
#include <linux/wireless.h>
/*
-TODO: describe compat status here.
+From initial port to busybox, removed most of the redundancy by
+converting implementation of a polymorphic interface to the strict
+functional style. The main role is run a script when link state
+changed, other activities like audio signal or detailed reports
+are on the script itself.
One questionable point of the design is netlink usage:
@@ -500,38 +504,30 @@ static NOINLINE int check_existence_through_netlink(void)
mhdr = (struct nlmsghdr*)replybuf;
while (bytes > 0) {
- if (!NLMSG_OK(mhdr, bytes)
- || bytes < sizeof(struct nlmsghdr)
- || bytes < mhdr->nlmsg_len
- ) {
+ if (!NLMSG_OK(mhdr, bytes)) {
bb_error_msg("netlink packet too small or truncated");
return -1;
}
if (mhdr->nlmsg_type == RTM_NEWLINK || mhdr->nlmsg_type == RTM_DELLINK) {
struct rtattr *attr;
- struct ifinfomsg *imsg;
int attr_len;
- imsg = NLMSG_DATA(mhdr);
-
if (mhdr->nlmsg_len < NLMSG_LENGTH(sizeof(struct ifinfomsg))) {
bb_error_msg("netlink packet too small or truncated");
return -1;
}
- attr = (struct rtattr*)((char*)imsg + NLMSG_ALIGN(sizeof(struct ifinfomsg)));
- attr_len = NLMSG_PAYLOAD(mhdr, sizeof(struct ifinfomsg));
+ attr = IFLA_RTA(NLMSG_DATA(mhdr));
+ attr_len = IFLA_PAYLOAD(mhdr);
while (RTA_OK(attr, attr_len)) {
if (attr->rta_type == IFLA_IFNAME) {
- char ifname[IFNAMSIZ + 1];
int len = RTA_PAYLOAD(attr);
-
if (len > IFNAMSIZ)
len = IFNAMSIZ;
- memcpy(ifname, RTA_DATA(attr), len);
- if (strcmp(G.iface, ifname) == 0) {
+
+ if (strncmp(G.iface, RTA_DATA(attr), len) == 0) {
G.iface_exists = (mhdr->nlmsg_type == RTM_NEWLINK);
}
}
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
