Hi Jack,
On Wed, 9 Mar 2011, Jack Schwartz wrote:
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.
Removed.
73: zvol_list doesn't need to be part of the instance, as it is used only by
this routine.
I've changed this so it isn't an instance variable.
105,118: Is there an implicit assumption that partition and slice lists are
orthogonal? Please add a comment to that effect here.
I have changed the comment to indicate *fdisk* partition so
as to distinguish between a partition and a slice.
263: Nit: can move under "if vdevs"
Moved.
usr/src/lib/install_target/physical.py
--------------------------------------
27: Nit: "formatting" has two t's. Rather than "etc" you could say managing
or something similar.
Changed.
Doesn't pep8 compliance require at least a ''' header?
(what is proper term for this?)
Either ''' or """ - I think we're mostly consistent in using
""".
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)
Can you suggest a better name?
71: Better to use C #define conversion instead of using 191, but OK.
Yep, changed to use a constant taken from the C header file
instead.
72: bootid is defined here but is not saved in to_xml. Likewise
is_linux_swap. how come?
Neither the bootid nor is_linux_swap part of the target schema,
thus they are not normalized.
163: comment seems inaccurate. May be that "Removes this partition from the
shadow list" is more accurate.
Actually it is infact doing a resize and not a remove.
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.)
Nope. I will let Drew comment on the above since he wrote this code.
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.
It's mentioned in one of the other files that in_use is populated
at runtime.
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
This was commented out code that's now been removed.
811 Nit: self.kernel_arch != "sparc" -> self.kernel_arch == "x86" is more
maintainable if there is ever another type of processor added.
Changed.
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.
Possibly, I'll talk with Drew about this.
usr/src/lib/install_target/logical.py
-------------------------------------
nit (maybe an RFE): use libzfs?
Actually we don't really want to do that since libzfs change
quite frequently whereas the cli output doesn't (atleast not
as often).
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."
Changed.
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?
If overwrite is false and a new snapshot of the same name is taken,
I believe that means there's a programming error and we want the
command to fail in that case.
Btw, this functionality is only ever used by the engine.
667-668: Just curious: When would name attribute be None? Adding a
comment about this would be helpful, being that it is unusual.
Good catch, name can't ever be None. If it is, it's not even
going to pass syntactic validation.
Removed this check.
891: What about destroying the ramdisk (the mkfile'd item itself)?
There isn't a use case for destroying the ramdisk.
usr/src/lib/install_target/test/test_target_instantiation.py
------------------------------------------------------------
184: ncly -> ncyl ?
Just curious: is ncyl an optional or pre-initialized field?
It's a 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?
Yep, the point of these tests is infact to show that the modules
can handle the use case right upto the point of doing destructive
actions.
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 code now raises an exception.
The rest looks fine.
usr/src/lib/install_target/td.py
--------------------------------
306: entires -> entries
Changed.
Delete XXX at 349, code block removed by comment from 350-367 ?
Removed.
Thanks so much for the review, Jack.
Alok
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
