Denys Vlasenko wrote:
> Hi,
>
> On Friday 09 July 2010 13:41, Maksym Kryzhanovskyy wrote:
> > attached is a patch for ifplugd. Please review and consider usage.
>
> Max, please briefly explain what this patch fixes.
>
> And, if there are both cosmetic changes and logic changes,
> it makes sense to split the patch into two - it's easier
> to understand the changes that way.
>
I split the previous patch into two, logical and cosmetic.
The first patch simplifies the method choice for detecting the link state,
also it makes 'set_ifreq_to_ifname' function more generic:
function old new delta
ifplugd_main 1056 1119 +63
maybe_up_new_iface 33 35 +2
detect_link 216 208 -8
detect_link_wlan 120 99 -21
set_ifreq_to_ifname 32 - -32
static.method 40 - -40
.rodata 133272 133232 -40
------------------------------------------------------------------------------
(add/remove: 0/2 grow/shrink: 2/3 up/down: 65/-141) Total: -76 bytes
The second patch includes rename the named constants acording to bb convention
and removes those which are not used. No size changes.
Please review and consider usage.
Max.
> --
> vda
>
>
>
diff --git a/networking/ifplugd.c b/networking/ifplugd.c
index eb74428..1e7537d 100644
--- a/networking/ifplugd.c
+++ b/networking/ifplugd.c
@@ -79,7 +79,6 @@ enum { // api mode
API_WLAN = 'w',
API_IFF = 'i',
};
-static const char api_modes[] ALIGN1 = "aempwi";
enum { // interface status
IFSTATUS_ERR = -1,
@@ -96,7 +95,6 @@ struct globals {
smallint iface_last_status;
smallint iface_prev_status;
smallint iface_exists;
- smallint api_method_num;
/* Used in getopt32, must have sizeof == sizeof(int) */
unsigned poll_time;
@@ -109,6 +107,10 @@ struct globals {
const char *extra_arg;
smallint (*detect_link_func)(void);
+ struct {
+ const char *name;
+ smallint (*func)(void);
+ } detect_link_methods[6];
};
#define G (*ptr_to_globals)
#define INIT_G() do { \
@@ -120,8 +122,19 @@ struct globals {
G.iface = "eth0"; \
G.api_mode = "a"; \
G.script_name = "/etc/ifplugd/ifplugd.action"; \
+ G.detect_link_methods[0].name = "SIOCETHTOOL"; \
+ G.detect_link_methods[0].func = &detect_link_ethtool; \
+ G.detect_link_methods[1].name = "SIOCGMIIPHY"; \
+ G.detect_link_methods[1].func = &detect_link_mii; \
+ G.detect_link_methods[2].name = "SIOCDEVPRIVATE"; \
+ G.detect_link_methods[2].func = &detect_link_priv; \
+ G.detect_link_methods[3].name = "wireless extension"; \
+ G.detect_link_methods[3].func = &detect_link_wlan; \
+ G.detect_link_methods[4].name = "IFF_RUNNING"; \
+ G.detect_link_methods[4].func = &detect_link_iff; \
} while (0)
+static const char api_modes[] ALIGN1 = "empwia";
static const char *strstatus(int status)
{
@@ -169,10 +182,15 @@ static int network_ioctl(int request, void* data, const char *errmsg)
return r;
}
-static void set_ifreq_to_ifname(struct ifreq *ifreq)
+static void set_ifreq_to_ifname(void *req, size_t sz)
{
- memset(ifreq, 0, sizeof(struct ifreq));
- strncpy_IFNAMSIZ(ifreq->ifr_name, G.iface);
+ struct req {
+ union {
+ char ifname[IFNAMSIZ];
+ } u;
+ };
+ memset(req, 0, sz);
+ strncpy_IFNAMSIZ(((struct req *)req)->u.ifname, G.iface);
}
static void up_iface(void)
@@ -182,7 +200,7 @@ static void up_iface(void)
if (!G.iface_exists)
return;
- set_ifreq_to_ifname(&ifrequest);
+ set_ifreq_to_ifname(&ifrequest, sizeof(struct ifreq));
if (network_ioctl(SIOCGIFFLAGS, &ifrequest, "getting interface flags") < 0) {
G.iface_exists = 0;
return;
@@ -240,7 +258,7 @@ static void maybe_up_new_iface(void)
G.iface, buf, driver_info.driver, driver_info.version);
}
#endif
- if (G.api_method_num == 0)
+ if (G.api_mode[0] == 'a')
G.detect_link_func = NULL;
}
@@ -249,7 +267,7 @@ static smallint detect_link_mii(void)
struct ifreq ifreq;
struct mii_ioctl_data *mii = (void *)&ifreq.ifr_data;
- set_ifreq_to_ifname(&ifreq);
+ set_ifreq_to_ifname(&ifreq, sizeof(struct ifreq));
if (network_ioctl(SIOCGMIIPHY, &ifreq, "SIOCGMIIPHY") < 0) {
return IFSTATUS_ERR;
@@ -269,7 +287,7 @@ static smallint detect_link_priv(void)
struct ifreq ifreq;
struct mii_ioctl_data *mii = (void *)&ifreq.ifr_data;
- set_ifreq_to_ifname(&ifreq);
+ set_ifreq_to_ifname(&ifreq, sizeof(struct ifreq));
if (network_ioctl(SIOCDEVPRIVATE, &ifreq, "SIOCDEVPRIVATE") < 0) {
return IFSTATUS_ERR;
@@ -289,7 +307,7 @@ static smallint detect_link_ethtool(void)
struct ifreq ifreq;
struct ethtool_value edata;
- set_ifreq_to_ifname(&ifreq);
+ set_ifreq_to_ifname(&ifreq, sizeof(struct ifreq));
edata.cmd = ETHTOOL_GLINK;
ifreq.ifr_data = (void*) &edata;
@@ -305,7 +323,7 @@ static smallint detect_link_iff(void)
{
struct ifreq ifreq;
- set_ifreq_to_ifname(&ifreq);
+ set_ifreq_to_ifname(&ifreq, sizeof(struct ifreq));
if (network_ioctl(SIOCGIFFLAGS, &ifreq, "SIOCGIFFLAGS") < 0) {
return IFSTATUS_ERR;
@@ -329,8 +347,7 @@ static smallint detect_link_wlan(void)
struct iwreq iwrequest;
uint8_t mac[ETH_ALEN];
- memset(&iwrequest, 0, sizeof(iwrequest));
- strncpy_IFNAMSIZ(iwrequest.ifr_ifrn.ifrn_name, G.iface);
+ set_ifreq_to_ifname(&iwrequest, sizeof(struct iwreq));
if (network_ioctl(SIOCGIWAP, &iwrequest, "SIOCGIWAP") < 0) {
return IFSTATUS_ERR;
@@ -351,16 +368,6 @@ static smallint detect_link_wlan(void)
static smallint detect_link(void)
{
- static const struct {
- const char *name;
- smallint (*func)(void);
- } method[] = {
- { "SIOCETHTOOL" , &detect_link_ethtool },
- { "SIOCGMIIPHY" , &detect_link_mii },
- { "SIOCDEVPRIVATE" , &detect_link_priv },
- { "wireless extension", &detect_link_wlan },
- { "IFF_RUNNING" , &detect_link_iff },
- };
smallint status;
if (!G.iface_exists)
@@ -374,34 +381,31 @@ static smallint detect_link(void)
up_iface();
if (!G.detect_link_func) {
- if (G.api_method_num == 0) {
- int i;
- smallint sv_logmode;
-
- sv_logmode = logmode;
- for (i = 0; i < ARRAY_SIZE(method); i++) {
- logmode = LOGMODE_NONE;
- status = method[i].func();
- logmode = sv_logmode;
- if (status != IFSTATUS_ERR) {
- G.detect_link_func = method[i].func;
- bb_error_msg("using %s detection mode", method[i].name);
- goto _2;
- }
+ smallint sv_logmode, i;
+
+ sv_logmode = logmode;
+ for (i = 0; G.detect_link_methods[i].func; i++) {
+ logmode = LOGMODE_NONE;
+ status = G.detect_link_methods[i].func();
+ logmode = sv_logmode;
+ if (status != IFSTATUS_ERR) {
+ G.detect_link_func = G.detect_link_methods[i].func;
+ bb_error_msg("using %s detection mode", G.detect_link_methods[i].name);
+ goto _2;
}
- goto _1;
}
- G.detect_link_func = method[G.api_method_num - 1].func;
+ goto _1;
}
status = G.detect_link_func();
- _1:
+
if (status == IFSTATUS_ERR) {
+ _1:
if (option_mask32 & FLAG_IGNORE_FAIL)
status = IFSTATUS_DOWN;
else if (option_mask32 & FLAG_IGNORE_FAIL_POSITIVE)
status = IFSTATUS_UP;
- else if (G.api_method_num == 0)
+ else if (G.api_mode[0] == 'a')
bb_error_msg("can't detect link status");
}
_2:
@@ -548,7 +552,7 @@ int ifplugd_main(int argc UNUSED_PARAM, char **argv)
api_mode_found = strchr(api_modes, G.api_mode[0]);
if (!api_mode_found)
bb_error_msg_and_die("unknown API mode '%s'", G.api_mode);
- G.api_method_num = api_mode_found - api_modes;
+ G.detect_link_func = G.detect_link_methods[api_mode_found - api_modes].func;
if (!(opts & FLAG_NO_DAEMON))
bb_daemonize_or_rexec(DAEMON_CHDIR_ROOT, argv);
@@ -578,7 +582,7 @@ int ifplugd_main(int argc UNUSED_PARAM, char **argv)
if (opts & FLAG_MONITOR) {
struct ifreq ifrequest;
- set_ifreq_to_ifname(&ifrequest);
+ set_ifreq_to_ifname(&ifrequest, sizeof(struct ifreq));
G.iface_exists = (network_ioctl(SIOCGIFINDEX, &ifrequest, NULL) == 0);
}
diff --git a/networking/ifplugd.c b/networking/ifplugd.c
index eb74428..e28af50 100644
--- a/networking/ifplugd.c
+++ b/networking/ifplugd.c
@@ -71,16 +71,6 @@ enum {
# define OPTION_STR "+ansfFi:r:It:u:d:m:pqlx:M"
#endif
-enum { // api mode
- API_AUTO = 'a',
- API_ETHTOOL = 'e',
- API_MII = 'm',
- API_PRIVATE = 'p',
- API_WLAN = 'w',
- API_IFF = 'i',
-};
-static const char api_modes[] ALIGN1 = "aempwi";
-
enum { // interface status
IFSTATUS_ERR = -1,
IFSTATUS_DOWN = 0,
@@ -88,15 +78,14 @@ enum { // interface status
};
enum { // constant fds
- ioctl_fd = 3,
- netlink_fd = 4,
+ IOCTL_FD = 3,
+ NETLINK_FD = 4,
};
struct globals {
smallint iface_last_status;
smallint iface_prev_status;
smallint iface_exists;
- smallint api_method_num;
/* Used in getopt32, must have sizeof == sizeof(int) */
unsigned poll_time;
@@ -109,6 +98,10 @@ struct globals {
const char *extra_arg;
smallint (*detect_link_func)(void);
+ struct {
+ const char *name;
+ smallint (*func)(void);
+ } detect_link_methods[6];
};
#define G (*ptr_to_globals)
#define INIT_G() do { \
@@ -120,8 +113,19 @@ struct globals {
G.iface = "eth0"; \
G.api_mode = "a"; \
G.script_name = "/etc/ifplugd/ifplugd.action"; \
+ G.detect_link_methods[0].name = "SIOCETHTOOL"; \
+ G.detect_link_methods[0].func = &detect_link_ethtool; \
+ G.detect_link_methods[1].name = "SIOCGMIIPHY"; \
+ G.detect_link_methods[1].func = &detect_link_mii; \
+ G.detect_link_methods[2].name = "SIOCDEVPRIVATE"; \
+ G.detect_link_methods[2].func = &detect_link_priv; \
+ G.detect_link_methods[3].name = "wireless extension"; \
+ G.detect_link_methods[3].func = &detect_link_wlan; \
+ G.detect_link_methods[4].name = "IFF_RUNNING"; \
+ G.detect_link_methods[4].func = &detect_link_iff; \
} while (0)
+static const char api_modes[] ALIGN1 = "empwia";
static const char *strstatus(int status)
{
@@ -163,16 +167,21 @@ static int run_script(const char *action)
static int network_ioctl(int request, void* data, const char *errmsg)
{
- int r = ioctl(ioctl_fd, request, data);
+ int r = ioctl(IOCTL_FD, request, data);
if (r < 0 && errmsg)
bb_perror_msg("%s failed", errmsg);
return r;
}
-static void set_ifreq_to_ifname(struct ifreq *ifreq)
+static void set_ifreq_to_ifname(void *req, size_t sz)
{
- memset(ifreq, 0, sizeof(struct ifreq));
- strncpy_IFNAMSIZ(ifreq->ifr_name, G.iface);
+ struct req {
+ union {
+ char ifname[IFNAMSIZ];
+ } u;
+ };
+ memset(req, 0, sz);
+ strncpy_IFNAMSIZ(((struct req *)req)->u.ifname, G.iface);
}
static void up_iface(void)
@@ -182,7 +191,7 @@ static void up_iface(void)
if (!G.iface_exists)
return;
- set_ifreq_to_ifname(&ifrequest);
+ set_ifreq_to_ifname(&ifrequest, sizeof(struct ifreq));
if (network_ioctl(SIOCGIFFLAGS, &ifrequest, "getting interface flags") < 0) {
G.iface_exists = 0;
return;
@@ -240,7 +249,7 @@ static void maybe_up_new_iface(void)
G.iface, buf, driver_info.driver, driver_info.version);
}
#endif
- if (G.api_method_num == 0)
+ if (G.api_mode[0] == 'a')
G.detect_link_func = NULL;
}
@@ -249,7 +258,7 @@ static smallint detect_link_mii(void)
struct ifreq ifreq;
struct mii_ioctl_data *mii = (void *)&ifreq.ifr_data;
- set_ifreq_to_ifname(&ifreq);
+ set_ifreq_to_ifname(&ifreq, sizeof(struct ifreq));
if (network_ioctl(SIOCGMIIPHY, &ifreq, "SIOCGMIIPHY") < 0) {
return IFSTATUS_ERR;
@@ -269,7 +278,7 @@ static smallint detect_link_priv(void)
struct ifreq ifreq;
struct mii_ioctl_data *mii = (void *)&ifreq.ifr_data;
- set_ifreq_to_ifname(&ifreq);
+ set_ifreq_to_ifname(&ifreq, sizeof(struct ifreq));
if (network_ioctl(SIOCDEVPRIVATE, &ifreq, "SIOCDEVPRIVATE") < 0) {
return IFSTATUS_ERR;
@@ -289,7 +298,7 @@ static smallint detect_link_ethtool(void)
struct ifreq ifreq;
struct ethtool_value edata;
- set_ifreq_to_ifname(&ifreq);
+ set_ifreq_to_ifname(&ifreq, sizeof(struct ifreq));
edata.cmd = ETHTOOL_GLINK;
ifreq.ifr_data = (void*) &edata;
@@ -305,7 +314,7 @@ static smallint detect_link_iff(void)
{
struct ifreq ifreq;
- set_ifreq_to_ifname(&ifreq);
+ set_ifreq_to_ifname(&ifreq, sizeof(struct ifreq));
if (network_ioctl(SIOCGIFFLAGS, &ifreq, "SIOCGIFFLAGS") < 0) {
return IFSTATUS_ERR;
@@ -329,8 +338,7 @@ static smallint detect_link_wlan(void)
struct iwreq iwrequest;
uint8_t mac[ETH_ALEN];
- memset(&iwrequest, 0, sizeof(iwrequest));
- strncpy_IFNAMSIZ(iwrequest.ifr_ifrn.ifrn_name, G.iface);
+ set_ifreq_to_ifname(&iwrequest, sizeof(struct iwreq));
if (network_ioctl(SIOCGIWAP, &iwrequest, "SIOCGIWAP") < 0) {
return IFSTATUS_ERR;
@@ -351,16 +359,6 @@ static smallint detect_link_wlan(void)
static smallint detect_link(void)
{
- static const struct {
- const char *name;
- smallint (*func)(void);
- } method[] = {
- { "SIOCETHTOOL" , &detect_link_ethtool },
- { "SIOCGMIIPHY" , &detect_link_mii },
- { "SIOCDEVPRIVATE" , &detect_link_priv },
- { "wireless extension", &detect_link_wlan },
- { "IFF_RUNNING" , &detect_link_iff },
- };
smallint status;
if (!G.iface_exists)
@@ -374,34 +372,31 @@ static smallint detect_link(void)
up_iface();
if (!G.detect_link_func) {
- if (G.api_method_num == 0) {
- int i;
- smallint sv_logmode;
-
- sv_logmode = logmode;
- for (i = 0; i < ARRAY_SIZE(method); i++) {
- logmode = LOGMODE_NONE;
- status = method[i].func();
- logmode = sv_logmode;
- if (status != IFSTATUS_ERR) {
- G.detect_link_func = method[i].func;
- bb_error_msg("using %s detection mode", method[i].name);
- goto _2;
- }
+ smallint sv_logmode, i;
+
+ sv_logmode = logmode;
+ for (i = 0; G.detect_link_methods[i].func; i++) {
+ logmode = LOGMODE_NONE;
+ status = G.detect_link_methods[i].func();
+ logmode = sv_logmode;
+ if (status != IFSTATUS_ERR) {
+ G.detect_link_func = G.detect_link_methods[i].func;
+ bb_error_msg("using %s detection mode", G.detect_link_methods[i].name);
+ goto _2;
}
- goto _1;
}
- G.detect_link_func = method[G.api_method_num - 1].func;
+ goto _1;
}
status = G.detect_link_func();
- _1:
+
if (status == IFSTATUS_ERR) {
+ _1:
if (option_mask32 & FLAG_IGNORE_FAIL)
status = IFSTATUS_DOWN;
else if (option_mask32 & FLAG_IGNORE_FAIL_POSITIVE)
status = IFSTATUS_UP;
- else if (G.api_method_num == 0)
+ else if (G.api_mode[0] == 'a')
bb_error_msg("can't detect link status");
}
_2:
@@ -423,7 +418,7 @@ static NOINLINE int check_existence_through_netlink(void)
struct nlmsghdr *mhdr;
ssize_t bytes;
- bytes = recv(netlink_fd, &replybuf, sizeof(replybuf), MSG_DONTWAIT);
+ bytes = recv(NETLINK_FD, &replybuf, sizeof(replybuf), MSG_DONTWAIT);
if (bytes < 0) {
if (errno == EAGAIN)
return G.iface_exists;
@@ -548,14 +543,14 @@ int ifplugd_main(int argc UNUSED_PARAM, char **argv)
api_mode_found = strchr(api_modes, G.api_mode[0]);
if (!api_mode_found)
bb_error_msg_and_die("unknown API mode '%s'", G.api_mode);
- G.api_method_num = api_mode_found - api_modes;
+ G.detect_link_func = G.detect_link_methods[api_mode_found - api_modes].func;
if (!(opts & FLAG_NO_DAEMON))
bb_daemonize_or_rexec(DAEMON_CHDIR_ROOT, argv);
- xmove_fd(xsocket(AF_INET, SOCK_DGRAM, 0), ioctl_fd);
+ xmove_fd(xsocket(AF_INET, SOCK_DGRAM, 0), IOCTL_FD);
if (opts & FLAG_MONITOR) {
- xmove_fd(netlink_open(), netlink_fd);
+ xmove_fd(netlink_open(), NETLINK_FD);
}
write_pidfile(pidfile_name);
@@ -578,7 +573,7 @@ int ifplugd_main(int argc UNUSED_PARAM, char **argv)
if (opts & FLAG_MONITOR) {
struct ifreq ifrequest;
- set_ifreq_to_ifname(&ifrequest);
+ set_ifreq_to_ifname(&ifrequest, sizeof(struct ifreq));
G.iface_exists = (network_ioctl(SIOCGIFINDEX, &ifrequest, NULL) == 0);
}
@@ -612,7 +607,7 @@ int ifplugd_main(int argc UNUSED_PARAM, char **argv)
}
/* Main loop */
- netlink_pollfd[0].fd = netlink_fd;
+ netlink_pollfd[0].fd = NETLINK_FD;
netlink_pollfd[0].events = POLLIN;
delay_time = 0;
while (1) {
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
