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
>
