Hi Matt,
Thanks for the review. Comments inline. I will publish a new webrev
after the changes have been made and retested.
Harold
On 05/11/12 02:50, Matt Keenan wrote:
Harold,
Thanks for sending this out for re-review, I'd missed it the first
time around.
Mostly looks good a few comments :
- method _get_candidate_disks() is pretty much the same code as
_find_disks(),
rather than have two methods can these not be combined and remove
find_disks
completely ?
The only remaining call to find_disks in target_selection.py is
expecting to always
find a specific disk, and only one. So I think it could be replaced
with a call to
_get_candidate_disks() and check the returned list ensuring it only
contains
one item. The logic in finding a matched disk within each method is
more or less
the same (except for how you check for boot_disk), and realistically
they should
be the same, unless you see some reason for making them separate.
No reason at all. Good idea. I will make that change.
- target_selecton.py:3418
Currently there's a check to determine if any matching disks were
found, and throw
an exception if none were found before calling controller.select_disk()
Should this check not be still here e.g.
if not mapped_disks:
raise.....
I don't believe so. There is a check in _generate_candidate_disk_list
for any discovered disks that have not been matched. It is at line
3361. Let me know if you don't think that is sufficient.
cheers
Matt
On 05/10/12 20:51, Harold Shaw wrote:
Dave Marker has already reviewed this but with the build 17-20 rule I
need to get 2 reviews, so can I get an additional code review for the
following bug:
CR: http://monaco.sfbay/detail.jsf?cr=7147174
There is an additional change that isn't noted in the webrev. I
moved line 194 in target_selection.py left by one tab. The function,
__pretty_print_disk, if called with a list of disks would only create
a string with the first disk in the list. The return statement
needed to be moved out of the loop.
Webrev: https://cr.opensolaris.org/action/browse/caiman/hshaw/7147174_2/
All of the target unit tests were run on both Sparc and x86. All of
the tests passed. Unit tests were added to both the Sparc and x86
target selection tests to verify the fix.
A number of AI manifests were written to test different sets of disk
specifications in the manifest. They were run using auto-install and
stopping before the target instantiation checkpoint. These manifests
have been sent to QE to be used as a basis for a new test suite that
will be able to test a large set of manifests that describe a range
of disk environments.
Full Installs were run with the default manifest and 2 of the test
manifests to ensure proper layout and installation.
_______________________________________________
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
