Dave:
Thanks for the comments.
Dave Miner wrote:
> Comments on the Orchestrator design, v 0.3:
>
> A general comment is that I think the memory management and error
> return/messaging scheme needs some work to feel clean. A number of
> specific API comments below reflect this, but it really is a global
> issue in the design.
>
> page 6, section 3.0 item 12: the text notes that "[t]his is a temporary
> piece of functionality...". It would be useful to note where this
> functionality will ultimately reside. Overall, within this list I think
> there are a number of other items which have the same property
> (keyboard, locales, user accounts) and thus it might make more sense to
> have a paragraph here which discusses this issue more generally.
>
> page 7, section 4.1: Perhaps I missed something, but I don't recall
> support of upgrading instances installed on SVM as being on the planned
> feature list, so what are we doing with this information? Also, we
> should be using "GPT" here instead of "EFI", right?
The SVM information is recorded here so that we can provide a useful
explanation to the user why some instances are not upgradeable. It
should be GPT . I will fix that.
>
> page 9: structure members meant to represent sizes should be uint64_t to
> help future-proof things. Also, assuming the partitionType is the
> indentifier code (e.g. 0xbf for Solaris2), wouldn't it more properly
> be a uint8_t?
okay.
>
> page 11, 4.1.2, om_initiateTargetDiscovery: the description says that
> the "progress of the callback will be returned..." when it should say
> "progress of the discovery will be returned...". Also, the text here
> says, "If target discovery cannot be started the installation
> application must exit." I think it's fundamentally incorrect for a
> component to be dictating that its caller must react in any particular
> way to a failure, certainly not in a fashion so drastic as "must
> exit". Whenever possible, the installation application and its
> supporting functions should be written to allow for the user to
> attempt to re-initiate a process, not exit and tell them "type
> suninstall to try again" or somesuch. Finally, how does the caller
> obtain details on the reason for a failure?
With out the disk data, we can't proceed. We want to provide as much
information as possible to the dwarf GUI so that what action needs to be
taken. The document needs more polish when it comes to Orchestrator Disk
Discovery functions (pointing to me :-) ).
>
> om_getDiskInfo: what happens if it's called while discovery is still
> in progress? And see comment above about dictating caller's behavior.
If it called while discovery still in progress, it will return an error
indicating that 'Target discovery is not completed yet'. I will update
the document.
>
> om_getDiskPartitionInfo: why not just take a DiskInfo as the first
> argument? And what is meant by "A failure will be fatal in this case"?
The DiskInfo structure returned from om_getDiskInfo is a linked list of
DiskInfo. Why the whole structure needs to be passed back when diskName
is enough to get the partitions.
>
> om_validateDiskPartitionUpdate: if we're going to use this as a
> boolean predicate, I'd suggest a name which reads that way, such as
> om_isPartitionInfoValid or something of that nature. Also,
Okay.
>
> om_getSolarisInstances: there's no way to free the space this
> allocates other than to commit the installation? What happens if the
> user goes back to the beginning of the installer, do we just leak that
> memory?
No memory leaks :-( . All allocated memory is kept on track by the
Orchestrator. Initially we were planning not to have any free functions.
The memory is freed when the Orchestrator detects that it is no longer
needed (like user commits to install/upgrade). There are always
possibilities that using these functions may not lead to commit to
install/upgrade. So we decided to add a generic function to free all the
allocated memory.
>
> om_validateUpgradeTarget: why shouldn't this be a boolean predicate a
> la the partition validation, in which case I'd suggest
> om_isTargetUpgradeable or similar? Also, I don't really understand
> the comment here about two-step validation - does this mean that
> om_getSolarisInstances is only returning some concept of a "valid"
> Solaris instance (which isn't defined there)?
Okay.
>
> om_releaseTargetData: this seems to have an excessively drastic
> effect; shouldn't we provide some slightly less drastic ways of
> freeing up results that we don't need?
All the allocated data are either disk specific or instance specific. I
don't understand the point of providing specific functions to free
partition and/or slice data. This only frees the data allocated by the
Orchestrator on behalf of the consumer. The Orchestrator cache will be
freed only when there is a subsequent TargetDiscovery() call or commit
to install/upgrade.
>
> 4.2.1 progress reporting: what about a progress reporting interface
> that can work in both a Gnome-friendly way as well as not requiring
> Gnome? For the current iteration a Gnome dependency would seem fine,
> since that's the only immediate consumer, but something to think about.
>
> 4.3 the list here has two #1 entries. Also, can you clarify what
> "User data" here means?
>
> 4.3.1 So where's the keyboard structure?
>
> 4.3.2 om_getInstallLanguages: there's no existing API that provides
> the locale list?
>
> om_getKeyboardTypes: again, no existing API? And specifying what the
> GUI will do here in reaction to a particular return is wrong; that
> belongs in the GUI spec, not here.
>
> om_getAvailableLocales: See earlier comments about specifying
> application behavior, but why would a null list of locales be fatal?
> "C" always works, and there seems no harm in proceeding in that case
> so long as the user's happy.
>
> om_getMinSize: The name here seems much too imprecise to describe what
> it's doing. Also, it feels quite incorrect to return a random value
> like 10 GB when we can't calculate something; I'd think we'd like an
> API which can take a described set of software and a disk layout and
> validate the two against each other. For this project the two are
> basically fixed, but at least thinking in that direction here would be
> nice.
>
> om_free: this seems impossibly imprecise; how is it going to know
> whether it's supposed to free a list of keyboards, a list of locales,
> or something else entirely?
>
> 4.4 om_performInstall: here's the first mention of nvpairs, but how is
> the caller supposed to create them without knowing the names to use
> for the values? Are there any requirements on ordering of the pairs
> or anything?
This function is not refined yet. We are working on nvpair and call back
functions. we will update it once our investigation is complete.
Thanks,
Sundar
>
> Dave
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://opensolaris.org/mailman/listinfo/caiman-discuss
