On Mon, 2008-09-08 at 09:49 -0400, James Carlson wrote: > Erik Trauschke writes: > > http://cr.opensolaris.org/~erisch/nc_080907/ > > Does this pass "hg nits" using the current tool set? The changeset > comments don't appear to be in the standard format, and there are > still SCCS keywords here. It's unclear if it'll pass the other tests. > > The evaluation in the CR doesn't have much information in it about the > fix; your sponsor should probably help you with that, as it'll affect > the quality of review comments you get. > ok, right now I don't know exactly what you are talking about. I'll look into that.
> 111,112: what is 'uint'? Suggest using the standard 'uint_t' here > instead. will change that > > 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. > > 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. > 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. > > 337,339,408,795,826,842: nit: indenting seems to be off. I'll check > > 400: nit: initialization does nothing. true, will change that > > 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. > > 823: computation is off by one: the number of ports in the inclusive > range 'lo' to 'hi' is 'hi - lo + 1'. > 840: cstyle problem: missing blank line between variable declaration > and executable code. > > 845: just use NULL instead of casting 0. > will fix that. > 847: not your code ... but what does '& 0xffff' accomplish here? > actually nothing for my understanding ;) it is limited by the mod operation anyway Thanks for the comments. I will have to check what this hg comment thing is all about. Regards Erik _______________________________________________ opensolaris-code mailing list opensolaris-code@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/opensolaris-code
