Hi Alok,
As promised, here's my review of section (a), as well as one comment
about the shadow_xxx files
- Keith
General:
Shadow* files comment:
The files are placed in:
.../vendor-packages/solaris_install/target/shadow/shadow_<something>.py
I think that should be
.../vendor-packages/solaris_install/target/shadow/<something>.py
The extra "shadow" component is redundant, especially when importing.
Nit: Are we using triple single-quotes or triple double-quotes for
docstrings? I think most of our code leans toward the former -
consistency would be nice (though I don't think we've had a 'formal'
decision)
td.py:
Is there any reason not to name this something less terse? Perhaps
"discovery.py" (so the import path becomes
"solaris_install.target.discovery").
84: Since TD is one of the longer running checkpoints, I'm hoping this
method will get fleshed out.
99-100: Could be combined with the block that starts at 118
118: May be worth moving this whole block to a separate function. May
not be. Up to you.
202-216: You have no idea how happy this makes me.
251: search_name appears to be unused
267: Popen call looks out of date - should be Popen.check_call()
I see a few other places in need of updating as well. (Apologies for
changing this syntax out from under you guys, but I promise it was for
the best, long-term!)
270: Could use "p.stdout.splitlines()"
271-272: I'm surprised this is necessary, though hopefully it doesn't
come up enough to be an issue.
276: Change to:
self.logger.debug("Populating DOC for zpool: %s", zpool_name) # Comma,
not %
291: You could (outside the 'for' loop) run:
# zpool list -H -o name,bootfs
And then store the output for use in when looking at each zpool. Or you
can give it a pool name:
# zpool list -H -o bootfs <pool>
And then you just have the value - that route might be more stable.
300: Can you clarify what this value is used for? I'm concerned that,
since data pools can set this to arbitrary values, if we're using it as
anything more than information for the user we could end up with problems.
308: At this point in the code, we start getting pretty deep indentation
levels. Please break some of this out into separate function calls for
readability.
318: some_str.rpartition() will always return a tuple of length 3 with
the leading "/dev/dsk" removed (note that's Rpartition, not partition),
so 317 isn't needed. This is particularly valuable given that line 321
assumes that "entry" has been set to something...
321, 328: You should probably use rpartition for these (you likely want
to ensure that we're partitioning this string from the right, not the left).
337: It's probably worth adding a "break" after you've finished
processing the match
376: Please add a comment here explaining why we want to skip the "root"
vdev_type.
384-385: The first dataset is the root dataset, not the pool. It just
happens to require the same name as the pool, but it can be mounted,
used and have properties set just like any other dataset, as far as I
understand. Should we really skip it? If it does need to be skipped,
should you use the "-s" (sort) flag to the zfs list call to ensure that
the sorting is exactly how we want it?
397-398: Comment could be clearer: Explain that the output from zfs list
returns values like "37G" and "3.2M" and that Size() expects "37GB" and
"3.2MB"
426: Nit: The first portion of the comparison ("if zpool_name") is
redundant. The second comparison will end up failing properly whether
zpool_name is None, an empty string, or a different pool name.
455: Is setup_iscsi() only ever going to be needed by td.py? Or would it
be worth having in a separate location?
520: If TD is run twice (or two separate TD checkpoints are run), will
setup_iscsi() behave appropriately?
550: Is there any concern that, e.g., self.search_type might be
"some_random_string"?
test_td.py:
33: Nit: Import from solaris_install.
38-39: "from x import *" is generally frowned upon. Preferable to use,
for example, import solaris_install.target.logical as logical;
logical.my_func().
151-169: Enclose in a "def main():", and then add:
if __name__ == '__main__':
main()
vdevs.py:
General: What changes in the ZFS library do we need to be aware of to
prevent incompatibility in this module?
126: retrival -> retrieval
141: I'm confused by this comment. The iteration is over nvlists, not
the pairs within those lists, right? If the nvlist names are non-unique,
doesn't the assignment on 159 override any previous assignments from
nvlists with the same name?
size.py:
General: The "if/elif/elif/..." blocks could be condensed with a
dictionary that maps unit strings to unit values. e.g.:
units = { "kb" : 2 ** 10,
2 ** 10 : "kb",
"mb" : 2 ** 20,
2 ** 20 : "mb"}
# Mappings in both directions was intentional
self.byte_value = self.value * units[self.suffix.lower()]
Sectors would still need to be handled specially though (since you'd
want probably want the units dict to be at the class level, and sectors
is an instance variable)
60, 63: I don't see much value in storing the value/suffix parts as
attributes - would recommend using local vars and ensuring everything
uses self.byte_value.
65: Yes, we care. Would rather raise a ValueError here than have an odd
AttributeError on line 69 when we try to access self.suffix.
86-93: Should this really be settable? (If so, then it doesn't need to
be a property yet)
117: __repr__ should return a string that could be pasted into a Python
interpreter to recreate the object, e.g.:
>>> repr(my_size)
Size("456b")
For precision, it should probably also always use the "bytes" value
(which shouldn't be cast to a float...)
However, the current __repr__ function would make a fine __str__
function (although both functions should use units consistent with what
it accepts, i.e., "TB" instead of "T")
The get() method should be case-insensitive.
Lots of room for enhancements to this class: subclass long instead of
object, include methods for __add__, __sub__, etc. (I'm sure none of
those are strictly necessary, but at the time I'm writing this, I'm just
kicking things around on a Friday afternoon)
test_zpool_vdevs.py:
27, 46: :comments:
setUp/tearDown: See usr/src/lib/install_engine/test/engine_test_utils.py
for helper functions for tests that need to create/destroy/reset the
InstallEngine singleton. (Drew may have already addressed this)
122, 123: Use self.assertEquals(). Also, a subtle "nit" - reverse the
order. (When the test fails, it's more convenient to see "Test failure:
<vdev_map_root object> != <f1 object>")
This comment also applies to 165-166, 217-218, and 224-229
On 03/ 4/11 01:11 PM, Alok Aggarwal wrote:
Just a reminder to those reviewing that the comments
are due by March 10. If you need more time, please let
me know.
Thanks,
Alok
On Thu, 24 Feb 2011, Alok Aggarwal wrote:
We would like volunteers to review the TI/TD project. The
review is being broken down into the following chunks to
aid the review process.
a) Target discovery
b) Target Instantiation
c) Target controller API and target validation
d) ctypes interfaces
e) Other
Webrev location:
http://cr.opensolaris.org/~aalok/cud_ti/
Please let us know which of the above sections you'd like to
sign up for, an approximate list of files corresponding to each of
the review sections is included below for your convenience.
We would like your comments back by March 10.
Thanks,
Drew, Alok (and Jean)
(a) usr/src/lib/install_target/td.py
usr/src/lib/install_target/test_td.py
usr/src/lib/install_target/vdevs.py
usr/src/lib/install_target/size.py
usr/src/lib/install_target/test/test_zpool_vdevs.py
b) usr/src/lib/install_target/ti.py
usr/src/lib/install_target/physical.py
usr/src/lib/install_target/logical.py
usr/src/lib/install_target/test/test_target_instantiation.py
usr/src/lib/install_target/test/ti_full.py
c) usr/src/lib/install_target/shadow/*
usr/src/lib/install_target/test/test_shadow_list.py
d) usr/src/lib/install_target/libadm/*
usr/src/lib/install_target/libbe/*
usr/src/lib/install_target/libdevinfo/*
usr/src/lib/install_target/libdiskmgt/*
usr/src/lib/install_target/libnvpair/*
e) Makefiles, DC changes, MP test changes, engine and
errorsvc changes, packaging
_______________________________________________
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
