Jan Kryl wrote: > Hi Tom, > > while I generaly agree with all your statements, I made > just the minimal required changes (fix typo in comment) >
Jan, Sorry if it was not clear - this was exactly my recommendation. Tom > to avoid another round of testing and to minimize differences > between onnv and amber road gate, where the fix has already > been integrated. > > Thanks for your time! > -jan > > > On 23/02/10 07:42 -0800, Tom Haynes wrote: > >> Jan Kryl wrote: >> >>> Doug, this is a gentle reminder to review the fix below. >>> I need to get this to build 135. If you can't make it >>> then let me know. >>> >>> thanks >>> jan >>> >>> >>> On 18/02/10 15:13 +0100, Jan Kryl wrote: >>> >>> >>>> Hi Doug, >>>> >>>> could you review a fix related to utf-8 fix, which you >>>> have reviewed two weeks ago? The former fix fixed the >>>> issue for file names with non-utf8 byte sequences. This >>>> fix fixes the same problem for directories, symlinks and >>>> special files. >>>> >>>> webrev: http://cr.opensolaris.org/~jkryl/nfs-utf8-dir/ >>>> >>>> thanks >>>> jan >>>> _______________________________________________ >>>> nfs-discuss mailing list >>>> nfs-discuss at opensolaris.org >>>> >>>> >>> _______________________________________________ >>> nfs-discuss mailing list >>> nfs-discuss at opensolaris.org >>> >>> >> The CR comments need to be edited with hg reci. The >> second line is not legal. >> >> I think you need to file a bug to make the error handling >> occur at the end of the function, in one place. I'm okay >> with this change keeping things in place, but in general, >> if you have this all over the place: >> >> 1639 if (name != nm) >> 1640 kmem_free(name, MAXPATHLEN + 1); >> 1641 kmem_free(nm, len); >> 1642 nfs4_ntov_table_free(&ntov, &sarg); >> 1643 resp->attrset = 0; >> >> then in can occur in one place and we reduce both >> clutter and increase legibility. >> >> And instead of making sure that everyone cuts and pastes >> the cleanup, we just direct them to one target. >> >> == >> >> 4270 * System V defines rmdir to return EEXIST, >> 4271 * not * ENOTEMPTY, if the directory is not >> >> Looks like someone else reformatted the comments. Can you >> remove the extra '*'? >> >> ===== >> >> I find "nm" and "name" to not be that enlightening. It seems like >> several of the calls you fixed would have been more obviously >> incorrect if the variable names were not so close. >> >> Again, though, I'm not making this a requirement - just food >> for thought for making this code easier to maintain. >> >> ==== >> >> All-in-all, please fix the comment and then you are okay. >> >>
