From owner-freebsd-ipfw@FreeBSD.ORG Thu Jan 6 21:57:36 2005 Return-Path: Delivered-To: freebsd-ipfw@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 99F6816A4CE for ; Thu, 6 Jan 2005 21:57:36 +0000 (GMT) Received: from odin.ac.hmc.edu (Odin.AC.HMC.Edu [134.173.32.75]) by mx1.FreeBSD.org (Postfix) with ESMTP id 44E5B43D58 for ; Thu, 6 Jan 2005 21:57:36 +0000 (GMT) (envelope-from brdavis@odin.ac.hmc.edu) Received: from odin.ac.hmc.edu (localhost.localdomain [127.0.0.1]) by odin.ac.hmc.edu (8.13.0/8.13.0) with ESMTP id j06LxN6v003384 for ; Thu, 6 Jan 2005 13:59:23 -0800 Received: (from brdavis@localhost) by odin.ac.hmc.edu (8.13.0/8.13.0/Submit) id j06LxNYr003383 for freebsd-ipfw@freebsd.org; Thu, 6 Jan 2005 13:59:23 -0800 Date: Thu, 6 Jan 2005 13:59:23 -0800 From: Brooks Davis To: freebsd-ipfw@freebsd.org Message-ID: <20050106215923.GA31004@odin.ac.hmc.edu> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.4.1i X-Virus-Scanned: by amavisd-new X-Spam-Status: No, hits=0.0 required=8.0 tests=none autolearn=no version=2.63 X-Spam-Checker-Version: SpamAssassin 2.63 (2004-01-11) on odin.ac.hmc.edu Subject: [PATCH] deprecating abrevations in ipfw X-BeenThere: freebsd-ipfw@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: IPFW Technical Discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 Jan 2005 21:57:36 -0000 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<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