Hi Dave - Thanks. I appreciate the feedback. I don't mind fixing the other stuff at all. When I looked at it to figure out the correct why to change it, it was really confusing, so I totally understand why it's not a good practice.
If you wouldn't mind one more look to see if I've addressed the outstanding issue: http://cr.opensolaris.org/~ginnie/4279-3/ Thx, ginnie On 08/26/09 12:14, Dave Miner wrote: > Virginia Wray wrote: >> 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 >> > > Liking it better. Just nits: > > td_mg.c 1414: When you're using boolean_t returns, it's appropriate, > and preferred, to just use the ! operator, i.e. "if (!td_is_fstyp(...))" > > td_mountall.c, 182, 186: by contrast, when you're *not* calling a > function that returns boolean_t, it's inappropriate to not check > against a value; these should be "if (... != 0)" (I know, it was wrong > before you got here, but you touched it ;-) > > Dave -- Ginnie
