Hi Dave - Thank you for the code review. See my responses in line. I've generated another webrev. Could you take a look and see if I've addressed your comments? I did run a test to make sure none of the changes caused a problem.
webrev: http://cr.opensolaris.org/~ginnie/4279-2/ thx, ginnie On 08/19/09 15:20, Dave Miner wrote: > Virginia Wray wrote: >> Hi - >> >> Could I get a code review for the following: >> http://defect.opensolaris.org/bz/show_bug.cgi?id=4279 >> >> Webrev: >> http://cr.opensolaris.org/~ginnie/4279/ >> > > General: please have the fix outlined in a comment in the bug report > when you come for code review. Sanjay provided only the bare minimum > of what this might do. Done. > > td_lib.h: Line 33 shouldn't be added. Header files should reference > other header files only if they directly require a definition from > that other header. This reference should be in the .c files that > actually call libfstyp. ok. Thanks. I've fixed that. > > td_mg.c > > 1195: I think it would be more accurate to say "Determines whether a > device contains a file system of the requested type". Added > > 1200: I'd explicitly state "ctds format", which is a known format, > rather than an example. Added > > 1205: I think you should be tighter about the return values here. For > example, 1222 returns -1, 1229 and 1244 return a libfstyp error, 1238 > returns an strcmp result. Unless you're going to give the caller info > on how to interpret the value, I'd just return -1 in all error cases. ok. For the open command, which returns -1 on error, I left it unchanged. For the fstyp errors, since they vary, I set status to -1 before return. > > 1220: Why not use strerror() here to get the failure reason from open()? I've added that. > > 1227: You really ought to use fstyp_strerror() here to retrieve > whatever libfstyp is willing to tell you. Thanks. I missed this when looking at the fstyp man pages. > > 1236-1238: could slightly restructure here and eliminate the > duplication with 1242-1244. I made a change that I believe is more succinct. > > > 1240: if there's a failure in fstyp_ident, you don't provide any debug > output. I've addressed that and added an fstyp_strerror. > > td_mountall.c: 22: code from the future, I take it? ;-) oops....fixed. > > Dave -- Ginnie
