Hi Dave - I reworked this function as a boolean. I agree with you. It makes more sense to do it this way. In doing this, I also found that the return values from td_mount_and_add_swap (in the td_mountall.c file) are only checked for a 0 or non-zero value, so I changed them all to -1 if they fail. It looks more symmetrical. The different ERR_ return values in lib_td.h need to be evaluated to see if they are necessary. The numbering seems random. I think they may be some legacy code. They are:
#define ERR_OPENING_VFSTAB 46 #define ERR_ADD_SWAP 47 #define ERR_MOUNT_FAIL 48 #define ERR_MUST_MANUAL_FSCK 49 #define ERR_FSCK_FAILURE 50 #define ERR_DELETE_SWAP 52 #define ERR_UMOUNT_FAIL 53 #define ERR_ZONE_MOUNT_FAIL 65 I thought I would file a bug on the need to evaluate and possibly clean them up. Here is another webrev. Thanks....feedback from others is also welcome. http://cr.opensolaris.org/~ginnie/4279/ CR: http://defect.opensolaris.org/bz/show_bug.cgi?id=4279 ginnie On 08/21/09 10:33, Dave Miner wrote: > Virginia Wray wrote: >> 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/ >> > > Only issues are in td_mg.c. > > - The change Jan suggested is good, but really turns the new function > into a predicate, which would argue for naming along the lines of > "is_fstyp()" or something like that, along with a B_TRUE/B_FALSE > return. I guess I'm not militant on this, but it would be slightly > better. > > - The indentation and continuation usage at 1221, 1228, 1238, 1240 is > incorrect. You don't need continuation markers within function > argument lists, and any continuation there should be indented a > half-tab, not a full one. I'm somewhat surprised that cstyle didn't > complain here. > > Dave -- Ginnie
