Date: Thu, 6 Jan 2005 13:59:23 -0800 From: Brooks Davis <brooks@one-eyed-alien.net> To: freebsd-ipfw@freebsd.org Subject: [PATCH] deprecating abrevations in ipfw Message-ID: <20050106215923.GA31004@odin.ac.hmc.edu>
next in thread | raw e-mail | index | archive | help
The ipfw program's code is littered with unmaintainable uses of strncmp
to implement abbreviations. The following patch replaces those with
two new functions which simplify the code and produce warnings that the
syntax is deprecated. In a future release, those can be converted to
hard errors and then finally the code can revert to using strcmp. The
intention is to explicitly support a small number of abbreviations that
actually make sense rather then allowing arbitrary shortening of some
words.
When I examined the code, I found three types of uses of strncmp. Most
commonly, strncmp(av, "string", sizeof(av)) was used to allow av to
match string or any shortened form of it. I have replaced this with a
new function _substrcmp(av, "string") which returns 0 if av is a
substring of "string", but emits a warning if av is not exactly
"string".
The next type was two instances of strncmp(av, "by", 2) which allowed
the abbreviation of bytes to "by", "byt", etc. Unfortunately, it also
supports "bykHUygh&*g&*7*ui". I added a second new function
_substrcmp2(av, "by", "bytes") which acts like the strncmp did, but
complains if the user doesn't spell out the word "bytes".
There was also one correct use of strncmp to match "table(" which might
have another token after it.
Since I was changing all the lines anyway, also fixed the treatment of
strncmp's return as a boolean in many cases. I did modify a few strcmp
cases as well to be fully consistent.
Once final feature of this patch is a slight rewrite of match_token and
match_value. The match_token changes make the code more readable in my
opinion. However, it's possible that for very large rule files, the
old form is enough cheaper then the new form that it's noticeable. The
changes to match_value simply make its variable names and flow more like
match_token. I'd probably commit the separately.
Any objections to this patch?
I plan to MFC this to RELENG_5 after a commit to HEAD.
-- Brooks
==== //depot/user/brooks/dingo/sbin/ipfw/ipfw2.c#1 - /home/brooks/working/freebsd/p4/dingo/sbin/ipfw/ipfw2.c ====
@@ -427,10 +427,12 @@
match_token(struct _s_x *table, char *string)
{
struct _s_x *pt;
- uint i = strlen(string);
+
+ if (strlen(string) == 0)
+ return -1;
- for (pt = table ; i && pt->s != NULL ; pt++)
- if (strlen(pt->s) == i && !bcmp(string, pt->s, i))
+ for (pt = table ; pt->s != NULL ; pt++)
+ if (strcmp(string, pt->s) == 0)
return pt->x;
return -1;
}
@@ -440,15 +442,67 @@
* with the value (NULL in case of failure).
*/
static char const *
-match_value(struct _s_x *p, int value)
+match_value(struct _s_x *table, int value)
{
- for (; p->s != NULL; p++)
- if (p->x == value)
- return p->s;
+ struct _s_x *pt;
+
+ for (pt = table; pt->s != NULL; pt++)
+ if (pt->x == value)
+ return pt->s;
return NULL;
}
/*
+ * _substrcmp takes two strings and returns 1 if they do not match,
+ * and 0 if they match exactly or the first string is a sub-string
+ * of the second. A warning is printed to stderr in the case that the
+ * first string is a sub-string of the second.
+ *
+ * This function will be removed in the future through the usual
+ * deprecation process.
+ */
+static int
+_substrcmp(const char *str1, const char* str2)
+{
+
+ if (strncmp(str1, str2, strlen(str1)) != 0)
+ return 1;
+
+ if (strlen(str1) != strlen(str2))
+ warnx("DEPRECATED: '%s' matched '%s' as a sub-string",
+ str1, str2);
+ return 0;
+}
+
+/*
+ * _substrcmp2 takes three strings and returns 1 if the first two do not match,
+ * and 0 if they match exactly or the second string is a sub-string
+ * of the first. A warning is printed to stderr in the case that the
+ * first string does not match the third.
+ *
+ * This function exists to warn about the bizzare construction
+ * strncmp(str, "by", 2) which is used to allow people to use a shotcut
+ * for "bytes". The problem is that in addition to accepting "by",
+ * "byt", "byte", and "bytes", it also excepts "by_rabid_dogs" and any
+ * other string beginning with "by".
+ *
+ * This function will be removed in the future through the usual
+ * deprecation process.
+ */
+static int
+_substrcmp2(const char *str1, const char* str2, const char* str3)
+{
+
+ if (strncmp(str1, str2, strlen(str2)) != 0)
+ return 1;
+
+ if (strcmp(str1, str3) != 0)
+ warnx("DEPRECATED: '%s' matched '%s'",
+ str1, str3);
+ return 0;
+}
+
+/*
* prints one port, symbolic or numeric
*/
static void
@@ -1760,7 +1814,7 @@
if (!ac)
errx(EX_USAGE, "set needs command");
- if (!strncmp(*av, "show", strlen(*av)) ) {
+ if (_substrcmp(*av, "show") == 0) {
void *data;
char const *msg;
@@ -1784,7 +1838,7 @@
msg = "";
}
printf("\n");
- } else if (!strncmp(*av, "swap", strlen(*av))) {
+ } else if (_substrcmp(*av, "swap") == 0) {
ac--; av++;
if (ac != 2)
errx(EX_USAGE, "set swap needs 2 set numbers\n");
@@ -1796,14 +1850,14 @@
errx(EX_DATAERR, "invalid set number %s\n", av[1]);
masks[0] = (4 << 24) | (new_set << 16) | (rulenum);
i = do_cmd(IP_FW_DEL, masks, sizeof(uint32_t));
- } else if (!strncmp(*av, "move", strlen(*av))) {
+ } else if (_substrcmp(*av, "move") == 0) {
ac--; av++;
- if (ac && !strncmp(*av, "rule", strlen(*av))) {
+ if (ac && _substrcmp(*av, "rule") == 0) {
cmd = 2;
ac--; av++;
} else
cmd = 3;
- if (ac != 3 || strncmp(av[1], "to", strlen(*av)))
+ if (ac != 3 || _substrcmp(av[1], "to") != 0)
errx(EX_USAGE, "syntax: set move [rule] X to Y\n");
rulenum = atoi(av[0]);
new_set = atoi(av[2]);
@@ -1814,9 +1868,9 @@
errx(EX_DATAERR, "invalid dest. set %s\n", av[1]);
masks[0] = (cmd << 24) | (new_set << 16) | (rulenum);
i = do_cmd(IP_FW_DEL, masks, sizeof(uint32_t));
- } else if (!strncmp(*av, "disable", strlen(*av)) ||
- !strncmp(*av, "enable", strlen(*av)) ) {
- int which = !strncmp(*av, "enable", strlen(*av)) ? 1 : 0;
+ } else if (_substrcmp(*av, "disable") == 0 ||
+ _substrcmp(*av, "enable") == 0 ) {
+ int which = _substrcmp(*av, "enable") == 0 ? 1 : 0;
ac--; av++;
masks[0] = masks[1] = 0;
@@ -1828,9 +1882,9 @@
errx(EX_DATAERR,
"invalid set number %d\n", i);
masks[which] |= (1<<i);
- } else if (!strncmp(*av, "disable", strlen(*av)))
+ } else if (_substrcmp(*av, "disable") == 0)
which = 0;
- else if (!strncmp(*av, "enable", strlen(*av)))
+ else if (_substrcmp(*av, "enable") == 0)
which = 1;
else
errx(EX_DATAERR,
@@ -1856,22 +1910,22 @@
if (ac == 0) {
warnx("missing keyword to enable/disable\n");
- } else if (strncmp(*av, "firewall", strlen(*av)) == 0) {
+ } else if (_substrcmp(*av, "firewall") == 0) {
sysctlbyname("net.inet.ip.fw.enable", NULL, 0,
&which, sizeof(which));
- } else if (strncmp(*av, "one_pass", strlen(*av)) == 0) {
+ } else if (_substrcmp(*av, "one_pass") == 0) {
sysctlbyname("net.inet.ip.fw.one_pass", NULL, 0,
&which, sizeof(which));
- } else if (strncmp(*av, "debug", strlen(*av)) == 0) {
+ } else if (_substrcmp(*av, "debug") == 0) {
sysctlbyname("net.inet.ip.fw.debug", NULL, 0,
&which, sizeof(which));
- } else if (strncmp(*av, "verbose", strlen(*av)) == 0) {
+ } else if (_substrcmp(*av, "verbose") == 0) {
sysctlbyname("net.inet.ip.fw.verbose", NULL, 0,
&which, sizeof(which));
- } else if (strncmp(*av, "dyn_keepalive", strlen(*av)) == 0) {
+ } else if (_substrcmp(*av, "dyn_keepalive") == 0) {
sysctlbyname("net.inet.ip.fw.dyn_keepalive", NULL, 0,
&which, sizeof(which));
- } else if (strncmp(*av, "altq", strlen(*av)) == 0) {
+ } else if (_substrcmp(*av, "altq") == 0) {
altq_set_enabled(which);
} else {
warnx("unrecognize enable/disable keyword: %s\n", *av);
@@ -2122,15 +2176,15 @@
cmd->o.len &= ~F_LEN_MASK; /* zero len */
- if (!strncmp(av, "any", strlen(av)))
+ if (_substrcmp(av, "any") == 0)
return;
- if (!strncmp(av, "me", strlen(av))) {
+ if (_substrcmp(av, "me") == 0) {
cmd->o.len |= F_INSN_SIZE(ipfw_insn);
return;
}
- if (!strncmp(av, "table(", 6)) {
+ if (strncmp(av, "table(", 6) == 0) {
char *p = strchr(av + 6, ',');
if (p)
@@ -2341,7 +2395,7 @@
av++; ac--;
NEED1("missing rule specification");
- if (ac > 0 && !strncmp(*av, "set", strlen(*av))) {
+ if (ac > 0 && _substrcmp(*av, "set") == 0) {
do_set = 1; /* delete set */
ac--; av++;
}
@@ -2389,7 +2443,7 @@
cmd->o.len |= F_INSN_SIZE(ipfw_insn_if);
/* Parse the interface or address */
- if (!strcmp(arg, "any"))
+ if (strcmp(arg, "any") == 0)
cmd->o.len = 0; /* effectively ignore this command */
else if (!isdigit(*arg)) {
strlcpy(cmd->name, arg, sizeof(cmd->name));
@@ -2446,7 +2500,8 @@
if (*end == 'K' || *end == 'k') {
p.fs.flags_fs |= DN_QSIZE_IS_BYTES;
p.fs.qsize *= 1024;
- } else if (*end == 'B' || !strncmp(end, "by", 2)) {
+ } else if (*end == 'B' ||
+ _substrcmp2(end, "by", "bytes") == 0) {
p.fs.flags_fs |= DN_QSIZE_IS_BYTES;
}
ac--; av++;
@@ -2603,7 +2658,8 @@
end++;
p.bandwidth *= 1000000;
}
- if (*end == 'B' || !strncmp(end, "by", 2))
+ if (*end == 'B' ||
+ _substrcmp2(end, "by", "bytes") == 0)
p.bandwidth *= 8;
if (p.bandwidth < 0)
errx(EX_DATAERR, "bandwidth too large");
@@ -2736,7 +2792,7 @@
for (i=0; i<6; i++)
addr[i] = mask[i] = 0;
- if (!strcmp(p, "any"))
+ if (strcmp(p, "any") == 0)
return;
for (i=0; *p && i<6;i++, p++) {
@@ -2857,7 +2913,7 @@
struct protoent *pe;
u_char proto = 0;
- if (!strncmp(av, "all", strlen(av)))
+ if (_substrcmp(av, "all") == 0)
; /* same as "ip" */
else if ((proto = atoi(av)) > 0)
; /* all done! */
@@ -2907,7 +2963,7 @@
static ipfw_insn *
add_ports(ipfw_insn *cmd, char *av, u_char proto, int opcode)
{
- if (!strncmp(av, "any", strlen(av))) {
+ if (_substrcmp(av, "any") == 0) {
return NULL;
} else if (fill_newports((ipfw_insn_u16 *)cmd, av, proto)) {
/* XXX todo: check that we have a protocol with ports */
@@ -2979,7 +3035,7 @@
}
/* [set N] -- set number (0..RESVD_SET), optional */
- if (ac > 1 && !strncmp(*av, "set", strlen(*av))) {
+ if (ac > 1 && _substrcmp(*av, "set") == 0) {
int set = strtoul(av[1], NULL, 10);
if (set < 0 || set > RESVD_SET)
errx(EX_DATAERR, "illegal set %s", av[1]);
@@ -2988,7 +3044,7 @@
}
/* [prob D] -- match probability, optional */
- if (ac > 1 && !strncmp(*av, "prob", strlen(*av))) {
+ if (ac > 1 && _substrcmp(*av, "prob") == 0) {
match_prob = strtod(av[1], NULL);
if (match_prob <= 0 || match_prob > 1)
@@ -3132,7 +3188,7 @@
have_log = (ipfw_insn *)c;
cmd->len = F_INSN_SIZE(ipfw_insn_log);
cmd->opcode = O_LOG;
- if (ac && !strncmp(*av, "logamount", strlen(*av))) {
+ if (ac && _substrcmp(*av, "logamount") == 0) {
ac--; av++;
NEED1("logamount requires argument");
l = atoi(*av);
@@ -3193,8 +3249,8 @@
#define CLOSE_PAR \
if (open_par) { \
if (ac && ( \
- !strncmp(*av, ")", strlen(*av)) || \
- !strncmp(*av, "}", strlen(*av)) )) { \
+ strcmp(*av, ")") == 0 || \
+ strcmp(*av, "}") == 0)) { \
prev = NULL; \
open_par = 0; \
ac--; av++; \
@@ -3203,7 +3259,7 @@
}
#define NOT_BLOCK \
- if (ac && !strncmp(*av, "not", strlen(*av))) { \
+ if (ac && _substrcmp(*av, "not") == 0) { \
if (cmd->len & F_NOT) \
errx(EX_USAGE, "double \"not\" not allowed\n"); \
cmd->len |= F_NOT; \
@@ -3211,7 +3267,7 @@
}
#define OR_BLOCK(target) \
- if (ac && !strncmp(*av, "or", strlen(*av))) { \
+ if (ac && _substrcmp(*av, "or") == 0) { \
if (prev == NULL || open_par == 0) \
errx(EX_DATAERR, "invalid OR block"); \
prev->len |= F_OR; \
@@ -3230,8 +3286,8 @@
*/
NOT_BLOCK;
NEED1("missing protocol");
- if (!strncmp(*av, "MAC", strlen(*av)) ||
- !strncmp(*av, "mac", strlen(*av))) {
+ if (_substrcmp(*av, "MAC") == 0 ||
+ _substrcmp(*av, "mac") == 0) {
ac--; av++; /* the "MAC" keyword */
add_mac(cmd, ac, av); /* exits in case of errors */
cmd = next_cmd(cmd);
@@ -3269,7 +3325,7 @@
/*
* "from", mandatory
*/
- if (!ac || strncmp(*av, "from", strlen(*av)))
+ if (!ac || _substrcmp(*av, "from") != 0)
errx(EX_USAGE, "missing ``from''");
ac--; av++;
@@ -3293,7 +3349,7 @@
*/
NOT_BLOCK; /* optional "not" */
if (ac) {
- if (!strncmp(*av, "any", strlen(*av)) ||
+ if (_substrcmp(*av, "any") == 0 ||
add_ports(cmd, *av, proto, O_IP_SRCPORT)) {
ac--; av++;
if (F_LEN(cmd) != 0)
@@ -3304,7 +3360,7 @@
/*
* "to", mandatory
*/
- if (!ac || strncmp(*av, "to", strlen(*av)))
+ if (!ac || _substrcmp(*av, "to") != 0)
errx(EX_USAGE, "missing ``to''");
av++; ac--;
@@ -3328,7 +3384,7 @@
*/
NOT_BLOCK; /* optional "not" */
if (ac) {
- if (!strncmp(*av, "any", strlen(*av)) ||
+ if (_substrcmp(*av, "any") == 0 ||
add_ports(cmd, *av, proto, O_IP_DSTPORT)) {
ac--; av++;
if (F_LEN(cmd) != 0)
@@ -3665,7 +3721,7 @@
case TOK_SRCPORT:
NEED1("missing source port");
- if (!strncmp(*av, "any", strlen(*av)) ||
+ if (_substrcmp(*av, "any") == 0 ||
add_ports(cmd, *av, proto, O_IP_SRCPORT)) {
ac--; av++;
} else
@@ -3674,7 +3730,7 @@
case TOK_DSTPORT:
NEED1("missing destination port");
- if (!strncmp(*av, "any", strlen(*av)) ||
+ if (_substrcmp(*av, "any") == 0 ||
add_ports(cmd, *av, proto, O_IP_DSTPORT)) {
ac--; av++;
} else
@@ -3920,8 +3976,8 @@
} else
errx(EX_USAGE, "table number required");
NEED1("table needs command");
- if (strncmp(*av, "add", strlen(*av)) == 0 ||
- strncmp(*av, "delete", strlen(*av)) == 0) {
+ if (_substrcmp(*av, "add") == 0 ||
+ _substrcmp(*av, "delete") == 0) {
do_add = **av == 'a';
ac--; av++;
if (!ac)
@@ -3945,10 +4001,10 @@
&ent, sizeof(ent)) < 0)
err(EX_OSERR, "setsockopt(IP_FW_TABLE_%s)",
do_add ? "ADD" : "DEL");
- } else if (strncmp(*av, "flush", strlen(*av)) == 0) {
+ } else if (_substrcmp(*av, "flush") == 0) {
if (do_cmd(IP_FW_TABLE_FLUSH, &ent.tbl, sizeof(ent.tbl)) < 0)
err(EX_OSERR, "setsockopt(IP_FW_TABLE_FLUSH)");
- } else if (strncmp(*av, "list", strlen(*av)) == 0) {
+ } else if (_substrcmp(*av, "list") == 0) {
a = ent.tbl;
l = sizeof(a);
if (do_cmd(IP_FW_TABLE_GETSIZE, &a, (uintptr_t)&l) < 0)
@@ -4160,9 +4216,9 @@
* optional: pipe or queue
*/
do_pipe = 0;
- if (!strncmp(*av, "pipe", strlen(*av)))
+ if (_substrcmp(*av, "pipe") == 0)
do_pipe = 1;
- else if (!strncmp(*av, "queue", strlen(*av)))
+ else if (_substrcmp(*av, "queue") == 0)
do_pipe = 2;
if (do_pipe) {
ac--;
@@ -4182,30 +4238,30 @@
av[1] = p;
}
- if (!strncmp(*av, "add", strlen(*av)))
+ if (_substrcmp(*av, "add") == 0)
add(ac, av);
- else if (do_pipe && !strncmp(*av, "config", strlen(*av)))
+ else if (do_pipe && _substrcmp(*av, "config") == 0)
config_pipe(ac, av);
- else if (!strncmp(*av, "delete", strlen(*av)))
+ else if (_substrcmp(*av, "delete") == 0)
delete(ac, av);
- else if (!strncmp(*av, "flush", strlen(*av)))
+ else if (_substrcmp(*av, "flush") == 0)
flush(do_force);
- else if (!strncmp(*av, "zero", strlen(*av)))
+ else if (_substrcmp(*av, "zero") == 0)
zero(ac, av, IP_FW_ZERO);
- else if (!strncmp(*av, "resetlog", strlen(*av)))
+ else if (_substrcmp(*av, "resetlog") == 0)
zero(ac, av, IP_FW_RESETLOG);
- else if (!strncmp(*av, "print", strlen(*av)) ||
- !strncmp(*av, "list", strlen(*av)))
+ else if (_substrcmp(*av, "print") == 0 ||
+ _substrcmp(*av, "list") == 0)
list(ac, av, do_acct);
- else if (!strncmp(*av, "set", strlen(*av)))
+ else if (_substrcmp(*av, "set") == 0)
sets_handler(ac, av);
- else if (!strncmp(*av, "table", strlen(*av)))
+ else if (_substrcmp(*av, "table") == 0)
table_handler(ac, av);
- else if (!strncmp(*av, "enable", strlen(*av)))
+ else if (_substrcmp(*av, "enable") == 0)
sysctl_handler(ac, av, 1);
- else if (!strncmp(*av, "disable", strlen(*av)))
+ else if (_substrcmp(*av, "disable") == 0)
sysctl_handler(ac, av, 0);
- else if (!strncmp(*av, "show", strlen(*av)))
+ else if (_substrcmp(*av, "show") == 0)
list(ac, av, 1 /* show counters */);
else
errx(EX_USAGE, "bad command `%s'", *av);
==== //depot/user/brooks/ports/slimserver/Makefile#20 - /home/brooks/working/freebsd/p4/ports/slimserver/Makefile ====
@@ -26,7 +26,7 @@
RUN_DEPENDS+= ${SLIM_CPAN_DEPS:S|^|${SITE_PERL}/|:S|:|:${PORTSDIR}/|}
.if ${PERL_LEVEL} < 500800
-IGNORE= Need Perl 5.8.x
+IGNORE= "Perl 5.8 or newer required. Install lang/perl5.8 and try again."
.endif
.if ${OSVERSION} < 502110
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20050106215923.GA31004>