Evan Layton wrote:
> I need to get reviews for the following bugs that are causing failures
> if the directories where the boot/grub menu files live are missing
> (/rpool/boot and /rpool/boot/grub). Also this fix resolves the issue
> with the add_splash_image_to_grub_menu that was causing a failure to
> create a menu.lst file when it's missing.
Hi Evan,
Depending on how much time/effort you have to put into items indirectly
related to your changes, here are some suggestions for "Python-izing"
ict.py a little. I tried to limit myself to code that you touched.
I know ict.py is one of those "C-like" Python files we have. Since
you're touching some of it, would it be possible to change, for example,
lines 329-330 and 321-322 to raise a ValueError instead of forcing a
sys.exit? For this change to mesh well and get called from C code,
exec_ict would need to be changed to be a try/except, log the traceback
appropriately, and set status=1. (Oh and side-note: the if __name__ ==
'__main__' block could be completely changed to:
if __name__ == '__main__' and sys.argv[2:] and sys.argv[1] in dir(ict):
exec_ict(*sys.argv)
The * operator expands an array into a tuple for passing to a function.
(Actually, the "sys.argv[2:] and sys.argv[1] in dir(ict)" might be worth
leaving out, so that someone who calls into ict.py with incorrect
parameters at least gets a traceback and instead of being silently ignored)
Also, and this is completely unrelated to the bugs, but line 339 should
probably be:
self.IS_SPARC = (platform.processor() == "sparc") - platform.processor()
is essentially "uname -p"
Line 345: Should be "except ValueError:" (otherwise it would catch
KeyboardInterrupts, SystemExits, etc.)
Once all other code review comments have been addressed, change all tabs
to spaces (per PEP 8). And include a pylint score (even if it's bad -
perhaps a "before" and "after"?)
I should stop before I get distracted and try to rewrite the entire thing...
- Keith
>
>
> The bugs:
> http://defect.opensolaris.org/bz/show_bug.cgi?id=7880
> http://defect.opensolaris.org/bz/show_bug.cgi?id=11436
>
>
> The webrev:
> http://cr.opensolaris.org/~evanl/7880/
>
>
> Thanks,
> -evan
>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
