Hi Tom, while I generaly agree with all your statements, I made just the minimal required changes (fix typo in comment) 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. >
