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? 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? 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? om_getDiskInfo: what happens if it's called while discovery is still in progress? And see comment above about dictating caller's behavior. 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"? 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, 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? 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)? 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? 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? Dave
