Joe, Per your and Dave Miner's recommendations, removed test driver from Makefile and repo - hg is very good for recovering this information later. Please review my revised webrev. Thank you, William
Joseph J VLcek wrote: > Great! Thanks > > William Schumann wrote: >> JV, >> >> jv35168 wrote: >>> 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" >> Done. >>> >>> Also as you suggest, removing it from the Makefile might make sense. >> I think that in the short term we can keep it around. The test >> drivers are not being built by nightly. >>> >>> 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? >> Indeed - appropriate warnings are made elsewhere. Removed. >> Thanks, >> William >>> >>> >>> >>> 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 >>> >
