Matt,
Just two minor nits with varshared.py:
193,195: for the boolean flag, can you add the argument name you're
setting? (This is just a personal nit of mine. Feel free to ignore... :) )
self.add_filesystem(dsname, dsmountpoint, in_be=True)
205: you're inside a paren so you don't need the "\"
Otherwise, looks good. No need to spin a 3rd review for either nit.
-Drew
On 7/7/11 11:24 AM, Matt Keenan wrote:
Thanks for the review drew.
New Webrev uploaed (version .2)
https://cr.opensolaris.org/action/browse/caiman/mattman/7048015.7049157.7049160.2
comments in line.
On 07/07/11 16:18, Drew Fisher wrote:
Matt:
Here's my code review comments. Most of the changes I suggested for
varshared.py are just optimizations to make the code more efficient.
target_selection.py
-------------------
1359: change to 'if not vdev_devices:'
Done
varshared.py
------------
28: doc string is referencing varshareddataset.py rather than
varshared.py
Done
91-104: can't this be two simple get_descendants calls?
def in_dataset_list(self):
for cl_type in [Filesystem, Zvol]:
dslist = self.target.get_descendants(cl_type=class_type, name=dsname)
if dslist is not None:
return dslist[0]
return None
This would also allow you to get rid of self.zvol_list and
self.filesystem_list, although you might have to return the class_type
it was
found under to help with your error messages.
I can just check isinstance(ds, Zvol), for the error messages.
116-120: Since there can only be one root_pool in the DESIRED tree, it
would
make sense to make a class property which finds the root pool. This
would
prevent multiple lookups of the pool:
@property
def root_poot(self):
if self._root_pool is not None:
return self._root_pool
for root_pool in [z for z in self.zpool_list if z.is_root]:
self._root_pool = root_pool
else:
self._root_pool = None
Then, line 122 becomes:
if self._root_pool is None:
Done
178-223: This is essentially a complete copy of process_var() but with
SHARED_DATASET_NAME and SHARED_DATASET_MOUNPOINT. Can you make a generic
funtion that handles both cases?
for name, mountpoint in [(VAR_DATASET_NAME, VAR_DATASET_MOUNTPOINT),
(SHARED_DATASET_NAME, SHARED_DATASET_MOUNTPOINT)]:
<all of the code>
They are quite similar, but there is distinctions.. either way did
what you asked, but new single function has built in knowledge of both
VAR and SHARED definitions for warnings etc...
229: You never do anything with dry_run so don't bother with this line
Done
-Drew
On 7/7/11 8:31 AM, Matt Keenan wrote:
Hi,
Can I get a code review for bugs :
7048015 Automated Installer should create a separate /var and shared
area
7049157 Text installer should create a separate /var and shared area
7049160 GUI installer should create a separate /var and shared area
http://monaco.sfbay.sun.com/detail.jsf?cr=7048015
Webrev :
https://cr.opensolaris.org/action/browse/caiman/mattman/7048015.7049157.7049160
All three installers need to create /var within the installed BE and
/var/shared globally available. This is achieved by adding two
Filesystem objects to the DESIRED root Zpool object before Target
Instantiation is called. TI will then simply create them.
A new checkpoint "VarSharedDataset" is being created to handle the
additions, and will be called by all three installers.
cheers
Matt
_______________________________________________
caiman-discuss mailing list
caiman-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
caiman-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
