Hi William,
William Schumann wrote: > 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. To be honest, I am not sure if this is the right thing to do, since errno != 0 wouldn't necessarily imply that there is a problem we neglected to handle. In general, the scenarios where some calls fail and set errno might be valid and handled appropriately and there is no policy which would say errno should be reset to zero in those cases. >> >> 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. You are right - it is my oversight - it should have been instead: ... (void) snprintf(cmd, sizeof (cmd), "/usr/sbin/installgrub %s %s/boot/grub/stage1" " %s/boot/grub/stage2 /dev/rdsk/%s", om_install_partition_is_logical() ? "-mf" : "", 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 "/". > 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? In general, I think that all three pieces should be taken from the same place - either "/" or target, since they are related. It is likely just a cosmetic issue :-) >> >> >> 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. It might be redundant, but it allows to follow consistent approach for obtaining information about target - use Target Discovery (TD) as our channel for obtaining that data. >> >> 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? Yes, please. >> >> 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. Even if current Solaris fdisk can't create gaps, those can be created by Linux fdisk command and its clones - e.g. by deleting the 2nd primary fdisk partition in the case above. > 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. As we discussed, sort_partitions_by_offset() function will not work correctly - it will sort less entries in case gaps are present. Also GUI part might be the source of problem, since the original code didn't allow gaps in partition_info_t structure. >> >> >> ... >> 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. I see - it is little bit confusing as the first half of function compares partitions by partition number and the second half compares by position on disk. Jan
