Hi William.
Here are my comments:
--- ai_manifest.rng:
I know you check that slice number is between 0-7, and this used to be
the old range. However, slices on my X86 system, go to 15. It may be
that slices 8-15 are special. Have you checked out whether or not you
need to extend the slice range to 15? If indeed you do need to change
the range to allow double-digits, the code at 281-282 of auto_parse.c
will need to change.
--- ai_manifest.xml: same as for bug 5652
--- auto_install.c:
32: Can remove. strings.h includes string.h
Since fdisk partitions are an X86 thing, 346 - 356 should be inside
#ifndef __sparc.
install_from_manifest sets install_slice_id from adi.install_slice_number.
its used in auto_modify_target_slices() line 286.
45, 278, 456: install_slice_id global variable isn't needed if you pass
it into auto_modify_target_slices() directly. Just declare it local to
install_from_manifest()
--- auto_install.h: Same as for bug 5652
--- auto_td.c: OK
--- auto_parse.c:
281-282: Why not just sscanf (p, "%d", adi->install_slice_number) ?
This would allow for expansion into multiple-digits in the future
without change or need to debug; tt would just work.
508: I know this is old code, but returning AUTO_INSTALL_SUCCESS here
restricts the number of successful tokens read and parsed to one. Is
this what is desired? Perhaps the intent was to replace the return on
508 with the one you added on 510?
What's the difference between
ai_manifest/ai_device_vtoc_slices/slice_number (returned from
ai_get_manifest_slice_number() and
ai_manifest/ai_target_device/target_device_slice_number (returned by
ai_get_manifest_disk_info? Can you just use the former in the manifest,
in effect treating the SPARC disk as one disk-wide "fdisk partition"?
That would save mods to the ai_manifest schema and xml files, and would
likely simplify the code.
--- disk_slices.c:
30: Can remove. strings.h includes string.h
462: Nit: comment is incorrect. Should be "If slice size is zero..."
561: I don't understand the interplay between install_slice_id and the
slice_edit_list. What if the latter reflects a different install slice
than install_slice_id? Actually, perhaps only the comment on 560 is
incorrect, since a slice is specified (by install_slice_id) if
use_whole_partition_for_slice_0 is false.
572: This line can be omitted.
1085: Nit: comment is incorrect. Should mention if not SPARC or if no
partition is defined.
--- orchestrator_api.h
168: I don't see where this is used. Please delete if not necessary.
--- perform_slim_install.c
106 and others: Please verify that these cannot be declared static.
Globals can produce hard-to-find bugs due to namespace issues. I
checked, and zfs_device can be kept to module scope, which helps. Not
sure about the other variables here and elsewhere; please check.
508: Just curious: how come the slice ID in the call to
prepare_zfs_root_pool_attrs here is hardwired to zero?
993 - 1001: Do you need to get disk name here, since it was just got by
the call on line 976? If yes, please correct the comment at 993 and say
why the call on 995 is needed in the X86 case.
As for flow, here's how I think it works:
Extraction and use:
auto_install.c/install_from_manifest calls
auto_parse.c/ai_get_manifest_disk_info
disk_slices.c/om_finalize_vtoc_for_TI
disk_slices.c/om_create_slice
installs in committed_disk_target->dslices->sinfo->slice_id
installs in slice_edit_list[slice_id] = B_TRUE
perform_slim_install/do_ti() calls
om_get_device_target_info extracts from slice_edit_list.
prepare_zfs_root_pool_attrs to set it into zfs_device and into
ti_ex_attrs.
calls ti_create_target with ti_ex_attrs.
I'm still curious to see how
ai_manifest/ai_device_vtoc_slices/slice_number fits into the picture.
Thanks,
Jack
William Schumann wrote:
> 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
>>
