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