William, I've looked over these changes and they all look fine to me. I only have a couple of miner issues.
Comments: Regarding the orchestrator test driver. I realize it is not being used at this time. But it could be again in the future. Please update bug 3112 to describe how the changes to the API, which you are making, will impact the liborchestrator test driver. "3112 liborchestrator test driver should be enhanced to address QA requirements" Also as you suggest, removing it from the Makefile might make sense. Question: usr/src/lib/liborchestrator/disk_slices.c 888 om_debug_print(OM_DBGLVL_WARN, "failed to delete slice %d from table\n", Is it possible on line 888 that the slice was already deleted, much like on line 515? Joe William Schumann wrote: > Joe, > Upon testing, I noticed a problem when the Solaris partition is > deleted and re-created at another location. In that case, the slice > information collected at TD is no longer valid and should be > invalidated. Added code in disk_slices.c to invalidate the slice > information. > I also decided to include the fix for 6628 installation shouldn't fail > when trying to delete nonexistent slice. > http://defect.opensolaris.org/bz/show_bug.cgi?id=6628 > I extended this to include partitions, so that if you specify a > partition or slice for deletion and it is not there, only a warning is > issued to the log file and the installation proceeds. > > Retested on SPARC and x86, deleting and recreating partitions and slices. > > Responses to your comments are below. > > Joseph J VLcek wrote: >> William Schumann wrote: >>> The AI manifest can specify partitions existing on a disk to be >>> deleted. Currently there is only support for deleting partitions if >>> you know the starting offset sector and length, which must match >>> those for the partition in fdisk(1m), (which you can find from what >>> 'fdisk -W - /dev/rdsk/cxtxdxp0' dumps). >>> >>> This provides support for deleting partitions using the manifest if >>> you know only the partition ID - (1-4) >>> >>> Affects x86-only >>> >>> http://defect.opensolaris.org/bz/show_bug.cgi?id=5654 >>> http://cr.opensolaris.org/~wmsch/bug-5654/ >>> >>> Tested: >>> - deleted multiple partitions and added new partitions >>> - tried to delete partition that doesn't exist >>> - regression tested deleting by starting sector + length >>> _______________________________________________ >>> caiman-discuss mailing list >>> caiman-discuss at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >> >> William, >> >> Things look good. I only have some nits... >> >> Hope this helps! Joe >> >> >> lib/liborchestrator/test_driver.c >> --------------------------------- >> >> Invocation to om_delete_partition needs to be updated to include the >> new parameter. > We are no longer maintaining test_driver.c officially. I have not yet > removed it from the Makefile, but it is neither built or included in > any packages at the moment. >> >> usr/src/lib/liborchestrator/disk_parts.c >> ---------------------------------------- >> >> Issue 1: to be consistent FD_NUMPART should be be OM_NUMPART on lines: >> >> 190 for (ipart = 0; ipart < FD_NUMPART; ipart++, pinfo++) { >> 1165 for (ipart = 0; ipart < FD_NUMPART; ipart++) { >> 1191 "%d (0-%d) for deletion\n", ipart, FD_NUMPART - 1); >> 1194 (FD_NUMPART - ipart - 1) * sizeof (*pinfo)); >> 1196 set_partition_unused(&pinfo[FD_NUMPART - 1]); >> > Changed. >> Issue 2: Question/Idea >> >> Wouldn't the old post-delete om_debug_print loop be valuable prior to >> returning from function om_delete_partition? >> >> 1144 for (ip = 0; ip < OM_NUMPART; ip++) >> 1145 om_debug_print(OM_DBGLVL_INFO, >> 1146 "post-delete dump: part_id=%d size=%d\n", >> 1147 pinfo[ip].partition_id, >> 1148 pinfo[ip].partition_size); >> >> Making this block of code a macro could keep the code clean... Just >> an idea... > When you look at the log, there seems to be enough debugging > information, and the complexity of the code does not warrant lots of > debugging info in the log, I think. > > Joe, > Would you please review the additional changes? > Thank you, > William
