Hi Drew,
Thanks for the comments...
On 23/05/2011 15:33, Drew Fisher wrote:
> For the most part this looks really good. target_selection.py is mostly
> absurd (in a good way) though :)
>
> usr/src/cmd/auto-install/auto_install.py
> ----------------------------------------------------
>
> Why the switch from "/" to "." in register_checkpoint() calls? I
> thought they had to be slashes?
>
Well, sort of...
There is a bug in the python where if you use slashes here, and an exception
is thrown, then an attempt to catch the exception won't work - or an
instanceof won't work either.
The problem is that if you use slashes as above to import the module in the
engie, then you end up with an exception with a name like:
solaris_install/auto_install/checkpoint/target_selection.SelectionError
while the class name of the object in one you've imported locally, is more
like:
solaris_install.auto_install.checkpoint.target_selection.SelectionError
so they are seen to be different classes!
To work around this, we are importing the modules at the top of the file, and
then registering them using dotted notation - this fixes that issue.
It took us a while for figure this out since we kept falling into a generic
catchall rather than the specific code.
I logged this with the engine code as bug 7041360.
>
> 156, 394, 438, 572: FIXME comment
Removed thanks.
>
> 239: combine with 240
>
> if linecache.getline(manifest, 1)[:2]:
Done
>
> 462: Let's talk offline about this. I'm sure we can move this code
> block up higher
Actually Matt and I thought about this and we think that we can't since we
need to have parsed the manifest, to get the transfer checkpoints which are
dynamically registered, and also we stop the list of checkpoints according to
the -s options "stop at" value.
>
> 1086: remove unneeded line
Done, didn't realise the with would close it automatically...
>
> usr/src/cmd/distro_const/checkpoints/pre_pkg_img_mod.py
> --------------------------------------------------------------------------------
>
>
> I'm surprised by this showing up here. This was my change for CR
> 7041226. Might want to check for other merge issues? Is this a merge
> error?
Yes, this is a merge error where I've done the webrev, I'll fix this. The
changes in this file should be copying the /usr/share/install/*.dtd to
/auto_install on the ISO.
I'll try fix the webrev, stange one...
>
> usr/src/lib/install_common/__init__.py
> ---------------------------------------------------
> 118-121: Same issue as above. Keith fixed this as part of CR <can't
> find this right now>
>
I'm not sure what you mean here, I don't see any thing in the webrev
at:
http://cr.opensolaris.org/~dkenny/cud_ai-to-slim-2/usr/src/lib/install_common/__init__.py.wdiff.html
around these lines...
> 407-420: "file" is a reserved word in python. Can you rename?
Done, changed to filename
>
> usr/src/lib/install_target/Makefile && controller.py
> ------------------------------------------------------------------
> Potentially another merge error? Wasn't this Dermot's push of
> controller.py to slim_source?
What differences are you seeing here?
Again, this is only a one-line change in the webrev.
But we've added a few lines since which are minor, but otherwise but we'll not
be changing it except for PEP8 compliance.
>
> usr/src/cmd/auto-install/checkpoints/target_selection.py
> -----------------------------------------------------------------
>
> 503, 537: s/This/this
Done
>
> 826-835: Why not just use zpool.get_first_child(name=zvol.name,
> class_type=Zvol) ?
Exactly, why not ... ;)
Changed.
>
> 837-850: Why not just use discovered.get_descendants(class_type=BE)
> instead of grabbing all the zpools?
The main purpose here is to get all the BEs with in a given zpool, but not
from the zpool itself, but from the version if itself in the discovered tree.
As such we can't just get all the BEs - the logic to get all BEs and then find
out which of these is in the zpool we want just seems counter intuitive.
>
> 1005 - 1018: You can simply this by using the default argument to
> dict.get()
>
> if zpool.exists:
> retsize = Size(zpool.get("size", "0b"))
> else:
> retsize = Size("0b")
> return retsize
Done.
>
> 1073, 2643, 2920: You're looping through a list of Logical DOC objects
> but the DTD specifies that only 1 Logical element can exist:
>
> <!ELEMENT target (disk*, logical?)>
Changed.
>
>
> 1121, 1129, 2411: TODO comment
Removed the first two, considering the last...
>
> 1142-1145: Why are these lines here?
>
Old comments since we used to do this kind of thing here, but it's moved to TC
now.
> 1433: align indent with [ on 1432
>
Done
> 1446: align
Done.
>
> 1502: s/iv/if
Done.
>
> 1509, 1889: you don't need the call to .keys()
>
Changed here, and some other places too... ;)
Thanks,
Darren.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
