Forgot to CC the alias ...
-------- Original Message --------
Subject: Re: [caiman-discuss] TI/TD code review
Date: Thu, 10 Mar 2011 20:18:28 -0700
From: Drew Fisher <[email protected]>
To: [email protected]
On 3/10/11 2:51 PM, Karen Tung wrote:
Hi Alok and Drew,
Here are my comments for files in section C.
usr/src/lib/install_target/shadow/shadow_physical.py:
- insert_slice() function: question: for inserting a slice into a
partition,
I think we can also check that the start_sector of the slice is bigger
than the start_sector of the partition.
We have this code here:
# verify the size of the slice does not exceed the size of the
parent
slice_size = value.size.sectors - value.start_sector
if self.parent_size< slice_size:
self.set_error(self.SliceTooLargeError())
Where self.parent_size is calculated based on the size of the container
object (Disk or Partition)
- lines 215-216: can these 2 lines be moved to the very top of this
function?
There's no point to do all the other computations if this is true.
All of the computations are done regardless of finding an error. If a
user inserts a slice with a dozen different errors, all of them will be
added to the errsvc for later retrieval so there's no need to relocate
this particular code chunk.
- line 234: s/continer/container/
Fixed
- insert_partition() function, the key 15 is used many times in this
function
to access the PARTITION_ID_MAP to map to the extended partition
part_type.
It would be good to define this as a constant for better readability.
The old numerical representations of partition IDs are gone and have
replaced instead with calls that look like this:
if value.part_type == value.name_to_num("WIN95 Extended(LBA)"):
which allows for much more readable code. Let me know if you'd still
prefer a string constant though.
- insert_partition() function, in varies places in this function, you
determine whether the partition to be inserted is a primary or logical
partition by comparing to hard coded value of 1-4 for primary partitions
and 5-32 for extended partitions. Since there are predefined constants
FD_NUMPART and MAX_EXT_PARTS, I think it is better to use those.
Fixed.
- insert_partition() function, there's no check to make sure that there
are at most 4 primary partitions.
The check is implicit. Only partition indexes 1-4 are considered
primary. If index 5 is inserted, it's automatically treated as a
logical partition.
- line 293: partition.name need to be casted to an int?
Fixed.
- line 308: value.name need to be cast to an int?
Fixed.
- line 311: partition.name cast to an int?
Fixed.
- lines 337-338: Question: is it ever possible that the disk has no size?
It may be possible that the disk does not have a dev_size attribute.
Disk.disk_prop is initialized to None by the constructor and if the
DiskProp class is never populated for a particular disk, it will not
have a dev_size attribute. I suspect it'll be rare, but I didn't want
to cause a traceback while validating new partitions.
- line 399: value.name cast to an int?
Fixed.
- line 261: define 11 as a constant?
See comment above about using name_to_num()
usr/src/lib/install_target/test/test_shadow_list.py:
TestZpool() tests:
- For delete zpool, perhaps also add tests for deleting all the added
pools, and
double deleting a pool?
Added an addition test for deleting all the added zpool along with an
entirely new test for double deleting a zpool.
- For delete vdev, add tests for deleting all vdev, and double deleting?
Added tests to existing test_delete_vdev unittest
TestZFSFilesystem tests:
- Similar to above, delete all ZFS filesystem, and double deleting?
Added tests.
TestZvol: some additional test cases?
- Add a zvol that's bigger than the pool?
- Add 2 zvols whose total size is bigger than the pool?
- add zvol with size 0?
- add zvol with negative size value?
We don't want to be in business of handling more size math, especially
since the calculation of the size of the zpool is going to be *very*
hard to do accurately. This is why there are no tests surrounding the
size of Zvol objects. ZFS has a very descriptive set of errors to
indicate what the user did wrong.
TestPartition tests:
- line 549, 580-581: remove comment?
Fixed.
- Some suggested additional partition tests that comes to mind:
* Add more than 4 primary partition
As explained above, there's nothing to indicate primary vs. logical
aside from the name/index of the Partition object.
* Invalid partition id
Added a test for it.
TestLogicalPartition: additional suggested test cases:
* Add a logical partition with a size that's bigger than the extended
partition
Added a test for it.
* invalid logical partition ID, like "50"
I would need to get a list of invalid partition IDs first. Do you
happen to have one/know of one?
TestFinalValidation tests:
- I remembered we decided that final validation would check for
a whole bunch of additional things, such as having 1 partition of
type Solaris, and having a root pool defined...etc... If those
things are checked, test_simple, test_simple_disk_with_one_partition,
and test_simple_disk_and_zpool should all fail. Furthermore, we
need to add more tests to make sure that those are indeed checked.
We're still in the process of adding the additional checks to
final_validation in __init__.py. Once those go in, we'll certainly
augment/extend the final_validation tests to go with them.
Thank you so very much, Karen!
-Drew
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
