Sundar Yamunachari wrote:
> Jan,
>
> *usr/src/lib/install_utils/install_utils.py*:
>
> Suggestion:
> 432-436: The checking of log to none can be made similar to other
> checking of log in lines 477-483, 490-495, and 517-522
> if log == None construct could be changed to if log != None so that it
> will be consistent
Per PEP-8, it should actually be either "log is None" or "log is not None"
Additionally, also use "if some_var" instead of "if len(some_var) > 0" -
sequence types (lists, strings, etc.) evaluate True when their length is
non-zero, and False if they're empty (this is also a PEP8 guideline)
Additionally, I think the code from 470-495 and 514-522 or so could be
greatly simplified by using the builtin splitlines function, which
splits a string at newline characters and returns them in an array:
for line in some_long_string.splitlines():
log(line)
However, I haven't taken a deep look at this code, so if I'm
oversimplifying just ignore me.
- Keith
>
>
>
> - Sundar
>
>
> Jan Damborsky wrote:
>> Hi,
>>
>> Could I please ask for reviewing the fix for bucket of AI bugs
>> related to error reporting ?
>>
>> Please provide your code review comments by COB Wednesday
>> October 28th, 2009. If you need more time, please let me know.
>>
>> Thank you very much,
>> Jan
>>
>> * Background
>> ============
>>
>> As far as AI error handling and reporting is concerned, there is
>> a plan to hook AI into new error handling service which is to be based
>> on work Evan initiated.
>> As it was identified that it will take some time to implement the
>> final solution (there are still things to be sorted out and dependencies
>> to be addressed - e.g. CUD effort, progress reporting or new
>> logging service), we decided to address existing bugs filed against AI
>> error reporting - those which are annoying or might be source of
>> confusion, but which doesn't involve too much effort to fix, since it is
>> likely that some portion of the changes will go away once AI switches to
>> new error and logging service.
>>
>> * Following bugs are addressed
>> ==============================
>>
>> 6651 autoinstall needs more useful error messages from Orchestrator
>> module
>> 7433 autoinstall should include output from pkg(5)
>> 8127 Complete /tmp/install_log not copied to
>> /var/sadm/system/logs/install_log
>> 8639 be_init() failed with error code 4044 when username is ""
>> in SC manifest
>> 10040 Error reporting when unable to download
>> solaris.zlib/solarismisc.zlib is unobvious
>> 10688 AI should validate repository reachability
>> 11421 ddm_drive_is_cdrom(): ioctl(DKIOCREMOVABLE)
>> failed in /tmp/install_log
>> 11640 decrypt liborchestrator IPS debug messages
>>
>> * Webrev
>> ========
>>
>> http://cr.opensolaris.org/~dambi/ai-err-report/
>>
>> * list of files modified - associated with bugs
>> 6651
>> - auto_install.c
>> - installation-screen.c
>> - om_misc.c
>> - orchestrator_api.h
>> - perform_slim_install.c
>>
>> 7433
>> - transfer_mod.py
>> - install_utils.py
>> - libtransfer.c
>>
>> 8127
>> - auto_install.c
>> - ict.c
>> - orchestrator_api.h
>> - perform_slim_install.c
>>
>> 8639
>> - auto_install.c
>> - auto_parse.c
>>
>> 10040
>> - live-fs-root
>>
>> 10688 - addressed by bug 7433
>>
>> 11421
>> - td_dd.c
>>
>> 11640
>> - perform_slim_install.c
>>
>> * PEP8 effort
>> =============
>>
>> Following Python files were made PEP8 compliant and 'pylinted'
>> (they are not pylint clean, but has ratio >7/10 with the exception
>> mentioned below):
>>
>> - install_utils.py (no Errors, no Fatals) - rated at 8.71/10
>>
>> Following green Cdiff webrev sections belong to bug fixes, everything
>> else is PEP8 related:
>> --- 404,414 ----
>> --- 424,441 ----
>> --- 451,539 ----
>>
>>
>> - install-finish (no Errors, no Fatals) - rated at 8.85/10
>>
>> Following green Cdiff webrev sections belong to bug fixes, everything
>> else is PEP8 related:
>> --- 212,238 ----
>>
>>
>> - transfer_mod.py (No Fatals)- rated at 0.33/10
>>
>> The problem of this file is following section:
>> ...
>> execfile('/usr/lib/python2.4/vendor-packages/osol_install/transfer_defs.py')
>>
>> ...
>> I have verified that all errors generated by pylint(1) complain about
>> undefined symbols coming from that file.
>> I was thinking about addressing this problem as Jean suggested (import
>> explicitly all symbols used). It worked in case of 'install-utils.py',
>> but it turned out that transfer_mod.py uses values which are generated
>> on-the-fly, so it didn't help. I have found bug 5559 which will change
>> the format of transfer_defs.py anyway, so I decided not to address
>> that problem by this fix.
>>
>> Following green Cdiff webrev sections belong to bug fixes, everything
>> else is PEP8 related:
>> --- 21,67 ----
>> --- 318,327 ----
>> --- 830,852 ----
>> --- 889,904 ----
>> --- 928,946 ----
>> --- 976,999 ----
>> --- 1011,1025 ----
>> --- 1044,1059 ----
>> --- 1100,1130 ----
>> --- 1146,1160 ----
>>
>>
>> * Testing
>> Please see following file for test used procedures and test results:
>>
>> http://cr.opensolaris.org/~dambi/ai_error_reporting-test/test-procedures.txt
>>
>>
>> Also Jeffrey Huang is involved in testing process.
>>
>>
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>
