Hi Alex,
I have couple of additional comments related to
the updated webrev - please see below.
As far as testing is concerned, since the change was activated
for standard DC build, I would recommend to test with new
DC, build LiveCD and AI images and give them a try.
If you would like, I could build the images and take a look
at them, since I have all infrastructure already in place.
Cheers,
Jan
test_ti.c
---------
Since TI_ATTR_DC_RAMDISK_BYTES_PER_INODE is now optional,
we should pass it only if specified:
1303 if (!dc_ramdisk_release &&
->
1303 if (!dc_ramdisk_release && (dc_ramdisk_nbpi != 0)
&&
ti_api.h
--------
293 /* uint_32_t - # of inodes to be created */
->
293 /* uint_32_t - # of bytes per inode */
ti_dcm.c
--------
Since TI_ATTR_DC_RAMDISK_BYTES_PER_INODE is now optional,
it is not error if not provided:
160 ls_write_dbg_message(TIDC, LS_DBGLVL_ERR,
->
160 ls_write_dbg_message(TIDC, LS_DBGLVL_INFO,
Alexander Eremin wrote:
> 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/
>
>
