Hi Jack, thank you very much for your comments - please see my response in line.
Jan Jack Schwartz wrote: > Hi Jan. > > Here are my comments: > > ti_dm.c: > > 284: Seems to me that nsecs is best defined as a diskaddr_t instead of > a uint32_t. I am not quite convinced that we need to use diskaddr_t for nsecs, since nsecs variable doesn't hold sector address, but it contains different kind of information - number of sectors per cylinder. This is calculated as nsecs = (uint32_t)geom.dkg_nhead * geom.dkg_nsect and both multiplicands are 'unsigned short' with size of 16 bits: http://docs.sun.com/app/docs/doc/820-7598/bjbdq?a=view And product will always fit into 32bits. > On line 315, other variables such as sector_min, are diskaddr_t, and > conceivably nsecs, the number of sectors, can be above sector_min. 'sector_min' is diskaddr_t type since it contains address of 1st available sector in context of creating VTOC slices which could hold data. In general, it will contain value which is multiple of 'nsecs'. I am not sure I can see the problem here - could you please clarify ? > Also, if the def of a diskaddr_t changes in the future, there won't > be a maintainability issue here. To be honest, I am not quite sure what kind of issues might be caused if diskaddr_t is changed in future, as it doesn't seem to be used when number of sectors per cylinder is being obtained/calculated. Could you please elaborate more on this ? > > I looked and diskaddr_t is a u_longlong_t (in uts/common/sys/types.h > a.k.a. /usr/include/sys/types.h) > > This also means changing the declaration of nsecs on 1477. > > 345, 362, 379: "i" is still an int. The first format specifier here > should be %d Agreed - looking at those lines, '%d' is used as format specifier for "i". It doesn't seem that changes are needed. > > ti_dm.h: good eye! Thank you :-) > > The rest looks fine to me. Thanks a lot again ! Jan > > Thanks, > Jack > > > > > On 07/07/09 08:42, Jan Damborsky wrote: >> Hi, >> >> could I please ask for reviewing simple change for following bug ? >> >> 9026 vtoc partition size is not cylinder size adjusted on disk bigger >> than 1.96TB >> >> webrev: >> http://cr.opensolaris.org/~dambi/bug-9026 >> >> While I was there, I have also fixed following issues: >> >> * error message in ti_zfm.c (thanks Greg Onufer for catching this). >> It had ZFS volume name and pool name swapped around. >> >> * typo in idm_cyls_to_mbs macro definition in ti_dm.h >> >> Thank you very much, >> Jan >> >> >> modules affected: >> ----------------- >> * libti >> >> >> testing done >> ------------ >> >> [1] LiveCD - test of fix >> ---------------------------- >> * HW: - Ultra 20 (1GB RWM) >> - target disk: external USB 4TB >> >> * SW: - created LiveCD ISO based on build 117 (contains fix for >> 6843138 GRUB issue) >> with modified libti >> >> * result: >> - installation succeeded (no adjustments done by TI to created VTOC) >> - system booted to desktop >> - format correctly recognized created VTOC >> >> [2] SPARC AI - regression test >> ------------------------------ >> * HW: - sun4v T1000 (2GB RWM - LDOM control domain) >> - target disk: 73GB >> >> * SW: AI image based on build 117 with modified libti >> >> * result: >> - installation succeeded (no adjustments done by TI to created VTOC) >> - system booted >> - format correctly recognized created VTOC >> >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >
