Hi Alok.
Lots of sophisticated code here... Here are my comments on section (b).
usr/src/lib/install_target/ti.py:
---------------------------------
69: Not needed, as zvol_list is replaced unconditionally on line 73.
73: zvol_list doesn't need to be part of the instance, as it is used
only by this routine.
105,118: Is there an implicit assumption that partition and slice lists
are orthogonal? Please add a comment to that effect here.
263: Nit: can move under "if vdevs"
usr/src/lib/install_target/physical.py
--------------------------------------
27: Nit: "formatting" has two t's. Rather than "etc" you could say
managing or something similar.
Doesn't pep8 compliance require at least a ''' header?
(what is proper term for this?)
58: partition_sort may be misnamed, as it could be used by any object
with a name attribute. Disk objects or anything that subclasses
DataObject has a name attribute. (actually a property)
71: Better to use C #define conversion instead of using 191, but OK.
72: bootid is defined here but is not saved in to_xml. Likewise
is_linux_swap. how come?
163: comment seems inaccurate. May be that "Removes this partition from
the shadow list" is more accurate.
205, 226: How come for some cases (e.g. line 189 for partition resize)
self._children = ShadowPhysical(self)
but other times this is not necessary? Both cases are deleting the
existing partition and then calling self.parent.add_partition(). Seems
like it is related to whether or not self.parent.insert_children() is
called.
But if this is the case, what happens when change_type is called (which
also delete()s and add_partition()s), followed by a call to add_slice()
which calls insert_children() but does not call ShadowPhysical() first?
241-251 and 270: Assumes there will be no errant or overlapping
partitions (i.e. that there will be only child.size.sectors between any
two consecutive child.start_sector items).
You could sort self._children on child.start_sector and get rid of the
V_BACKUP child, then check the "child" pairs for overlap easily.
Additionally you can get rid of the usage list entirely, which would
simplify the loop at 255-267. In fact, the 241-246 and 255-267 loops
could probably be combined and simplified together.
243: Is there such a thing as a backup partition? (I know about a
backup slice.)
Nit: in class Slice to_xml() and from_xml(), leave a comment that
self.in_use is not saved not restored, and that it is found dynamically
each time the application starts.
Disk and Partition classes share many methods, for example add_slice(),
delete_slice(), get_gaps(), __copy__(). These methods total ~70 lines.
Too bad this cannot be consolidated somehow...
666-667, 696: same comment as for 242-251, 270.
781: there is already ACTIVE def set to 80
811 Nit: self.kernel_arch != "sparc" -> self.kernel_arch == "x86" is
more maintainable if there is ever another type of processor added.
947: Nit: I don't see the bang for the buck in having a DiskKeyword
class. Seems a list would suite the same application just fine.
Another idea is to have DiskProp absorb DiskKeyword contents, either
having boot_disk as a boolean property, or just having it be true if it
exists and false it it doesn't.
usr/src/lib/install_target/logical.py
-------------------------------------
nit (maybe an RFE): use libzfs?
251: If you don't validate, I suggest turning the existing comment into
a statement, i.e. "No need to validate here becase the schema has a list
of valid redunancy values already."
I don't understand the logic of snapshot() on 428.
If overwrite is true, the old snapshot is destroyed before a new
snapshot of the same name is taken. But if overwrite is false, still a
new snapshot of the same name is taken, which will fail. Why not just
err out if an existing snapshot exists and overwrite is false? Or am I
missing something?
667-668: Just curious: When would name attribute be None? Adding a
comment about this would be helpful, being that it is unusual.
891: What about destroying the ramdisk (the mkfile'd item itself)?
usr/src/lib/install_target/test/test_target_instantiation.py
------------------------------------------------------------
184: ncly -> ncyl ?
Just curious: is ncyl an optional or pre-initialized field?
General (hopefully naive) question: how do these tests show that things
are working? I see the try/except clauses in the tests, but that just
means that things aren't null), since there are very few exceptions
explicitly raised (for example only one in ti.py, one not in a
from_xml() routine in physical.py and logical.py).
Please add a comment before each test to explain what the expected
outcome is supposed to be, where it is not obvious. For example, what
is the outcome of test_empty_zpool_preserve() supposed to be? ... Or
since these all have dry_run=True, is the point of these tests is to
show that the modules can handle the use case without actually doing
anything?
usr/src/lib/install_target/test/ti_full.py
------------------------------------------
OK.
Extra files:
============
usr/src/lib/install_target/size.py:
------------------------------------------------
65, 83: I say we do care, because if the value is not set properly, then
get() cannot return a valid value. If exceptions are raised here then
that likely indicates a programming error, so it can make the code more
robust.
The rest looks fine.
usr/src/lib/install_target/td.py
--------------------------------
306: entires -> entries
Delete XXX at 349, code block removed by comment from 350-367 ?
Thanks,
Jack
On 02/25/11 11:15 AM, Jack Schwartz wrote:
Hi Alok.
I'll look over at least (b).
Thanks,
Jack
On 02/24/11 12:33 PM, 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
