Jan, Please find my responses inline, including questions. Jan Damborsky wrote: > 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 ? During my initial tests, I was seeing errno as non-zero coming into the routines. I was unable to reproduce it, but I still want to know when and if it occurs, so I added the assertions as a way to try to catch the condition. > > 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. > The second one was accidental. Will remove. > > 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); Yes, the '-f' option would increase readability. The example you give, however, includes the -m option unconditionally, which forces GRUB to be installed in the MBR (master boot record). The requirement for writing GRUB to the MBR is necessary only for booting from logical partitions. In fact, some users will not like being forced to have GRUB installed on the MBR, and I will file an RFE to this effect. > ... > > 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 "/". It seems safer to run the installgrub in "/", which is certain to be native to the environment. Why do you think installgrub should be taken from the target? > > > 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) ? I coded this before the TD attribute was available. It is still not included in partition_info_t, so it would have to be added, which I would hesitate to do because it is redundant information easily derived by the libfdisk macro. The macro provided by libfdisk is also more readable. > > > > 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) */ Will change. > > > > 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. I can add this. > > 727 - I can see that the mechanism how index into array is calculated was > changed - now it is calculated from cXtXtXpN as N-1. Yes, does this need to be more clearly documented? > > 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| > --------------------------------------------------------------------- AI will allow gaps as you mention (p1 and p3, but not p2). There is an ancient bug in fdisk that prevented such gaps from appearing. I used a test fdisk binary that has a fix for this, and the behavior was correct - the gaps in partition numbering are preserved.
I just tested test_td against a disk format with these gaps. The output is correct for me, but this is definitely not exhaustive testing. I will consult with Ginnie on this. > > > ... > 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. I meant this in order of the partition P number. p1-p4 are always primary/extended, p5-p36 are always logical. Will modify comment to make this clearer. William > > > > 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 > >
