Sundar Yamunachari wrote: > 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.
We are allowing the user to set the hostname for the system. The default is solaris-devx, as it is now. This will be added to the document. >> >> >> 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. We will provide upgrade progress as well. This isn't clear in the document and we will add this. >> >> >> 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? Actually, both members will be set in the non-NULL list case. The text describing this is not very clear and I will fix. The original parameter name for the function was 'languages'. Then I changed it to locales, but didn't update the text. I will fix. >> 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() Fair enough. I can separate these two pieces of functionality out. >> om_free() >> - How do we know what type of object is being passed in to >> free? This is being totally re-designed. We should have an updated design doc soon with better memory management functions. >> >> 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. My thoughts on this were that we have not disabled the sysidtools fully in any release of the installer to date. So, doing this by modifying the install scripts to not run the sysidtools might be problematic. We are not running any of the existing sysidtool functionality with Dwarf. The thought was that we would go through the same sequence with Dwarf as we do today with the installer, but mute the UI screens by having a sysidcfg file for all configuration attributes. I have no issue with not running the sysidtools, but am concerned about what this type of change means to Dwarf and thought it would be more straightforward to create the dummy sysidcfg file. thanks, sarah
