Hi Jean & Dave,
I removed items I have no further comments on.
On 08/ 3/10 10:17 AM, jean.mccormack wrote:
On 08/ 2/10 10:55 AM, Keith Mitchell wrote:
[...]
Nit: I don't think disks have an "offset," do they?
No. But all PhysicalTargets have an offset so we make Disk have an
offset property that always returns 0.
That makes sense.
Diagram 2:
Can you explain the "Mixins"? I know I've read about this Python
concept before, but it's still not concrete, and there's no
explanation of the Partition/SliceChildrenMixin classes here.
We'll explain with an example. Both DiskPartitionChildren and
PartitionExtChildren inherit from PartitionChildrenMixin.
When you add a partition to a disk it's similar to adding a logical
partition to an extended partition so we're putting the common code in
the mixin. It could just be a function that they just take advantage
of instead of a mixin and might end up that way in the long run.
Ok, thanks.
I'm not certain all the subclasses will be required explicitly. For
example, where the only thing changing is basic values like the
"limit," once could simply instantiate different objects and have a
limit parameter, rather than an entire class for it. I've not thought
about it deeply, but I almost think a single PhysicalChildList class
could be sufficient, with appropriate parameters for limit, class(es)
accepted, etc.
The reason for having separate classes is so a disk doesn't have to
check various combinations of PhysicalChildList.
For example:
If you make a PhysicalChildList that only holds primary partitions and
is limited to 4. The Disk class needs to be able to tell that is a
valid PhysicalChildList.
You could also have a PhysicalChildList that contains Slices limited
to 16.
The code in Disk now has to check for at least 2 possibilities:
if physlist.type = Slice:
assert(physlist.LIMIT = 16)
elif physlist.type = Partition
assert(physlist.LIMIT=4)
But with the class hierarchy there is just one test:
assert(isinstance(object, DiskChildren)
And that one simple test remains valid even if we add a
DiskFooChildren that inherits from DiskChildren.
I'm not sure I understand why the first set of assertions would be
needed - it seems like the point of having the PhysicalChildList class
(or DiskChildren or whatever) is so that the Disk class doesn't have to
do that kind of checking, it defers it to the sub-class. If two or maybe
three subclasses are needed, that seems reasonable; it just seems like
having 9 separate subclasses is overkill when one or two subclasses
combined with proper parameters could do the job.
Page 16:
The type() function has very limited value for making logical
decisions (mostly due to complications from subclasses). It'd be best
not to encourage its use in this fashion - isinstance() should work
fine here.
Agreed, type is limited and isinstance() is more useful. But in this
example where we set function and we use type, we need a type returned
and not a bool.
In retrospect, that was a foolish comment on my part. Pardon the noise.
[...]
7.3: I think it'd be better to have a separate error class for
"received libtd error" and "incorrect input" - suggest perhaps
ValueError for the latter. "DiscoveryError" would also be a more
descriptive name for TDError.
The ManifestParser design specified raising one error class that
contained a message and the original error object.
In order to make all the checkpoints consistent, we'd like to follow
that methodology. No issues renaming it to DiscoveryError instead of
TDError.
From the caller's standpoint, raising a single error regardless of root
cause isn't very useful. The caller needs to be able to logically
diverge based on different exception scenarios, as the errors would need
to be handled differently, and the called library/module/checkpoint
shouldn't really be making assumptions about what the caller will be
doing with that information.
For the manifest parser case, a single exception type is workable
(though I still would've preferred distinct sub-classes), as the
exceptions there all have a similar fix: tell the user that there's an
issue with the manifest in sufficient detail for the user to resolve the
issue.
For TD, the two error cases are very different scenarios. In the case of
a libtd error, the likely result is that the app informs the user that
there's a fatal error in one of the libraries, and exits gracefully. On
the other hand, the invalid parameter issue would need to be handled
differently - it's most likely a programming error, and would lead to an
"ungraceful" exit (a la pkg(1), "unhandled exception occurred, please
file a bug")
Will libtd still be in C? If so, in the last sentence here,
"exception raised" is misleading (error/return code would be more
accurate, I think). If not, I strongly caution against re-masking a
Python exception unless specific additional context can be added.
Yes libtd will be written in C. We will be interfacing with that
library using the ctypes module so it will be raising exceptions.
Ok. The wording is confusing as it implies libtd raised something that
you'd be catching and re-raising, as opposed to checking the return code
from a libtd call via ctypes and raising an exception if something occurred.
[...]
8.2: So 'destroy' objects will simply be names like "c0t0d0s0", and
not full Physical/LogicalTargets? Would it be possible to accept both?
It will be a fully qualified path in the DOC, something like
discovered_target.children[4].children[3].
The idea is to try to make it easy to specify exactly what you want to
destroy and not have to specify a huge tree. You also don't want to
destroy anything that wasn't discovered so it ought to be in the DOC.
Sure, but why not provide a reference to the object itself, rather than
a name?
For "create," what would happen if the disk originally had a Solaris2
partition and a Windows partition, and the object under "create" was
that disk with just the Solaris2 partition (unchanged) with modified
slices? Would the Windows partition be destroyed, left untouched, or
would this be an error?
This is the exact reason we have the destroy object. What will happen
despends upon whether you tell us to destroy the Windows partition or
not.
Ok, so what happens if the Windows partition wasn't destroyed with a
"destroy" command first?
8.3: Same comments as 7.3
Same answer.
And same response.
- Keith
Jean & Dave
On 07/27/10 12:34 PM, jean.mccormack wrote:
Please review the design doc for the Common Object classes/TI/TD
design located at:
http://hub.opensolaris.org/bin/view/Project+caiman/CUE_docs
We'd like to get comments by COB on Tuesday August 3rd.
Thanks,
Jean, Dave, & Sanjay
_______________________________________________
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
