Hi Jean, William,
please see my comments below for slim_source changes.
Could you also please attach test procedures which were
used to validate the changes ?
Thank you,
Jan
auto_parse.c
------------
264, 313 - what is the purpose of those 'assert' sections -
what scenarios are we trying to catch there ?
274 - what is the added value of this error message appended to
the previous one ? Since printing value of pointer,
I am not sure it would provide useful information to the user.
ict.c
-----
Looking at installgrub(1M) man page, I am wondering if we could
take advantage of '-f' option in order to avoid problem with
interactive behavior:
...
-f Suppresses interaction when overwriting the master
boot sector.
...
I might recommend to slightly restructure the command to improve
readability, for instance:
...
(void) snprintf(cmd, sizeof (cmd),
"/usr/sbin/installgrub -m %s %s/boot/grub/stage1"
" %s/boot/grub/stage2 /dev/rdsk/%s",
om_install_partition_is_logical() ? "-f" : "",
target, target, device);
...
I am also wondering if it is intentional that installgrub itself
is run from "/", but stage1 and stage2 are taken from target.
I would assume that all would be taken from one place - either
target or "/".
disk_part.c
-----------
...
34 #ifdef __sparc
35 #define fdisk_is_dos_extended(p) (B_FALSE)
36 #else
37 #include <libfdisk.h>
38 #endif
...
Do we need fdisk_is_dos_extended() at all ? Could we instead use
check for (TD_PART_ATTR_PART_TYPE == TD_PART_ATTR_PART_TYPE_EXT) ?
orchestrator_api.h
------------------
nit:
169 uint8_t partition_id; /* fdisk id (1-36) */
->
169 uint8_t partition_id; /* fdisk id: 1 -
(FD_NUMPART + MAX_EXT_PARTS) */
target_discovery.c
------------------
728 - I might recommend to check if 'part' is in range, so that
we could avoid memory corruption if something goes wrong.
727 - I can see that the mechanism how index into array is calculated was
changed - now it is calculated from cXtXtXpN as N-1.
What happens if there are gaps - e.g. we have p1 and p3, but no p2 ?
For instance:
root at test-125:~# /opt/install-test/bin/test_td -p all -v
Partition discovery for all disks
---------------------------------------------------------------------
num | name| active| ID| lswp| 1st block|#of blocks|size [MB]|
---------------------------------------------------------------------
1 | c2d0p4| No| 83| No| 27310500| 4819500| 2353|
2 | c2d0p3| No| 05| No| 25687935| 1622565| 792|
3 | c2d0p1| Yes| BF| No| 16065| 25165824| 12288|
---------------------------------------------------------------------
...
1259 /*
1260 * logicals come after primaries
1261 */
...
I don't think it is always assured - see example above - extended
partition (and thus logical volumes) might precede primary partition
on the disk.
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
