Hi Ginnie,
The updated code look great. They are much easier to go through and
understand
than the previous version. Here are my comments on the updated code:
General:
------------
- In transfer_cpio.py, transfer_ips.py and transfer_svr4.py, a dictionary
called "trans_attr" is used for storing what needs to be done.
All the keys used for "trans_attr" are currently hard coded strings.
It would be better to have all the keys be constants. That way, the
same key can be used by the function setting it and the functions using it.
I also noticed a lot of these keys are the same string, so, maybe they
even be combined as well...
- What about the needed changes to Makefile.master, Makefile.lib and
Targetdirs...etc? Those are not included in this webrev. Will they be
part of the final putback?
--------------------
transfer_cpio.py
--------------------
- line 102-103: If self.src is None, we won't get into line 103.
But we will get into line 111, and that will cause an exception because
self.src
is None. Do you need to check and return an error if self.src is None?
- line 172-173: why do we need 2 different progress reporting lines?
lines 185-188: Looks like the actual exception is not returned to the
caller.
At the very lease, we should log the exception using
self.logger.exception(msg)
so detail of the exception doesn't get lost.
- line 190: should the _cleanup_tmp_files() be done in a "finally" clause
so it always gets done?
- line 383-385: what about stdout, will it be ignored?
- line 381, and 385: the use of an "err_file" to capture the errors.
Looks like the content of the err_file is not checked or "transferred" to
anywhere. So, the caller will not be able to find out exactly
what errors are raised. Why not
log all stdout and stderr to the log handler? You might be able to just
use the exec_cmd_outputs_to_log() function in osol_install.install_utils
or implement something similar.
- line 501: after you call get_size() here, do you need to check and
make absolutely sure that you are not getting self.DEFAULT_SIZE? If
something
is wrong, you might get self.DEFAULT_SIZE again. Is that OK?
- line 576-577: Isn't the handle to the DOC already set in the __init__()
of this class? In line 559.
- Lines 579-581: these lines also seemed to be duplicate from the the
ones in __init__(), lines 562-564.
--------------------
transfer_info.py
----------------------
- lines 376-547: I think it would be very useful to define all the
strings used in this function for action and action type to be constants.
Then, the other code that uses CPIOSpec can use those constants too.
It will be easier to maintain in the long run.
- line 602: are you going to fix this comment? :-)
--------------------
transfer_ips.py
--------------------
- line 76: need to update this to the latest URL
- line 223: would it be better to log a message in "warning" level
instead of
"debug". I am afraid that debug level messages will get lost among all the
other messages in the log. Warning will make it stand out more.
- lines 253-256: can we print these log statements in lines 248-251? Seems
like it is not necessary to have another set of "if" statements.
- line 356: add a logging statement here so we know what would have been
added in dry run mode?
- line 440: since you are done here already, why need to check_abort()?
- lines 573-578: duplicate code as 548-553?
- line 667: need to remove this line?
- lines 649-683: define all the "keys" used for trans_attr as constants?
- lines 840-843: also define all these keys as constants?
---------
transfer_p5i.py
---------
- again, all the keys for "trans_attr" should be constants.
---------------
transfer_svr4.py
---------------
- lines 157: the "timeout" value doesn't seem to be used in this function.
- line 168: why do we need to throw an exception for "abort"?
- line 230-232: why not send stderr to logger too?
- lines 319-324: duplicate code to lines 301-306?
Thanks,
--Karen
On 11/ 2/10 05:58 PM, Ginnie Wray wrote:
I've incorporate the code review comments from the first round of
code reviews and would like follow up feedback. The code is posted at:
http://cr.opensolaris.org/~ginnie/transfer/
I've posted the diffs from the first code review, so I would like
to clarify: Keith suggested changing the names of the files from
transfer_*.py to just *.py, which I've done. That, however, made it
difficult to post the diffs from the original transfer_*.py files.
What I've done is included the new file names, and then also posted
the diffs from the old files. So, there is no need to review any of
the files marked as new. They are identical to the transfer_*.py file.
My thought was that it would make it a little easier to review the diffs.
Let me know if you have any questions or comments. I would like to
push on Nov. 16th, so if I could get feedback by the end of the week,
I would appreciate it.
thanks,
ginnie
_______________________________________________
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
