Hi Niall,
Thank you for your review! Comments in-line.
On 4/11/11 8:36 AM, Niall Power wrote:
Hi Drew,
I'm looking through section B (instantiation). I haven't finished physical.py
yet so my comments are in relation to logical.py and instantiation.py:
logical.py
----------
197:
@property
def datasets(self):
The command invoked is "zfs list -H -t fileystem,,,"
This only returns a subset of the potential datasets as it doesn't
include zvols or snapshots which are also datasets. Would it be more
appropriate to rename this property method to "def filesystems(self)"?
It doesn't exactly do what it says on the tin.
I agree with you. I *think* this is a hold-over from the original
zfs.py code Keith added for the engine. I need to verify that property
isn't used anywhere (a basic grep returns nothing obvious, but I need to
examine the engine and DC files more closely).
219:
Zpool.get()
Method doesn't strip out the header line from the results nor split the
output so that the value can be isolated from the other columns
eg. Here's the output returned from Zpool.get("bootfs")
NAME PROPERTY VALUE SOURCE
rpool bootfs rpool/ROOT/solaris-161 local
I think the caller is responsible for this as /usr/sbin/zpool get has no
-H flag to exclude the headers or -o flag to print just the columns
wanted. I'll look into fixing this to at least remove the headers.
Again, need to dredge through the libraries and DC to find all
invocations of Zpool.get().
class Filesystem: snapname(), set(), get(), create(), exists():
Instead of having to caclulate the full filesystem name in each of
these methods, would it be quicker and easier to just store full_name as
a
property of the Filesystem object?
I agree. Will fix.
class BE: 789 def mount()& class LOFI: 878
os.mkdir(mountpoint) assumes the parent directory of mountpoint exists.
Would os.makedirs() be more suitable to create intermediate directories?
Absolutely. Fixed.
shadow/Zpool.py
---------------
35:
""" ShadowZpool - class to hold and validate Solaris x86 Zpool objects.
"""
Is it really x86 specific?
Haha, yes. SPARC uses extfs3. :P
(fixed)
instantiation.py
----------------
63:
self.target = self.doc.get_descendants(name=Target.DESIRED,
class_type=Target)[0]
if not self.target:
raise RuntimeError("No desired target nodes specified")
Is it ever possible for self.target to be None here?
Wouldn't you get an IndexError in the previous line where you lookup index [0]
of the results of self.doc.get_descendants()?
Changed to:
self.target = self.doc.get_descendants(name=Target.DESIRED,
class_type=Target, not_found_is_err=True)[0]
Which will handle all of conditions you exposed above.
104: if partition_list:
Should this be encased within a conditional for architecture since MBR
partitions are not valid on Sparc? The manifest parser can't enforce this
restriction, is it enforced anywhere else (eg. in Target Controller?)
No, because partition_list is populated via
disk.get_descendants(class_type=Partition). If there are no Partition
objects in the DOC under that disk (as would be the case for SPARC), it
will return an empty list. An empty list is a boolean false in Python
so the code under "if partition_list" will never be executed.
I'll finish going through physical.py tomorrow.
Awesome, thank you!
-Drew
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
