jeanm wrote:
>
>>
>> instantiate.c:
>>
>> -General: I am a bit confused here. Why do we have libti code in the
>> libtarget_pymod library? I know we need to translate the text
>> installer objects to libti type of attributes, but in looking at the
>> libti_pymod, it seems that there are already functions in there to do
>> what we need to do. Specifically, py_ti_create_target(). You would
>> have to translate the text installer object tuples to the TI
>> attributes and create a new tuple. Is there a reason you felt that
>> setting up the nvlist and calling ti_create_target() here was
>> required, instead of setting up the tuple and calling the libti_pymod
>> functions? If there are gaps in the libti_pymod code, or you plan to
>> merge this in, can you please file a bug to move/merge this code with
>> that as we move forward?
>>
> The idea is to make the python extensions that deal with the target
> (td, ti) be in the same place and use the same objects.
> The new ti interface also allows the new engine to not have knowledge
> of the ti module. i.e. it doesn't need to set up the nvlists etc. It
> just calls a function like create_disk_target and the ti knowledge is
> built into the extension module.
>
> My plan was to eventually get rid of the current libti_pymod code and
> have the current users use the new libtarget_pymod.
> There wasn't time (and it was too risky) to do that for this release.
> I will file a bug to move this code in the future.
Bug 13553 has been filed to address the removal of libti_pymod and any
associated modifications to DC and any others that use it.
Jean
>
> Jean
>
>>
>> partition.c:
>>> 33 /*
>>> 34 * NOTE: libdiskmgmt has labelled partition data incorrectly.
>>> We correct it here
>>> 35 * in the Python names. TgtPartition data structure still
>>> use the
>>> 36 * libdiskmgmt naming scheme. Here is the conversion:
>>> 37 *
>>> 38 * libdiskmgmt | Python | example
>>> 39 * --------------+--------+----------------
>>> 40 * id | number | 1
>>> 41 * type | id | 0xBF (Solaris2)
>>> 42 * typetypething | type | ??? (Primary)
>>> 43 *
>>> 44 * libdiskmgt doesn't yet recognize anything but Primary
>>> partitions but will
>>> 45 * soon change.
>>> 46 */
>>
>> Two things.. if there are bugs in libdiskmgt, please file a bug
>> report. The last comment is no longer valid, so please remove.
>>
>>> 57 * XXX until extended partitions project goes back 1-4 are only
>>> partition
>>> 58 * "id" libdiskmgmt understands.
>>> 59 */
>>> 60 #define MAXID 4
>>> 61 #define STR_MAXID STRINGIZE(MAXID)
>>
>> This needs to be changed. And, we should use the fdisk defines for
>> this, not our own. Extended partition support has been integrated
>> into libdiskmgt.
>>
>>> 502 assert(result != NULL);
>>
>> We are asserting here that there are always slice objects as children
>> of a partition. Is this always true?
>>
>> General question:
>> -Is it possible to have more than one 'label' in python for goto's?
>> It seems like the answer is no, but I see that we have errors handled
>> sometimes inline and sometimes with a 'goto'. I actually find this
>> confusing. Especially, when we have an error label but we don't
>> always use it. For example:
>>> f (newblk % geo->cylsz != 0) {
>>> 603 PyErr_SetString(PyExc_ValueError,
>>> 604 "\"blocks\" must be a multiple of "
>>> 605 "tgt.Geometry.cylsz");
>>> 606 return (-1);
>>> 607 }
>>
>> Is there anyway to combine this with the other label and use a goto?
>>
>> Another general comment: Sometimes in the code we comment the
>> reference counting manipulation that is done, and sometimes we don't.
>> Perhaps we should be more liberal in the areas where it might be
>> confusing. For example:
>>
>>> 663 static PyObject *
>>> 664 TgtPartition_get_id(TgtPartition *self, void *closure)
>>> 665 {
>>> 666 PyObject *result = PyInt_FromLong((long)self->id);
>>> 667 if (result == NULL)
>>> 668 return (PyErr_NoMemory());
>>> 669 return (result);
>>> 670 }
>>
>> The call to PyIn_FromLong returns a new reference, but this may not
>> be obvious to other folks. Not a deal breaker, just a suggestion.
>>
>>> 40 #define MAXNUM 15
>>
>> Is there a define somewhere we can use that is more 'official'. Kind
>> of like what is provided in fdisk.h?
>>
>> slice.c:
>>
>> It looks like the definition for
>>> 200 #define SET_BOOL(nm) self->nm = (nm == Py_True) ? 1 : 0
>>
>> SET_BOOL is the same wherever it is used. I think it would be cleaner
>> to use #defines in the header files as required and only if the macro
>> is really different to the #define and #undef inline.
>>
>>> 214 * type: type object, do *NOT* assume it is TgtSliceType!
>>
>> This comment implies you are going to do some checking with regard to
>> the 'type' value passed in,but in the code it is simply
>> assumed to be a slice object:
>> 225 self = (TgtSlice *)type->tp_alloc(type, 0);
>>
>> But, it's certainly possible I misunderstand how tp_alloc() works.
>>> 439 if (self->user != NULL)
>>
>> This check isn't required.
>>> 518 return (PyLong_FromLongLong(self->blocks));
>>
>>
>> Sometimes you return the value from these calls, and sometimes you
>> check for NULL and return PyErr_NoMemory():
>>
>>> 550 if (result == NULL)
>>> 551 return (PyErr_NoMemory());
>>
>> It would be good if this was consistent, unless there is a specific
>> reason that this is different in some cases.
>>
>> zpool.c:
>> -The printfs are for debugging I presume? It would probably be better
>> ro have an explicit debug value that can be set and print accordingly.
>>> 114 if (self->mountpoint == NULL) {
>>> 115 PyErr_NoMemory();
>>> 116 return (-1);
>>> 117 }
>>
>> This type of error in this file seems like a good candidate for a goto.
>>
>> Nit:
>>> 219 * type: type object, do *NOT* assume it is TgtDiskType!
>>
>> This comment is wrong, it is a TgtZFSDataset type.
>>
>> libtd/td_dd.c:
>>> if (errn != 0 || slice_stats == NULL) {
>>> 1965 DDM_DEBUG(DDM_DBGLVL_ERROR,
>>> 1966 "ddm_get_slice_stats(): Can't get slice inuse
>>> data, "
>>> 1967 "for %s, err=%d\n", name, errn);
>>> 1968 return (errn);
>>> 1969 }
>>
>> Shouldn't this be a && here? If the err == 0, then returning and
>> error seems incorrect, even if the slice_stats are NULL. Not sure if
>> this can happen.
>>
>> libzoneinfo_pymod/libzoneinfo.c:
>>
>> -Kind of an unfortunate name for this, at first I thought that this
>> code had to do with Solaris Zones. maybe consider renaming. Not a
>> huge deal, just a thought. I suppose you modeled the name after the
>> system libzoneinfo?
>>
>> sarah
>> ****
>>
>>
>> Keith Mitchell wrote:
>>> Hi Glenn and all other reviewers,
>>>
>>> I have posted an updated webrev at:
>>> http://cr.opensolaris.org/~kemitche/text_v2/
>>>
>>> Additionally, the opensolaris.org project gate has been updated to
>>> match those changes.
>>> hg clone ssh://anon at hg.opensolaris.org/hg/caiman/text_installer
>>>
>>> Please disregard the initial webrev. The updated webrev contains the
>>> most recent changes project files, and some of those files have
>>> changed significantly since the first webrev. An updated breakdown
>>> of which files fall into which sections is below.
>>>
>>> Thanks,
>>> Keith
>>>
>>> Breakdown of file groupings:
>>>
>>> Installation Profile:
>>> cmd/text-install/osol_install/profile/__init__.py
>>> cmd/text-install/osol_install/profile/disk_info.py*
>>> cmd/text-install/osol_install/profile/install_profile.py
>>> cmd/text-install/osol_install/profile/ip_address.py
>>> cmd/text-install/osol_install/profile/network_info.py
>>> cmd/text-install/osol_install/profile/system_info.py
>>> cmd/text-install/osol_install/profile/user_info.py
>>>
>>> * The changes mentioned in previous emails are present in the
>>> updated webrev; please DO review this file
>>>
>>> UI Components:
>>> cmd/text-install/osol_install/text_install/action.py
>>> cmd/text-install/osol_install/text_install/base_screen.py*
>>> cmd/text-install/osol_install/text_install/color_theme.py
>>> cmd/text-install/osol_install/text_install/edit_field.py
>>> cmd/text-install/osol_install/text_install/error_window.py
>>> cmd/text-install/osol_install/text_install/inner_window.py
>>> cmd/text-install/osol_install/text_install/list_item.py
>>> cmd/text-install/osol_install/text_install/main_window.py
>>> cmd/text-install/osol_install/text_install/scroll_window.py
>>> cmd/text-install/osol_install/text_install/window_area.py
>>>
>>> * This file is also relevant to the "Screens" group
>>>
>>> Screens:
>>> cmd/text-install/osol_install/text_install/date_time.py
>>> cmd/text-install/osol_install/text_install/disk_selection.py
>>> cmd/text-install/osol_install/text_install/fdisk_partitions.py
>>> cmd/text-install/osol_install/text_install/help_screen.py
>>> cmd/text-install/osol_install/text_install/install_progress.py
>>> cmd/text-install/osol_install/text_install/install_status.py
>>> cmd/text-install/osol_install/text_install/log_viewer.py
>>> cmd/text-install/osol_install/text_install/network_nic_configure.py
>>> cmd/text-install/osol_install/text_install/network_nic_select.py
>>> cmd/text-install/osol_install/text_install/network_type.py
>>> cmd/text-install/osol_install/text_install/partition_edit_screen.py
>>> cmd/text-install/osol_install/text_install/summary.py
>>> cmd/text-install/osol_install/text_install/timezone.py
>>> cmd/text-install/osol_install/text_install/timezone_locations.py
>>> cmd/text-install/osol_install/text_install/timezone_regions.py
>>> cmd/text-install/osol_install/text_install/users.py
>>> cmd/text-install/osol_install/text_install/welcome.py
>>>
>>> Text Install python core:
>>> cmd/text-install/osol_install/text_install/__init__.py
>>> cmd/text-install/osol_install/text_install/disk_window.py*
>>> cmd/text-install/osol_install/text_install/screen_list.py
>>> cmd/text-install/osol_install/text_install/text-install.py**
>>> cmd/text-install/osol_install/text_install/ti_install_utils.py
>>> cmd/text-install/osol_install/text_install/ti_install.py
>>>
>>> * This subclass of "InnerWindow" has enough knowledge of Text
>>> Installer specifics that it can't be classified as a 'generic' UI
>>> component
>>> ** This is the 'main' program
>>>
>>> Image/Distro-const:
>>> cmd/distro_const/auto_install/ai_plat_setup.py
>>> cmd/distro_const/auto_install/ai_post_boot_archive_pkg_image_mod
>>> cmd/distro_const/auto_install/ai_sparc_image.xml
>>> cmd/distro_const/auto_install/ai_x86_image.xml
>>> cmd/distro_const/text_install/text_mode_sparc.xml
>>> cmd/distro_const/text_install/text_mode_x86.xml
>>> cmd/distro_const/text_install/tm_gen_cd_content
>>> cmd/distro_const/text_install/tm_generic_live.xml
>>> cmd/distro_const/text_install/tm_pre_boot_archive_pkg_image_mod
>>> cmd/distro_const/utils/grub_setup.py
>>> cmd/distro_const/utils/Makefile
>>> cmd/distro_const/utils/plat_setup.py
>>> cmd/distro_const/utils/post_boot_archive_pkg_image_mod_custom
>>> cmd/slim-install/finish/install-finish
>>> cmd/slim-install/svc/media-fs-root
>>> cmd/text-install/svc/text-mode-menu.xml
>>> cmd/text-install/text-mode-menu/text-mode-menu.ksh
>>>
>>> Library Changes:
>>> lib/libtarget_pymod/discover.c
>>> lib/libtarget_pymod/disk.c
>>> lib/libtarget_pymod/disk.h
>>> lib/libtarget_pymod/geometry.c
>>> lib/libtarget_pymod/geometry.h
>>> lib/libtarget_pymod/instantiate.c
>>> lib/libtarget_pymod/instantiate.h
>>> lib/libtarget_pymod/mapfile-vers
>>> lib/libtarget_pymod/partition.c
>>> lib/libtarget_pymod/partition.h
>>> lib/libtarget_pymod/slice.c
>>> lib/libtarget_pymod/slice.h
>>> lib/libtarget_pymod/tgt.c
>>> lib/libtarget_pymod/tgt.h
>>> lib/libtarget_pymod/zpool.c
>>> lib/libtarget_pymod/zpool.h
>>> lib/libtd/td_api.h
>>> lib/libtd/td_dd.c
>>> lib/libtd/td_dd.h
>>> lib/libtd/test_td.c
>>> lib/libti/ti_api.h
>>> lib/libzoneinfo_pymod/libzoneinfo.c
>>>
>>> Packaging/Makefiles: Package definitions, dependencies, makefiles,
>>> and so forth
>>> Makefile.master
>>> README
>>> Targetdirs
>>> cmd/Makefile
>>> cmd/Makefile.cmd
>>> cmd/Makefile.targ
>>> cmd/distro_const/Makefile
>>> cmd/distro_const/auto_install/Makefile
>>> cmd/distro_const/text_install/Makefile
>>> cmd/text-install/Makefile
>>> cmd/text-install/helpfiles/Makefile
>>> cmd/text-install/osol_install/profile/Makefile
>>> cmd/text-install/osol_install/text_install/Makefile
>>> cmd/text-install/svc/Makefile
>>> cmd/text-install/text-mode-menu/Makefile
>>> lib/Makefile
>>> lib/libict/Makefile
>>> lib/libtarget_pymod/Makefile
>>> lib/libtd/Makefile
>>> lib/libzoneinfo_pymod/Makefile
>>>
>>> Other: (Help Files, miscellaneous)*
>>> cmd/text-install/helpfiles/date_time.txt
>>> cmd/text-install/helpfiles/disks.txt
>>> cmd/text-install/helpfiles/network.txt
>>> cmd/text-install/helpfiles/network_manual.txt
>>> cmd/text-install/helpfiles/sparc_solaris_slices.txt
>>> cmd/text-install/helpfiles/sparc_solaris_slices_select.txt
>>> cmd/text-install/helpfiles/summary.txt
>>> cmd/text-install/helpfiles/timezone.txt
>>> cmd/text-install/helpfiles/users.txt
>>> cmd/text-install/helpfiles/welcome.txt
>>> cmd/text-install/helpfiles/x86_fdisk_partitions.txt
>>> cmd/text-install/helpfiles/x86_fdisk_partitions_select.txt
>>> cmd/text-install/helpfiles/x86_fdisk_slices.txt
>>> cmd/text-install/helpfiles/x86_fdisk_slices_select.txt
>>> cmd/text-install/osol_install/__init__.py**
>>>
>>> * The help text files are under separate review.
>>> ** This file is not packaged, as osol_install/__init__.py is
>>> delivered by SUNWinstall. It exists so that the UI can be run
>>> directly from the workspace.
>>>
>>> Glenn Lagasse wrote:
>>>> Hi Keith,
>>>>
>>>> * Keith Mitchell (Keith.Mitchell at Sun.COM) wrote:
>>>>
>>>>> All,
>>>>>
>>>>> As of yet, we've not received any code review comments for the first
>>>>> round of review. I realize that there is a lot of code, so to
>>>>> facilitate reviewing, I've broken down the files into the
>>>>> subsections (see below for which files fall into which subsections).
>>>>> Reviewers may find it easier to select one or two subsections for
>>>>> review.
>>>>>
>>>>
>>>> Is this [1] link still the link to use for reviewing? I ask
>>>> because the
>>>> DC xml files for building the TM image don't appear to be updated for
>>>> the post-VMC integration (specifically they don't include the im-pop
>>>> finalizer script).
>>>>
>>>> Cheers,
>>>>
>>>>
>>> _______________________________________________
>>> 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
>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
