Erik Trauschke writes:
> Here is the link to the entry in the bug database
> http://bugs.opensolaris.org/bugdatabase/view_bug.do?bug_id=6691168
>
> Here is a link to my webrev directory containing the changes
> http://cr.opensolaris.org/~erisch/nc/
General comment: is it just me or is all this converting between
strings and integers and back again (which exists in the underlying
code) at least a little weird? It seems like portlist should be
converted to be just an int array rather than an array of strings, and
then get rid of the subsequent parsers for portlist.
77-79: I'm not sure this is worthwhile. It'd be simpler and clearer
(at least in my opinion) to do something like this near line 829:
if (portcount + (hi - lo) + 1 > PORT_MAX)
errx(1, "too many ports");
If it's still what you prefer to do, then I suggest at least making
the macro itself use the common ON C style, like this:
#define ADD_LIST_SAFE(list, index, item, max, out) \
if (index < max) { \
list[index] = item; \
} else { \
out; \
}
754: you should have your sponsor (at least) update those CRs to
reference this file as being another copy. If I were an RTI Advocate
(I'm not), I'd probably be less generous: I'd say that if you know
about a problem (lack of common C library function), and you're adding
to the problem (yet another copy), then you're now on the hook to fix
it, even if you didn't cause it in the first place. (But, like I
said, I don't have that authority. :->)
Ditching strsep and using strtok would avoid that particular problem.
795: why +1? I don't see what stores through tmplist[PORT_MAX].
795, et seq.: it'd be simpler to use portlist, rather than putting a
huge (256KB) array on the stack and (effectively) copying at the end
(lines 844-850).
801-802,841: nit: slightly simpler as:
while ((token = strsep(&p, ",") != NULL) {
(Perhaps simpler still as strtok, given the missing libc function.)
807: unclear why this is '0' here. Is ",0," valid while ",0-2," is
not valid?
808: nit: should compare against NULL.
810: why not check for this before doing the first strtonum? It looks
to me like the two cases could be collapsed into a single case (using
"hi = lo;" in the case where there's no '-'), and that'd also get rid
of the need for the macro. To be more precise:
if ((n = strchr(token, '-')) != NULL)
*n++ = '\0';
lo = strtonum(token, PORT_MIN, PORT_MAX, &errstr);
if (errstr != NULL)
errx(1, "port number %s: %s", errstr, token);
if (n == NULL) {
hi = lo;
} else {
hi = strtonum(n, PORT_MIN, PORT_MAX, &errstr);
if (errstr != NULL)
errx(1, "port number %s: %s", errstr, n);
}
if (lo > hi) {
[...]
That collapses it all to a single case.
811: shouldn't this print out errstr?
823: what is 'p'? Doesn't this dump core? (Should be 'token'; should
match strtonum call.)
830,831: tiny nit: I would have done i = lo; i <= hi; i++ instead.
859: this probably drops core (division by zero) when portcount is 1.
Suggest adding "&& portcount > 1" at line 853.
> The fix incorporates a function called strsep() which is about to get in
> libc in the future but is not available so far (see CR6487100 &
> 4383867). So don't wonder about the implementation of strsep() inside
> the netcat.c.
Strange ... one of those two CRs appears to be a duplicate.
--
James Carlson, Solaris Networking <[EMAIL PROTECTED]>
Sun Microsystems / 35 Network Drive 71.232W Vox +1 781 442 2084
MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677
_______________________________________________
opensolaris-code mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/opensolaris-code
