Re: [PATCH 01/28] Clean up parsing of port ranges

On Wed, Sep 28, 2022 at 10:57:31PM +0200, Stefano Brivio wrote:

On Wed, 28 Sep 2022 14:33:12 +1000 David Gibson wrote:

...

conf_ports() parses ranges of ports for the -t, -u, -T and -U options. The code is quite difficult to the follow, to the point that clang-tidy and cppcheck disagree on whether one of the pointers can be NULL at some points.

Rework the code with the use of two new helper functions: * parse_port_range() operates a bit like strtoul(), but can parse a whole port range specification (e.g. '80' or '1000-1015') * next_chunk() does the necessary wrapping around strchr() to advance to just after the next given delimiter, while cleanly handling if there are no more delimiters

The new version is easier to follow,

Indeed. Just one excess whitespace:

...

[...] + if (*p == ':') { /* There's a range to map to as well */ + if (parse_port_range(p + 1, &p, &mapped_range)) goto bad; - - if (start_dst > end_dst) /* 22-80:8080:8022 */ + if ((mapped_range.last - mapped_range.first) != + (orig_range.last - orig_range.first)) goto bad; + } else { + mapped_range = orig_range; + }

- if (end_dst != -1 && - end_dst - start_dst != end_src - start_src) - goto bad; /* 22-81:8022:8080 */ - - for (i = start_src; i <= end_src; i++) { - if (bitmap_isset(fwd->map, i)) - goto overlap; + if ((*p != '\0') && (*p != ',')) /* Garbage after the ranges */

...here. I can drop it myself.

Actually, I might as well respin it quickly. That way I can add some of the extra background information you've supplied to the commit messages, which might be useful for posterity. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson