On Wed, Jun 07, 2017 at 01:17:10PM -0400, Ted Unangst wrote:
Klemens Nanni wrote:
Besides fixing a tautological 'v < 0' and using more descriptive/less
errorprone sizeof(target) this patch unifies strtoul(3) handling both
logically and cosmetically.
A great deal of these look like exactly the situation strtonum was desinged to
cope with. this means dropping support for hex bases, but i'm happy telling
people they need to learn decimal. :)
Leave error checks to strtonum(3) and use data types properly reflecting
those of the target's struct member. This also fixes my previously
introduced systematic flaw:
sizeof(int) != INT_MAX
sizeof is clearly smaller; in fact reliably finding a variable's maximum
value at runtime is impossible in C.
We might also pass the limits to strtonum directly so ifconfig can tell
us what's wrong, e.g.
- v = strtonum(value, 0, UINT8_MAX, &errstr);
+ v = strtonum(value, 1, 10, &errstr);
# ifconfig bridge0 holdcnt 0
- ifconfig: too small arg for holdcnt: 0 # strtonum()
+ ifconfig: bridge0: Invalid argument # ioctl()
This is left unchanged for now since it would also mean having our
limits defined twice which isn't neccessarily a good idea with regard to
future changes.
Feedback/OK?
Index: brconfig.c
===================================================================
RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v
retrieving revision 1.15
diff -u -p -r1.15 brconfig.c
--- brconfig.c 7 Jun 2017 16:47:29 -0000 1.15
+++ brconfig.c 7 Jun 2017 21:30:56 -0000
@@ -220,7 +220,6 @@ bridge_ifsetflag(const char *ifsname, u_
err(1, "%s: ioctl SIOCBRDGGIFFLGS %s", name, ifsname);
req.ifbr_ifsflags |= flag & ~IFBIF_RO_MASK;
-
if (ioctl(s, SIOCBRDGSIFFLGS, (caddr_t)&req) < 0)
err(1, "%s: ioctl SIOCBRDGSIFFLGS %s", name, ifsname);
}
@@ -232,12 +231,10 @@ bridge_ifclrflag(const char *ifsname, u_
strlcpy(req.ifbr_name, name, sizeof(req.ifbr_name));
strlcpy(req.ifbr_ifsname, ifsname, sizeof(req.ifbr_ifsname));
-
if (ioctl(s, SIOCBRDGGIFFLGS, (caddr_t)&req) < 0)
err(1, "%s: ioctl SIOCBRDGGIFFLGS %s", name, ifsname);
req.ifbr_ifsflags &= ~(flag | IFBIF_RO_MASK);
-
if (ioctl(s, SIOCBRDGSIFFLGS, (caddr_t)&req) < 0)
err(1, "%s: ioctl SIOCBRDGSIFFLGS %s", name, ifsname);
}
@@ -400,15 +397,12 @@ void
bridge_timeout(const char *arg, int d)
{
struct ifbrparam bp;
- long newtime;
- char *endptr;
+ int newtime;
+ const char *errstr;
- errno = 0;
- newtime = strtol(arg, &endptr, 0);
- if (arg[0] == '\0' || endptr[0] != '\0' ||
- (newtime & ~INT_MAX) != 0L ||
- (errno == ERANGE && newtime == LONG_MAX))
- errx(1, "invalid arg for timeout: %s", arg);
+ newtime = strtonum(arg, 0, INT_MAX, &errstr);
+ if (errstr != NULL)
+ errx(1, "%s arg for timeout: %s", errstr, arg);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
bp.ifbrp_ctime = newtime;
@@ -420,14 +414,12 @@ void
bridge_maxage(const char *arg, int d)
{
struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ uint8_t v;
+ const char *errstr;
- errno = 0;
- v = strtoul(arg, &endptr, 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
- (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for maxage: %s", arg);
+ v = strtonum(arg, 0, UINT8_MAX, &errstr);
+ if (errstr != NULL)
+ errx(1, "%s arg for maxage: %s", errstr, arg);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
bp.ifbrp_maxage = v;
@@ -439,14 +431,12 @@ void
bridge_priority(const char *arg, int d)
{
struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ uint16_t v;
+ const char *errstr;
- errno = 0;
- v = strtoul(arg, &endptr, 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffffUL ||
- (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for spanpriority: %s", arg);
+ v = strtonum(arg, 0, UINT16_MAX, &errstr);
+ if (errstr != NULL)
+ errx(1, "%s arg for spanpriority: %s", errstr, arg);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
bp.ifbrp_prio = v;
@@ -478,14 +468,12 @@ void
bridge_fwddelay(const char *arg, int d)
{
struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ uint8_t v;
+ const char *errstr;
- errno = 0;
- v = strtoul(arg, &endptr, 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
- (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for fwddelay: %s", arg);
+ v = strtonum(arg, 0, UINT8_MAX, &errstr);
+ if (errstr != NULL)
+ errx(1, "%s arg for fwddelay: %s", errstr, arg);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
bp.ifbrp_fwddelay = v;
@@ -497,14 +485,12 @@ void
bridge_hellotime(const char *arg, int d)
{
struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ uint8_t v;
+ const char *errstr;
- errno = 0;
- v = strtoul(arg, &endptr, 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
- (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for hellotime: %s", arg);
+ v = strtonum(arg, 0, UINT8_MAX, &errstr);
+ if (errstr != NULL)
+ errx(1, "%s arg for hellotime: %s", errstr, arg);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
bp.ifbrp_hellotime = v;
@@ -516,14 +502,12 @@ void
bridge_maxaddr(const char *arg, int d)
{
struct ifbrparam bp;
- unsigned long newsize;
- char *endptr;
+ uint32_t newsize;
+ const char *errstr;
- errno = 0;
- newsize = strtoul(arg, &endptr, 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || newsize > 0xffffffffUL ||
- (errno == ERANGE && newsize == ULONG_MAX))
- errx(1, "invalid arg for maxaddr: %s", arg);
+ newsize = strtonum(arg, 0, UINT32_MAX, &errstr);
+ if (errstr != NULL)
+ errx(1, "%s arg for maxaddr: %s", errstr, arg);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
bp.ifbrp_csize = newsize;
@@ -537,13 +521,12 @@ bridge_deladdr(const char *addr, int d)
struct ifbareq ifba;
struct ether_addr *ea;
- strlcpy(ifba.ifba_name, name, sizeof(ifba.ifba_name));
ea = ether_aton(addr);
if (ea == NULL)
err(1, "Invalid address: %s", addr);
+ strlcpy(ifba.ifba_name, name, sizeof(ifba.ifba_name));
bcopy(ea, &ifba.ifba_dst, sizeof(struct ether_addr));
-
if (ioctl(s, SIOCBRDGDADDR, &ifba) < 0)
err(1, "%s: %s", name, addr);
}
@@ -552,19 +535,16 @@ void
bridge_ifprio(const char *ifname, const char *val)
{
struct ifbreq breq;
- unsigned long v;
- char *endptr;
+ uint8_t v;
+ const char *errstr;
+
+ v = strtonum(val, 0, UINT8_MAX, &errstr);
+ if (errstr != NULL)
+ errx(1, "%s arg for ifpriority: %s", errstr, val);
strlcpy(breq.ifbr_name, name, sizeof(breq.ifbr_name));
strlcpy(breq.ifbr_ifsname, ifname, sizeof(breq.ifbr_ifsname));
-
- errno = 0;
- v = strtoul(val, &endptr, 0);
- if (val[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
- (errno == ERANGE && v == ULONG_MAX))
- err(1, "invalid arg for ifpriority: %s", val);
breq.ifbr_priority = v;
-
if (ioctl(s, SIOCBRDGSIFPRIO, (caddr_t)&breq) < 0)
err(1, "%s: %s", name, val);
}
@@ -573,20 +553,16 @@ void
bridge_ifcost(const char *ifname, const char *val)
{
struct ifbreq breq;
- unsigned long v;
- char *endptr;
+ uint32_t v;
+ const char *errstr;
+
+ v = strtonum(val, 0, UINT32_MAX, &errstr);
+ if (errstr != NULL)
+ errx(1, "%s arg for ifcost: %s", errstr, val);
strlcpy(breq.ifbr_name, name, sizeof(breq.ifbr_name));
strlcpy(breq.ifbr_ifsname, ifname, sizeof(breq.ifbr_ifsname));
-
- errno = 0;
- v = strtoul(val, &endptr, 0);
- if (val[0] == '\0' || endptr[0] != '\0' || v > 0xffffffffUL ||
- (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for ifcost: %s", val);
-
breq.ifbr_path_cost = v;
-
if (ioctl(s, SIOCBRDGSIFCOST, (caddr_t)&breq) < 0)
err(1, "%s: %s", name, val);
}
@@ -598,9 +574,7 @@ bridge_noifcost(const char *ifname, int
strlcpy(breq.ifbr_name, name, sizeof(breq.ifbr_name));
strlcpy(breq.ifbr_ifsname, ifname, sizeof(breq.ifbr_ifsname));
-
breq.ifbr_path_cost = 0;
-
if (ioctl(s, SIOCBRDGSIFCOST, (caddr_t)&breq) < 0)
err(1, "%s", name);
}
@@ -611,16 +585,14 @@ bridge_addaddr(const char *ifname, const
struct ifbareq ifba;
struct ether_addr *ea;
- strlcpy(ifba.ifba_name, name, sizeof(ifba.ifba_name));
- strlcpy(ifba.ifba_ifsname, ifname, sizeof(ifba.ifba_ifsname));
-
ea = ether_aton(addr);
if (ea == NULL)
errx(1, "Invalid address: %s", addr);
+ strlcpy(ifba.ifba_name, name, sizeof(ifba.ifba_name));
+ strlcpy(ifba.ifba_ifsname, ifname, sizeof(ifba.ifba_ifsname));
bcopy(ea, &ifba.ifba_dst, sizeof(struct ether_addr));
ifba.ifba_flags = IFBAF_STATIC;
-
if (ioctl(s, SIOCBRDGSADDR, &ifba) < 0)
err(1, "%s: %s", name, addr);
}
@@ -649,7 +621,6 @@ bridge_addrs(const char *delim, int d)
ifbac.ifbac_buf = inbuf = inb;
strlcpy(ifbac.ifbac_name, name, sizeof(ifbac.ifbac_name));
if (ioctl(s, SIOCBRDGRTS, &ifbac) < 0) {
- if (errno == ENETDOWN)
return;
err(1, "%s", name);
}
@@ -679,13 +650,15 @@ void
bridge_holdcnt(const char *value, int d)
{
struct ifbrparam bp;
+ uint8_t v;
const char *errstr;
- bp.ifbrp_txhc = strtonum(value, 0, UINT8_MAX, &errstr);
- if (errstr)
- err(1, "holdcnt %s %s", value, errstr);
+ v = strtonum(value, 0, UINT8_MAX, &errstr);
+ if (errstr != NULL)
+ errx(1, "%s arg for holdcnt: %s", errstr, value);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
+ bp.ifbrp_txhc = v;
if (ioctl(s, SIOCBRDGSTXHC, (caddr_t)&bp) < 0)
err(1, "%s", name);
}
@@ -700,7 +673,6 @@ is_bridge(char *brdg)
struct ifbaconf ifbac;
strlcpy(ifr.ifr_name, brdg, sizeof(ifr.ifr_name));
-
if (ioctl(s, SIOCGIFFLAGS, (caddr_t)&ifr) < 0)
return (0);
@@ -1047,12 +1019,11 @@ switch_datapathid(const char *arg, int d
{
struct ifbrparam bp;
uint64_t newdpid;
- char *endptr;
+ const char *errstr;
- errno = 0;
- newdpid = strtoull(arg, &endptr, 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || errno == ERANGE)
- errx(1, "invalid arg for datapath-id: %s", arg);
+ newdpid = strtonum(arg, 0, UINT64_MAX, &errstr);
+ if (errstr != NULL)
+ errx(1, "%s arg for datapath-id: %s", errstr, arg);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
bp.ifbrp_datapath = newdpid;
@@ -1065,16 +1036,14 @@ switch_portno(const char *ifname, const
{
struct ifbreq breq;
uint32_t newportidx;
- char *endptr;
+ const char *errstr;
+
+ newportidx = strtonum(val, 0, UINT32_MAX, &errstr);
+ if (errstr != NULL)
+ errx(1, "%s arg for portidx: %s", errstr, val);
strlcpy(breq.ifbr_name, name, sizeof(breq.ifbr_name));
strlcpy(breq.ifbr_ifsname, ifname, sizeof(breq.ifbr_ifsname));
-
- errno = 0;
- newportidx = strtol(val, &endptr, 0);
- if (val[0] == '\0' || endptr[0] != '\0' || errno == ERANGE)
- errx(1, "invalid arg for portidx: %s", val);
-
breq.ifbr_portno = newportidx;
if (ioctl(s, SIOCSWSPORTNO, (caddr_t)&breq) < 0)
err(1, "%s", name);
