On Fri, 2009-11-20 at 14:57 +0100, Jan Damborsky wrote:
> Hi Alex,
>
> please see my comments below.
>
> BTW, thank you very much for addressing
> cstyle issues.
>
> Jan
>
>
> boot_archive_archive.py
> -----------------------
>
> 356 TI_ATTR_DC_RAMDISK_INODES_NUM: TI_DC_RAMDISK_INODES_DEFAULT,
>
> Ramdisk is created with 2048 bytes/inode, but looking at bug Report,
> it suggests to use optimized value 16384 instead. Why is default used ?
>
>
> ti_api.h
> --------
>
> 102 #define TI_DC_RAMDISK_INODES_DEFAULT 2048
>
> Default is hardcoded - I might recommend not to define it at all and
> instead take advantage of the fact that newfs(1M) calculates default value
> if none is explicitly specified - please also see below.
>
>
> 296 /* uint_32_t - # of inodes to be created */
> 297 #define TI_ATTR_DC_RAMDISK_INODES_NUM "ti_dc_ramdisk_inodes_num"
>
> Since we are actually tuning number of bytes per inode, not number
> of inodes, I might recommend to change the comment and name of the
> attribute to reflect this - e.g.
>
> 296 /* uint_32_t - # of bytes per inode */
> 297 #define TI_ATTR_DC_RAMDISK_BYTES_PER_INODE
> "ti_dc_ramdisk_bytes_per_inode"
>
>
> ti_dcm.c
> --------
>
> TI_ATTR_DC_RAMDISK_INODES_NUM is mandatory attribute and TI fails to create
> ramdisk if it is not specified.
>
> Since it is optional parameter for newfs(1M), I might recommend to make
> it optional for TI as well and let newfs(1M) work out the default nbpi
> if this attribute is not provided - e.g. something like
>
> 158 if (nvlist_lookup_uint32(attrs, TI_ATTR_DC_RAMDISK_INODES_NUM,
> 159 &ramdisk_nbpi) != 0) {
> 160 ls_write_dbg_message(TIDC, LS_DBGLVL_ERR,
> 161 "Number of bytes per inode not provided\n");
> 162 return (TI_E_INVALID_RAMDISK_ATTR);
> 163 }
> ->
> 158 if (nvlist_lookup_uint32(attrs, TI_ATTR_DC_RAMDISK_INODES_NUM,
> 159 &ramdisk_nbpi) != 0) {
> 160 ls_write_dbg_message(TIDC, LS_DBGLVL_INFO,
> 161 "Number of bytes per inode not provided, newfs(1M)"
> 162 " will use the default value\n");
> 163 ramdisk_nbpi = 0;
> 164 }
>
> ...
>
> 211 (void) snprintf(cmd, sizeof (cmd), "/usr/sbin/newfs -m 0 -o space
> "
> 212 "-i %d %s 0</dev/null", ramdisk_nbpi, pseudod);
> ->
> 211 if (ramdisk_nbpi != 0)
> 212 (void) snprintf(cmd, sizeof (cmd), "/usr/sbin/newfs -m 0 -o
> space "
> 213 "-i %d %s 0</dev/null", ramdisk_nbpi, pseudod);
> 214 else
> 215 (void) snprintf(cmd, sizeof (cmd), "/usr/sbin/newfs -m 0 -o
> space "
> 216 "%s 0</dev/null", pseudod);
>
>
> Alexander Eremin wrote:
> > Please review fix for 8205
> >
> > webrev:http://cr.opensolaris.org/~alhazred/8205/
> >
> > # test_ti_static
> > ...
> > ramdisk options (select ramdisk type: option "-t r")
> > -r ramdisk_size size of ramdisk in Kbytes
> > -m ramdisk_mp mount point of ramdisk
> > -i nbpi number of bytes per inode
> > -b boot_archive file name of boot archive
> > -R release indicated target (see -m, -b)
> > ....
> >
> > install_log output:
> > <TIMM_I Nov 17 13:21:13> Target type to be created: DC_RAMDISK
> > <TIDC_I Nov 17 13:21:13> ramdisk cmd: /usr/sbin/mkfile 102400k test 2>&1
> > 1>/dev/null
> > <TIDC_I Nov 17 13:21:13> ramdisk cmd: /usr/sbin/lofiadm -a test
> > <TIDC_I Nov 17 13:21:13> ramdisk cmd: /usr/sbin/newfs -m 0 -o space -i
> > 16384 0</dev/null 2>&1 1>/dev/null
> > <TIDC_I Nov 17 13:21:13> ramdisk cmd: /usr/sbin/mount -o
> > nologging /tmp/11 2>&1 1>/dev/null
> >
> > AI DC test is ok, this time boot_archive_archive.py uses default nbpi.
> >
> > Also libti_pymodule/libti.c is brought into accord with cstyle standard.
Thanks Jan,
resubmitting code review after fixing reviewed lines:
http://cr.opensolaris.org/~alhazred/8205/
--
::alhazred
