On Tue, 2008-09-30 at 17:11 -0400, James Carlson wrote: > Vladimir Kotal writes: > > Erik Trauschke wrote: > > > Alright, I put a new webrev online: > > > http://cr.opensolaris.org/~erisch/nc_080910/ > > > > I went through all the AIs raised by Jim and they seem to be addressed > > except the following: > > > > 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? > > > > - lines 333-337 in current code > > I think there was an answer to this posted to the list -- the answer > (as I recall) was that he needed to have a check for family != > AF_UNIX, and this test block already existed for the local_listen > call. I don't believe that it was functional in any way; I don't > think that testing it on each pass of the loop helps anything. > > I grumbled and said, "ok, I guess I don't care that much." > > Assuming my memory is correct, I'm pretty sure it'd be easier to read > with a separate check of the 'family' value outside of the loop (say > as an 'else' on line 328 in the current code), but I can live with the > wart if that's what the RE wants.
Yes, that's true. You can add the check as an 'else'. The reason for me to do it that way was that I had to decide between two ugly solutions: 1. like I did it in the end, having it checked with every loop (although this loop is usually only run once anyway) 2. put it above the loop as an 'else' although the check for !AF_UNIX is done one statement later again, anyway. For me both possibilities do not look clean to me but I don't want to rip up large parts of the code for something like that. However, if you both think variant 2. looks better I have no problem changing it. > > > 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. > > > > - line 408 in current code > > - it seems this should be 'if (s != -1)' > > Yes; I would have recommended "!= -1" as well, but the current code, > though perhaps a little surprising with "> -1", does the job and > answers the review comment. > I wanted to make sure that it does not call close() even if one of the called functions sets s to something < -1. On closer inspection this can not happen so I can change it to '!= -1' Furthermore I forgot to to the same thing for line 453. Will fix that. Erik _______________________________________________ opensolaris-code mailing list opensolaris-code@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/opensolaris-code
