Hi Keith and all,
My code review comments for the library code. I reviewed all files in:
libtarget_pymod
libtd
libzoneinfo
If a file is not listed I had no comments for it. I will send my review
of the profile code later today.
thanks,
sarah
****
libtarget_pymod/discover.c:
> 69 //PyThreadState *_save;
> 70 //_save = PyEval_SaveThread();
These need to be removed or used.
> 319 if (attrl != NULL)
> 320 td_attribute_list_free(attrl);
Not sure you need to check for attrl != NULL here. Looks like the
function will do the right thing, either way. Ditto for other places you
are checking for this.
> 483 disk = Tgt_NEW(TgtDisk);
> 484 if (disk == NULL) {
> 485 PyErr_NoMemory();
> 486 goto TgtDisk_Create_CLEANUP;
> 487 }
> 488
disk.c:
Seems like you could just return the PyErr_NoMemory() here. No need to
do the 'goto'. I don't think you need to call the Py_XDECREF() on this.
> 97 PyObject *blocks = NULL; /* Python2.4 missing uint64_t fmt spec
> */
This reference to an issue in python 2.4 should no longer be an issue,
right?
> 136 /* Python 2.4 has no scan code for a potentially 64-bit value */
So, does python 2.6 have scan code for a potential 64-bit value?
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?
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
