Hi Geoffrey,
Please see my response inline.
On 02/01/12 10:30, Geoffrey Hart wrote:
Karen: Thanks for all the comments. They are very helpful.
As for testing. I have build all the DC ISOs with the updated
manifests and I have performed GUI and Text installs with the
resulting manifests.
Changes I have made per your suggestions:
- Very good point about adding comments to the bugs. I will add these.
- I have moved 416-417 into the if statement as you suggested.
- Removed line 420 since this info is already in the following exception
- Good catch on the DC manifests being set to fail on missing files.
That was left over from my debugging. Updated to False.
- Very good catch with the call to TransferCPIO in BootArchiveArchive.
I have updated the code for this and testing it now.
Open issue for discussion:
You asked about failing immediately if a file is missing. This is a
valid idea since it will fail quicker, but in this scenario I would
argue against it.
1) The user will be better informed if they are told about all the
missing files, not just the first missing file we encounter.
2) This section of the code is not in the copy logic which would be
very slow. This code is a pre-scan of the files where we build up the
final list of files to pass to CPIO. This function takes very little
time to run, so the cost of look for all the missing files is low
(unlike the time to fully copy all the files).
Your thoughts?
Both of the above are valid points. I think it would be useful to
report all missing files in 1 shot too so users
can fix all mistakes. Please put some comment in the code talking about
this deliberate choice to avoid confusions
in the future.
Thanks,
--Karen
Thanks again for all the great comments.
- Geoffrey
On 01/23/12 12:39, Karen Tung wrote:
Hi Geoffrey,
In my rush to send out my comments, I overlooked some of the changes.
So, I will need to rescind a couple of my comments below, and add a few
new comments. If I didn't mention anything about my previous
comments, they are still valid comments from me.
I am so sorry about the confusion. Please see my comments inline.
On 01/23/12 10:17, Karen Tung wrote:
Hi Geoffrey,
You didn't say explicitly in your list of testing done.
Can confirm that the following testing was done?
1) Build all the DC ISOs with the updated manifests?
2) Perform GUI and text install with the ISO built from the updated
manifests.
General Comments:
1) I see that you updated both bugs with the diff output of the
changes.
For those people that are not familiar with the code or those who
don't want to read the
code to understand what was changed, it would be very useful to
write a few sentences
explaining what the solution is.
2) The changes in this code review seem to address bug 7010088. How is
bug 7092487 addressed with these changes?
Please ignore the comment above. I see how you address the bug.
My comment about the changes are:
usr/src/lib/install_transfer/cpio.py:
line 416-417: I think it makes more sense to have these 2 lines inside
the if statement for cpio_proc.returncode != 0.
line 420-421: is it necessary to have both of these statements?
Do messages that come with the exception get logged automatically
at the "exception" log level? I think that's done automatically. If
not,
I think line 420 should be at a level higher than debug so the
information
goes to the console/simple-log also.
3) For 7010088, I thought we just want to print warnings if a file
in the list
of files to be CPIO-ed during DC is not found. We do not want to
abort the
DC build. However, the changes added argument
"fatal_if_not_found=true"
to all the DC manifests.
usr/src/lib/install_transfer/cpio.py:
- I don't see any code that interprets the argument
fatal_if_not_found, and set
the self.fatal_fail variable in this file. Where is that code?
I see the code now. Please ignore this comment.
- Since we want to quit if self.fatal_fail is true. Why not check
for self.fatal_fail
at line 301 and fail immediately? Why waste time to copy all the
other files if we will
fail eventually?
Additional comment:
- The TransferCPIOAttr class is used directly in DC's
BootArchiveArchive checkpoint.
In that usage scenario, I think we should set the fatal_if_not_found
flag to true, since
we are simply don't a copy from the root archive build area to the
actual root archive.
It's important that all the files we expect to get copied gets copied.
Thanks,
--Karen
Thanks,
--Karen
On 01/23/12 09:10, Geoffrey Hart wrote:
Happy Monday all!
Could I please have a code review for:
7010088 transfer-cpio should abort when a file to be
transferred is not found
7092487 cpio transfer errors are not detected or reported
https://cr.opensolaris.org/action/browse/caiman/ghart/transferFixes/webrev/
Testing done for this fix:
Confirmed that DC and the installers can correctly pass the new
parameter to CPIO.
Confirmed that DC still creates bootable ISOs with the new logic.
Confirmed the installers still complete successfully.
Confirmed that TransferCPIO will fail on missing files (if the
input bool is set).
Confirmed that CPIO copy failures will result in an exception.
Confirmed that new logic will still result in the same files being
copied. Thus the new logic didn't cull any of the expected files.
Thanks,
-Geoffrey
_______________________________________________
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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
