Hi Jan - Thank you for the code review. I've incorporated your suggestions, as well as Dave's. I've generated another webrev. Could you take a look and see if I've addressed your comments? I checked with Mary, and the test she ran on Solaris 10 was with a ufs file system. I should have mentioned in my original code review request the tests I was having her run.
Revised webrev: http://cr.opensolaris.org/~ginnie/4279-2/ thx, ginnie On 08/20/09 03:25, Jan Damborsky wrote: > Hi Ginnie, > > please see my comments below (only those > not already covered by Dave are mentioned). > > Thank you, > Jan > > > td_mg.c > ------- > Since fstyp_ident() allows checking for particular filesystem, > you might avoid using strcmp() and simplifying the code > as well as making life for fstyp_ident() easier, since it wouldn't > need to enumerate trough all known filesystem modules - e.g. > > 1232 if ((status = fstyp_ident(fstyp_handle, NULL, &fstype)) > == 0) { > 1233 td_debug_print(LS_DBGLVL_INFO, \ > 1234 "td_fstyp_check():fstype is %s\n", fstype); > 1235 if ((status = strcmp(fstype, fs)) == 0) { > 1236 fstyp_fini(fstyp_handle); > 1237 (void) close(fd); > 1238 return (status); > 1239 } > 1240 } > > -> > > if ((status = fstyp_ident(fstyp_handle, fs, &fstype)) == 0) { > > /* filesystem matches */ > > } else { > > /* > * filesystem is not that type, capture output of > * fstyp_strerror(fstyp_handle, status) > */ > > } > > > Speaking about testing, I can see from bug report that error messages > disappeared after fix was applied - this covers 'ZFS scenario'. > > With respect to 'UFS scenario' (the code path where there is UFS > filesystem > present on slice), if it was not already covered, I might recommend to > run the > installer on system with Solaris instance on UFS and check that the code > path is activated (td_fstyp_check() correctly recognizes UFS filesystem) > and installer reports Solaris instance on it. > > > > 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/ >> >> thx, >> >
