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. 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. 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". 1200: I'd explicitly state "ctds format", which is a known format, rather than an example. 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. 1220: Why not use strerror() here to get the failure reason from open()? 1227: You really ought to use fstyp_strerror() here to retrieve whatever libfstyp is willing to tell you. 1236-1238: could slightly restructure here and eliminate the duplication with 1242-1244. 1240: if there's a failure in fstyp_ident, you don't provide any debug output. td_mountall.c: 22: code from the future, I take it? ;-) Dave
