Erik Trauschke writes: > On Mon, 2008-09-08 at 09:49 -0400, James Carlson wrote: > > 142: why "+1"? Port numbers can't be that long. > > +1 for the 0 terminating the string. It might be needed by the string > processing functions.
The #define you're using already includes the 0 terminator. > > 327-328: is this part of this fix? What does it have to do with > > complex port lists? (Probably needs some more information in the CR.) > > > > it is not part of the original CR but will prevent the app from a > segmentation fault in certain cases. If this is not the way to go, I or > my sponsor have to create a new CR for this. If it's a "trivial" fix for some problem you saw while working on this feature, then there's no problem in just mentioning the fix as part of the CR evaluation (your sponsor should help you with that). If it's anything other than trivial, it should probably have its own CR. In any event, I don't care too much whether there's one CR or two; just that the issue (whatever it is) is correctly recorded in some CR somewhere, so that users who run into the problem can find the fix. (I guess a basic question here might be: is this something that happens in practice, or is it just a fix for a theoretical problem?) > > 335-339: why is this checked here inside the loop? Isn't the 'uport' > > string set just once by the command line arguments? What does > > checking the string on each pass buy? > > when using nc in listening mode only one port is allowed and the > build_ports() function is not called. I had the choice of putting it > there or as an else to the if in 326. I decided for the first because it > seemed weird to me to do the same check twice directly after each other. > Since this for loop is usually only run once or only for each new > connection it shouldn't have any performance impact. OK ... it reads odd, but I guess that's fine. > > 410: this isn't part of your change, but it doesn't look right. 's' > > isn't a boolean, and zero *is* a legal file descriptor. > > have to check how it is supposed to work. First guess is to change it to > test for < 0 and initialize s to -1. But changing this will also not be > part of the original CR. Right; just something I noticed in scanning the code. > Thanks for the comments. I will have to check what this hg comment thing > is all about. It should be one line like this: 6691168 allow for complex port list specification in nc or two like this: 6691168 allow for complex port list specification in nc Contributed by Erik Trauschke <[EMAIL PROTECTED]> "hg pbchk" should print warnings about these. -- 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 opensolaris-code@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/opensolaris-code
