Hi Ginnie,
Virginia Wray wrote: > 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? Yes - this is what I had in my mind :-) The changes look good (modulo Dave's 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. ok - that is fine. Thank you for clarifying this. Jan > > 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, >>> >> >
