Jack, As we discussed offline, bug 5632, which allows install slice selection, has many common code changes with bug 4195, so would you please review 4195 instead? 4195 is a general bug that covers a range of AI engine changes for SPARC. All of your previous comments are still relevant.
http://defect.opensolaris.org/bz/show_bug.cgi?id=4195 http://cr.opensolaris.org/~wmsch/bug-4195/ My responses can be found below. I took all of your suggestions, except for get_manifest_element_array(), which I'm keeping for near future use. My apologies for withholding source code from the webrev. auto_install.c provides the missing links you asked about. I updated bug 4195 to explain other code changes. William Jack Schwartz wrote: > HI William. > > Here are my comments: > > General: Need to update copyrights to 2009. > > ai_manifest.rng: OK > > ai_manifest.xml: OK > > auto_install.h: OK > > auto_parse.c: > > get_manifest_element_array() is not used and can be deleted. > This is a generic routine that is going to replace many separate routines that get manifest array information very soon. > 282: Where is adi->install_slice_number used? I see it declared in > auto_install.h, set here, but don't see it anywhere in the slim_source > gate nor > in any other files of the webrev. Can it, plus the code from 276-283 be > removed? This seems required though... Is there a file missing from the > webrev? > > disk_slices.c: > > 403: Comment is incorrect. It creates, not protects, a slice. > > 476: Can this just be changed to > if (is_root) > since an install is always going to be on a partition marked V_ROOT, right? > > orchestrator_api.h: > > install_slice_id is declared here as part of disk_info_t, but > all instances of install_slice_id in disk_slices.c are local variables and I > don't see this variablename anywhere else in the webrev or in the > slim_source > gate. Can it be removed? > > slim_install.c: > > 508: Why '\0' and not just plain old 0? The last param to > prepare_zfs_root_pool_attrs is a uint_8, a sort of int, technically not > a char. > > 938: Header for do_ti() is missing. I looked for it because I wanted to > verify > that the header said something about an install slice being pre-determined > before do_ti() is called (since 1028-1034 checks for this). Some other > functions in this module are also missing headers. > > 972-974: Comment here is incorrect and can be deleted. > > Generally: > > I see a few large problems, or else I'm majorly confused... > > The first problem is that I don't see target_device_slice_number being used. > I see auto_parse.c bringing in the target_device_slice_number from the > manifest > and setting it into adi->install_slice_number, but I don't see > adi->install_slice_number being used, either in the webrev or in the > gate. Please > describe the flow between where target_device_slice_number is extracted from > the manifest and when it is used. > > Another problem I see: > om_finalize_vtoc_for_TI() has an extra argument, but I don't see where > calls to om_finalize_vtoc_for_TI are changed to handle that extra argument. > Are there files missing from the webrev. > > If you prefer we can take this offline. Please let me know. > > Thanks, > Jack > > William Schumann wrote: > >> [repeating request for code review for 2009 SPARC release] >> >> This allows the user to specify the number of a slice to install to on >> the target disk. Optional, defaults to slice 0. >> >> http://defect.opensolaris.org/bz/show_bug.cgi?id=5652 >> http://cr.opensolaris.org/~wmsch/bug-5652/ >> >> Added Relax NG element: >> ai_manifest/ai_target_device/target_device_slice_number >> >> Added generic function to replace 6 separate functions for processing >> ai_target_device elements. >> >> Fixed bug in auto_parse_sc_manifest() where it was falling off the end >> of the function without returning a value. >> >> Regression tested liborchestrator with GUI. >> William >> >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >> >> > > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >
