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.
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
>
