Looks OK to me. Joe
William Schumann wrote: > 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 >>>> >>
