Hi Jan -
Thank you for the code review. See my responses inline.
ginnie
>
>
> ------------------------------------------------------------------------
>
> Subject:
> Re: [caiman-discuss] Code review for the Install on Extended
> Partitions project
> From:
> Jan Damborsky <Jan.Damborsky at Sun.COM>
> Date:
> Wed, 14 Oct 2009 14:14:40 +0200
> To:
> jeanm <Jean.McCormack at Sun.COM>, Virginia.Wray at Sun.COM
>
> To:
> jeanm <Jean.McCormack at Sun.COM>, Virginia.Wray at Sun.COM
> CC:
> caiman-discuss <caiman-discuss at opensolaris.org>
>
>
> Hi Jean, Ginnie,
>
> please see my comments below for libdiskmgt changes.
> (I will also review slim_source part - comments will
> be sent in separate email).
>
> Thank you,
> Jan
>
>
> libdiskmgt.h
> ------------
>
> just a nit:
>
> 124 }dm_partition_type_t;
> ->
> 124 } dm_partition_type_t;
Corrected.
>
>
> partition.c
> -----------
>
> 371 - it seems that 'j' variable is not needed as it is just (i + 1) -
> the code might be slightly simplified then:
>
> 354 int i, j;
> ...
> 371 for (i = 0, j = 1; i < TOTAL_NUMPART; i++,
> j++) {
> ...
> 380 (void)
> snprintf(part_name,
> 381 sizeof
> (part_name),
> 382 "%s%d", pname,
> j);
> 383 } else {
> 384 (void)
> snprintf(part_name,
> 385 sizeof
> (part_name),
> 386 "%d", j);
> 387 }
> ...
>
> ->
>
> 354 int i;
> ...
> 371 for (i = 0; i < TOTAL_NUMPART; i++) {
> ...
> 380 (void)
> snprintf(part_name,
> 381 sizeof
> (part_name),
> 382 "%s%d", pname,
> i + 1);
> 383 } else {
> 384 (void)
> snprintf(part_name,
> 385 sizeof
> (part_name),
> 386 "%d", i + 1);
> 387 }
>
Good point. It is less complex. I've implemented your suggestion.
> Could you please also add comment before this block of code explaining
> what
> this code does ?
I've added a comment
>
> I assume these changes are addressing bug 6539687. If this is the
> case, I think
> that the same modifications should be done in function
> partition_get_assocs() -
> lines 193-199
I've added the functionality there.
>
>
> nit:
> 582 memset(&iparts[i], 0, sizeof (iparts[i]));
> ->
> 582 memset(&iparts[i], 0, sizeof (struct ipart));
>
>
> 433-451
> I am wondering if it might be possible to provide DM_PARTITION attribute
> for Sparc as well (I assume it would be always set to DM_PRIMARY).
> Then we wouldn't force the consumer to distinguish between Sparc and x86.
Yes....this is a good idea. I've added that.
>
>
>
> jeanm wrote:
>>
>> Please review the following for the installation on extended
>> partitions project:
>>
>> For the ON gate (questions should be directed to Ginnie Wray):
>>
>> http://cr.opensolaris.org/~ginnie/libdisk3/
>>
>>
>> For the slim_source gate (questions for libtd to Ginnie, rest of the
>> code questions should be directed to William Schumann)
>>
>> file:///net/indiana-build.central/export/home/ws199450/extp/webrev/index.html
>>
>>
>>
>>
>> Please respond by COB on Friday October 16.
>>
>> Thanks,
>>
>> Jean McCormack
>>
>>
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://mail.opensolaris.org/pipermail/caiman-discuss/attachments/20091020/7c63b879/attachment.html>
