Hi Karen -
Thanks for your review. I've replied in line.
ginnie
On 11/05/10 13:12, Karen Tung wrote:
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...
ok. will do.
- 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?
For this code review, I diffed what Jean had out in the first code
review. I will be putting back anything that is in the first code review
that hasn't been put back. I checked as I was preparing this, and a lot
of what was in these files have been put back by some of the other
projects. I think the only things that haven't are the ones that are
transfer specific. For example, somethng like:
ROOTPYTHONVENDORINSTALLTRANSFER= $(ROOTPYTHONVENDORSOLINSTALL)/transfer
--------------------
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?
I believe you're right.
- line 172-173: why do we need 2 different progress reporting lines?
That was an oversite on my part. I need to clean up.
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.
ok.
- line 190: should the _cleanup_tmp_files() be done in a "finally" clause
so it always gets done?
yes.
- 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.
I'll check with Jean on the subprocess stuff and see why it was
implemented this way. I think you bring up good suggestions, so if she
didn't have a particular reason for doing this way, I'll fix it.
- 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?
That's a good point. I'll fix that.
- line 576-577: Isn't the handle to the DOC already set in the __init__()
of this class? In line 559.
seems like it. I'll check this.
- Lines 579-581: these lines also seemed to be duplicate from the the
ones in __init__(), lines 562-564.
good point. I'll check and make sure it works and remove if it is.
--------------------
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.
ok.
- line 602: are you going to fix this comment? :-)
Yes....I wanted to ask Jean for a definition. oops.
--------------------
transfer_ips.py
--------------------
- line 76: need to update this to the latest URL
ok.
- 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.
That's a good idea.
- 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.
That's a good idea too.
- line 356: add a logging statement here so we know what would have been
added in dry run mode?
ok.
- line 440: since you are done here already, why need to check_abort()?
I'll remove that.
- lines 573-578: duplicate code as 548-553?
I will make sure it works w/o it and remove if it is duplicative.
- line 667: need to remove this line?
yes. I missed that.
- lines 649-683: define all the "keys" used for trans_attr as constants?
ok.
- lines 840-843: also define all these keys as constants?
ok.
---------
transfer_p5i.py
---------
- again, all the keys for "trans_attr" should be constants.
yes.
---------------
transfer_svr4.py
---------------
- lines 157: the "timeout" value doesn't seem to be used in this function.
Yes. I should have removed that.
- line 168: why do we need to throw an exception for "abort"?
I can remove that. it's certainly optional. Would you suggest logging a
message? or just let it return.
- line 230-232: why not send stderr to logger too?
I merged the stderr and the stdout. I thought that was a little cleaner.
- lines 319-324: duplicate code to lines 301-306?
I'll double check that it works w/o the self.doc call in parse_input and
remove it if it does.
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
