Ethan,
Thanks for the review comments.
Ethan Quach wrote:
> Sarah, Sundar,
>
> Finally got a chance to go through the design, I've got some general
> comments below:
>
>
> 3.0:
>
> Should hostname be listed here, since the GUI is going to prompt for
> this?
I don't think we are prompting for the hostname.
>
>
> 4.1:
>
> om_content_type_t enum:
> - why only three types? ~or~ if this enum is used to delineate
> between the Solaris and LinuxSwap parts that share the same ID,
> why is Linux in here?
That is just an example. We are planning to identify the contents if
possible. For dwarf, it might be only linux swap and Solaris.
>
> SliceInfo_t struct:
> - the mountPoint member doesn't seem to belong in this struct.
> These Disk/Partition/Slice structures all seem to hold data
> describing physical attributes. Knowledge of where a slice is
> mounted should be at the software or OS level. Is there a
> reason why you choose to put this here?
I agree. It was added initially to help to find out whether root was
mounted and intend to be mounted on this slice. Also to see what is
mounted on each slice.
>
> upgradeInfo struct:
> - don't we need a type specifier the specify the type of union?
Can you explain?
> - nit: zonesConfigured should be changed to zonesInstalled
Configured sounds better since for creating zones, you are not
installing from a media.
> - Would it make more sense to make the "whyUpgradeNotAllowed"
> and "incorrectZoneList" members be integer error codes instead
> string pointers?
Yes. It will be changed.
>
>
> 4.1.2:
>
> om_initiateTargetDiscovery()
> - The sentence beneath the FALSE return value description is
> superfluous. The fatality of the error should be a design
> decision decided on by the consumer.
Agreed. Wording will change
>
> om_validateDiskPartition() and om_validateUpgradeTarget()
> - should set string pointers instead of message id pointers.
> Otherwise you'd have to expose another interface for the
> consumer to get the error message:
> get_<blah_type>_err_message(messageID).
Agreed.
>
> om_releaseTargetData()
> - does this function release the *copies* of the cache data
> which were returned to the caller in the various om_get***()
> functions? If so, this seems dangerous. The consumer is
> allowed to make modifications to these copies that were
> returned, but then a call to om_releaseTargetData() free's them.
> The consumer may have handles to these objects or their members
> floating around.
>
> Would it make more sense to have om_releaseTargetData() release
> just the cache data, and then provide additional free'ing
> functions for the consumer to call to free their copies of the
> objects?
Yes. That is what we will be doing. Check the next version of the document.
>
>
> 4.2
>
> Will upgrade analysis (space analysis) take any significant amount of
> time in dwarf? i.e. would it need a progress reporting as well?
Possible because it will be using the current install code. Since we may
support upgrading only from SDXE, it may not take much time. But your
point is valid.
Thanks,
Sundar
>
>
> 4.3.2
>
> om_getInstallLanguages()
> - Error Handling. I'm unclear as to what is being returned in
> the error case. We're returning a non-NULL list of on_locale_t
> objects with just the 'language' parameter set to NULL. Is the
> 'locale' member valid? If so, what for and why not just set the
> passed 'locales' parameter to NULL for the error case?
>
> om_getKeyboardTypes()
> - the overloading of this functions return code is assuming what
> the consumer is intending to do with the list of keyboard types
> returned. Can we instead separate this out into two functions:
> om_isSelfIDKeyboard()
> om_getKeyboardTypes()
>
> om_free()
> - How do we know what type of object is being passed in to
> free?
>
>
> 5.1
>
> Why do we need to set dummy values for timezone, timeserver, and locale?
> This indicates to me that the sysidtools are going to run before the
> dwarf GUI runs. Why? The individual sysidtools don't run outside of
> the GUI. The orchestrator is going to dictate which sysidtools run and
> when. So we could:
>
> a) fully populate a sysidcfg with the data gathered from the
> GUI and the data from the default sysidcfg file, and then run
> all the sysidtools to configure the apporiate files.
>
> b) not run the sysidtools at all, and create new functions in
> the orchestrator to configure the system with the data.
>
> c) a mixture of both.
>
>
> Sarah Jelinek wrote:
>>
>>
>> Ethan Quach wrote:
>>>
>>>
>>> Dave Miner wrote:
>>>> Niall Power wrote:
>>>>> More comments/questions after having spent more timing examining the
>>>>> disk layout modelling:)
>>>>>
>>>>> 4.1.1:
>>>>> The data model assumes that only one VTOC (DiskSlices_t) can exist
>>>>> on disk. On X86, the DiskSlice_t is associated with a parition.
>>>>> Will the assumption on X86 that only one partition can contain a
>>>>> VTOC hold true
>>>>> in the future? Will ZFS make this irrelevant?
>>>>>
>>>>
>>>> It's likely to hold true for a while, at least; there aren't any
>>>> firm plans that I'm aware of which would change this.
>>>
>>> But the configuration exists nonetheless. People have hacked their
>>> way into creating a disk with multiple solaris fdisk partitions, each
>>> with an instance of solaris installed.
>>>
>>> I suppose how we want to handle this in dwarf is the question. Right
>>> now, the existence of two solaris fdisk partitions on a disk is
>>> detectable, but only one VTOC is mapped by the driver.
>>>
>>> The current GUI installer doesn't handle this very gracefully (makes
>>> you delete one.) Are we doing the same here in dwarf?
>>
>> Actually, we will be talking about just this issue in todays meeting.
>> It is supposed to be only 1 Solaris fdisk partition, but as you note
>> people have hacked there way in to other configurations.
>>
>> sarah
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://opensolaris.org/mailman/listinfo/caiman-discuss
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://opensolaris.org/mailman/listinfo/caiman-discuss
