Hi Alok and Drew,
Here are my comments on files in section 4. If you want me to take
a look at the other sections, just let me know.
----------
General:
----------
- In all the checkpoints I reviewed, there's a block of code in the
parse_doc() function that gets the pkg_img_path, ba_build, and temp_dir,
and media_dir. I think it would be cleaner to retrieve
all these values with a function that returns all the retrieved values
so we don't need to repeat the same code everywhere.
- In most of the checkpoints, the dry_run flag in execute_checkpoints()
is ignored, please handle them accordingly. If it is ignored, please
put a comment, and say why it is OK for all the operation to happen
even in dry_run mode.
- In get_progress_estimate() of all checkpoints, it just return a single
hard coded value. I understand that we have not
finalize which machine will be used for generating
the numbers, but I think we should have code in place
to return values based on the size of the image/data we try to operation
on instead of returning the same value regardless of size.
------------
usr/src/cmd/distro_const/checkpoints/ai_publish_pkg.py
------------
- The create_repository() function uses pkg(5) CLI calls to
create the repo. Is it better to use pkg python interfaces
to do the same?
-----------------
usr/src/cmd/distro_const/checkpoints/create_iso.py
-----------------
- line 97-98, 203-204: duplicate code to check for incremental build.
Perhaps just check once and store the value.
- line 132: Does the file description need to be closed after reading?
- line 132: do we also need to make sure the self.volsetid that's
read from the file is not ""?
- create_incremental_media() function makes a copy of the ISO so
USB images can be created. I think this method is too heavy
duty. Would it be easier to store the name of the ISO in
the DOC. In the create_usb.py script, you just look
up the name used for the ISO.
- line 202: perhaps you need an "else" here to throw an exception.
I know architecture can be only "i386" or "sparc", but the way
things are coded right now makes it look like an "else" is needed.
-----------------
usr/src/cmd/distro_const/checkpoints/create_usb.py: OK
-----------------
-----------------
usr/src/cmd/distro_const/checkpoints/custom_script.py
-----------------
- Line 30: when is the "CalledProcessError" used?
- line 65: I think it is better for this to be done in
the __init__() part of this checkpoint, and we check and make
sure the "command_to_execute" is not None. No point fail later.
- lines 72-74: why do we want to redirect stdout/stderr to NULL?
I think it is important to log all messages/errors. You can use
the exec_cmd_outputs_to_log() function in
lib/install_utils/install_utils.py.
--------------
usr/src/cmd/distro_const/checkpoints/grub_setup.py
--------------
- Lines 85-90: Instead of having hard coded defaults here, I think
it is better for them to be defined as constants in the class so
they are easy to find and change in the future.
- Line 85-90: I noticed that you didn't assign a default value for
self.grub_mods.title. I think it would be easist to check that
no special title is specified here, and retrieve the value from
/etc/release.
Then, use a flag to keep track of whether a special title is specified.
That would avoid the 2 places later that has the duplicate code to
get the value from /etc/release.
- Question: in existing DC, there's an entry in the manifest called
<title> within the <grub_menu_modifications> section.
The value specified in <title> is used for populating
all the grub menu entries. So, if I want to do the same
thing with the new DC and manifest, I would just do
the following:
<grub_mods min_mem="0" title="myentry" default_entry="0" timeout="5">
</grub_mods>
Note that I didn't specify a <grub_entry> because I don't want another
grub entry. Have we tested this case to make sure it works?
If it does, I think we need to put better comment in the manifests than
what we have now. The way it looks right now, it looks like I will have to
specify a <grub_entry>
- Line 145-162: In the update_img_info_path() function,
the GRUB_TITLE line is always written to the .image_info file.
The algorithm in the current DC grub_setup.py file only
writes the GRUB_TITLE line to the .image_info file if a special
title is specified. If I remember right, there's special logic
in installadm(1M) to deal with the GRUB_TITLE line being present or not.
Please check installadm(1M) to make sure this new behavior is
OK or change it back to the way it is done in the current DC.
- LiveCDGrubSetup.build_entries() function: What happened to all
the "special" entries that are in
the existing all_lang_slim_cd_x86.xml? Such as the livessh=enable
and all the entries to support accessibility. I checked the
dc_livecd.xml, and don't see them there either.
- LiveCDGrubSetup class: for the the Live CD, the accessibility
entries MUST be the last entry on the grub menu. I don't see any
special code in this class to deal with that.
- Line 236-256: AIGrubSetup.update_img_info_path().
The code to get the grub title from etc/release is duplicate.
Furthermore, as with the comment above, the GRUB_TITLE should
not always be written out to the file.
- Line 253: Need to remove this comment?? :-)
--------------
usr/src/cmd/distro_const/checkpoints/pkg_img_mod.py
--------------
- line 105-108: shouldn't this check before done before line 93,
where the value for self.pkg_img_path is first used?
- lines 118-196: The mkisofs and lofiadm commands and options
for creating and compressing the user and misc archives are very similar,
except the ISO sort file for the solaris.zlib. I think they
should be combined to avoid the duplicate code.
- lines 195-196: I think it will be very useful to put a comment here
to talk about exactly why the "usr" directory have to be removed
here. Otherwise, somebody who tries to clean up the code later
might want to move the removing of the "usr" directory to
the create_user_archive() function, which is where it seems to belong.
- line 225-228, 304-307: why do you need to get these 2 values again?
Aren't
they already retrieved in the parent class's __init__()?
- strip_platform(), strip_x86_platform(), and strip_sparc_platform() all
have code
to walk the platform directory and remove everything except a few things.
I think it might be cleaner to define a function that contains the the
"walk" code
and remove code, and pass in the list of things you don't want to remove.
- line 384: The text installer will need to call
create_livecd_content_file()
function, since the text installer is a cpio based install. It is not
identical the AI.
---------
usr/src/cmd/distro_const/checkpoints/test/test_ai_publish_package.py - OK
---------
---------
usr/src/cmd/distro_const/checkpoints/test/test_create_iso.py - OK
---------
---------
usr/src/cmd/distro_const/checkpoints/test/test_create_iso.py - OK
---------
---------
usr/src/cmd/distro_const/checkpoints/test/test_create_usb.py
---------
- What about testing generating USB using an ISO created with
incremental option?
------------
usr/src/cmd/distro_const/checkpoints/test/test_custom_script.py
------------
- Lines 151-153: no code for this test case? :-)
- What about a test case where the custom script specified is not found?
------------
usr/src/cmd/distro_const/checkpoints/test/test_grub_setup.py - OK
------------
------------
usr/src/cmd/distro_const/checkpoints/test/test_pkg_img_mod.py - OK
------------
------------
usr/src/cmd/distro_const/checkpoints/test/testlib.py - OK
------------
Thanks,
--Karen
On 10/20/10 09:28 AM, Karen Tung wrote:
I will take section 4.
Thanks,
--Karen
On 10/19/10 15:46, Alok Aggarwal wrote:
This is a code review request for the rewrite of DC
to make use of CUD. The implementation here closely
follows the design document that can be found here:
http://cvs.opensolaris.org/source/xref/caiman/caiman-docs/distro_constructor/dc_cud_design.pdf
Note that this wad does not include the support for
VMC. The support for VMC will be a separate, follow on
project.
Since the changes are fairly substantial we are breaking
up the review into the following sections:
DC app, RBAC, Makefiles and packaging [1]
Manifests and XML serialization/de-serialization [2]
Pre pkg image mod, boot archive configuration and archive archive
checkpoints [3]
All the other remaining checkpoints [4]
Please sign up to review one or more of the sections
and let us know exactly which ones you will be reviewing.
Webrev location is:
http://cr.opensolaris.org/~aalok/cud_dc
Please have your comments in by COB November 5th.
Thanks,
Drew and Alok
[1] usr/src/Makefile.master
usr/src/Targetdirs
usr/src/cmd/Makefile.targ
usr/src/cmd/distro_const/Makefile
usr/src/cmd/distro_const/distro_const.py
usr/src/cmd/distro_const/cli.py
usr/src/cmd/rbac/Makefile
usr/src/cmd/rbac/exec_attr.distribution-constructor
usr/src/cmd/rbac/prof_attr.distribution-constructor
usr/src/cmd/distro_const/checkpoints/Makefile
usr/src/cmd/distro_const/manifest/Makefile
usr/src/cmd/distro_const/profile/Makefile
usr/src/pkg/manifests/install-distribution-constructor.mf
usr/src/cmd/distro_const/checkpoints/defaultfiles/Makefile
usr/src/cmd/distro_const/checkpoints/defaultfiles/coreadm.default
usr/src/cmd/distro_const/checkpoints/defaultfiles/rtc_config.default
[2] usr/src/cmd/distro_const/__init__.py
usr/src/cmd/distro_const/configuration.py
usr/src/cmd/distro_const/distro_spec.py
usr/src/cmd/distro_const/execution_checkpoint.py
usr/src/cmd/distro_const/manifest/boot_archive_contents_sparc.xml
usr/src/cmd/distro_const/manifest/boot_archive_contents_x86.xml
usr/src/cmd/distro_const/manifest/dc_ai_sparc.xml
usr/src/cmd/distro_const/manifest/dc_ai_x86.xml
usr/src/cmd/distro_const/manifest/dc_livecd.xml
usr/src/cmd/distro_const/manifest/dc_text_sparc.xml
usr/src/cmd/distro_const/manifest/dc_text_x86.xml
usr/src/cmd/distro_const/profile/ai.xml
usr/src/cmd/distro_const/profile/generic.xml
usr/src/cmd/distro_const/profile/livecd.xml
usr/src/cmd/distro_const/profile/text.xml
[3] usr/src/cmd/distro_const/checkpoints/pre_pkg_img_mod.py
usr/src/cmd/distro_const/checkpoints/boot_archive_configure.py
usr/src/cmd/distro_const/checkpoints/boot_archive_archive.py
usr/src/cmd/distro_const/checkpoints/test/test_pre_pkg_img_mod.py
usr/src/cmd/distro_const/checkpoints/test/test_boot_archive_configure.py
usr/src/cmd/distro_const/checkpoints/test/test_boot_archive_archive.py
[4] usr/src/cmd/distro_const/checkpoints/ai_publish_pkg.py
usr/src/cmd/distro_const/checkpoints/create_iso.py
usr/src/cmd/distro_const/checkpoints/create_usb.py
usr/src/cmd/distro_const/checkpoints/custom_script.py
usr/src/cmd/distro_const/checkpoints/grub_setup.py
usr/src/cmd/distro_const/checkpoints/pkg_img_mod.py
usr/src/cmd/distro_const/checkpoints/test/test_ai_publish_package.py
usr/src/cmd/distro_const/checkpoints/test/test_create_iso.py
usr/src/cmd/distro_const/checkpoints/test/test_create_usb.py
usr/src/cmd/distro_const/checkpoints/test/test_custom_script.py
usr/src/cmd/distro_const/checkpoints/test/test_grub_setup.py
usr/src/cmd/distro_const/checkpoints/test/test_pkg_img_mod.py
usr/src/cmd/distro_const/checkpoints/test/testlib.py
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
