Hi Drew,
On Wed, 27 Oct 2010, Drew Fisher wrote:
lofi.py: 120, 132: I think the '-f' flag should be used here.
Fixed on line 120. lofiadm doesn't have a -f flag to correspond with the -d
flag (according to the man page). I'm assuming you wanted a "force" flag,
but it doesn't appear to support one.
Sigh, yes it does seem to be an undocumented flag.
Please add a '-f' though to the lofiadm call.
target_spec.py: This file should really use instance variables
for defining the target dtd fields. For example: "target" in
the Target class should be an instance variable TARGET_LABEL
and referred to everywhere like that. Again this isn't a bug
or something it's just a future maintenance problem that might
just be handled at a later point in time.
I feel the LABEL stuff clutters an already fairly straight-forward class.
Also, I think you mean class variables ;) I'll ping you on IRC to talk more
about this.
Doesn't need to be addressed right now, we can talk about
this later.
target_spec.py: A ParsingError exception needs to be raised
with an appropriate message instead of the currently defined
funky exception. I think this should be done now.
Fixed.
I just looked at the webrev and it's actually not fixed.
RuntimeError is being raised whereas it really should be
a ParsingError.
target_spec.py: 102, 140: The target schema enforces the disk_name
name_type to be one of the valid types. If it isn't, we won't even
get to this code. The 'else' clause on both these lines should just
be removed.
Actually, Dave Marker pointed out how the DTD file is supposed to work:
<!ELEMENT disk ((disk_name|disk_prop|disk_keyword|iscsi), partition*,
slice*)>
This means that the <disk> element can have exactly 1 sub element in
[disk_name, disk_prop, disk_keyword, iscsi]. The conditional needs to look
something like this:
if self.ctd is not None:
<code>
elif self.volid is not None:
<code>
elif self.devpath is not None:
<code>
elif self.devid is not None:
<code>
elif self.disk_prop is not None:
<code>
elif self.disk_keyword is not None:
<code>
else:
raise
This will correctly set the priority order to the same thing as in the DTD
file with preference to disk_name.ctd.
Fixed to represent this code.
line 110 RuntimeError needs to change to ParsingError.
target_spec.py: 321-322: typos make this code invalid.
Added a pair of try/excepts around the code to catch ValueError (what you get
if the cast to int() fails). Should something fail, I simply pass since the
instantiation of the Partition class will set those values to 0 by default
The code is still invalid due to typos.
s/paritition/partition
In addition to these files, you also need to include
the packaging changes associated with these files.
Ok, I added target stuff to usr/src/pkg/manifests/system-library-install.mf
I think you also need the definition of
ROOTPYTHONVENDORSOLINSTALLTARGET (used in install_target/Makefile)
in $SRC/Makefile.master as well as INS directives
in $SRC/lib/Makefile.targ.
And, there needs to be an entry in $SRC/Targetdirs as
well for solaris_install/target.
Alok
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
