Hi Drew and Glenn,
Thanks for the previous code review.
I have updated the code with your comments and suggestions.
All the ISO were rebuilt with the updated code, and all the testing
specified below was redone.
updated webrev:
https://cr.opensolaris.org/action/browse/caiman/ktung/umount-bug-fix3/webrev/
Thanks,
--Karen
On 02/23/12 12:38, Karen Tung wrote:
On 02/23/12 12:36, Drew Fisher wrote:
On 2/23/12 1:31 PM, Karen Tung wrote:
Hi Drew,
Thanks for the review. Please see my response inline.
On 02/23/12 12:20, Drew Fisher wrote:
Karen,
I was looking at this and what you have will work, it just seems
like there are a few unnecessary things that we could clean up:
Why not just do something very simple like:
p = run([MOUNT], logger=logger)
if "/lib/libc.so.1" in p.stdout:
logger.debug("Optimized libc is mounted.")
run([UMOUNT, "/lib/libc.so.1"], logger=logger)
There's no need to keep a LIBC global here. /lib/libc.so.1 isn't
going anywhere so there's no need to worry about changing it later
down the road. No need to make unnecessary variables / lookups.
Well, I can remove the LIBC global. I defined it like that because
I don't like
hard coding multiple instances of the same string in the code.
Also, walking each line of the output isn't really necessary is
it? If libc.so.1 is anywhere in the mount output you know it's
mounted, right?
I walk each line because I want to find the one that *begins* with
/lib/libc.so.1.
Is there a way to find that without walking each line?
If we just check the output like you suggested, will we pick up
false instances where
/lib/libc.so.1 is just part of the output for something else?
I don't think anything is going to mount /lib/libc.so.1 and you can
only mount one path once so it's not going to show up multiple times..
What about changing the if-statement to:
if "/lib/libc.so.1 on" in p.stdout (add the 'on' part)
so that we match this:
[mox:src] > mount | grep libc
/lib/libc.so.1 on /usr/lib/libc/libc_hwcap1.so.1
read/write/setuid/devices/rstchown/dev=3250002 on Tue Feb 14 10:17:54
2012
I think that would work. I am going to change the code like you
suggested,
and publish a new webrev later today.
Thanks,
--Karen
Thanks,
--Karen
What do you think?
-Drew
On 2/23/12 12:11 PM, Karen Tung wrote:
Thanks everyone for reviewing my initial changes for bug 7108163.
Sanjay pointed out to me privately that the fix only takes care of
unmounting optimized libc overlays for x86. It does not take care
of libc overlays for SPARC. For now, there's no libc overlay for
sparc,
so, the fix would still work, but the code will have to be changed
again if we have
a libc overlay for sparc in the future.
I completely re-implemented my changes to take care of the possibility
of having optimized libc for sparc in the future.
The updated webrev is below:
https://cr.opensolaris.org/action/browse/caiman/ktung/umount-bug-fix2/webrev/
For testing, I have performed the same testing outlined below. In
addition, I
also built a SPARC text installer image and AI x86 images.
Mary helped installed a T4 machine using the SPARC text installer
image to make
sure the changes does not affect SPARC systems. She also helped
perform a
a net-boot text install using the AI x86 image to make sure
everything works well
there too.
Thanks,
--Karen
On 02/21/12 17:01, Karen Tung wrote:
I would like to get a review of my changes for fixing the
following bug:
7108163 <http://monaco.us.oracle.com/detail.jsf?cr=7108163>
Solaris 11 image fails to boot after switching between x86 CPU models
http://monaco.us.oracle.com/detail.jsf?cr=7108163
webrev:
https://cr.opensolaris.org/action/browse/caiman/ktung/umount-bug/webrev/
Testing:
- built a x86 text installer image that includes the updated file.
- Install the ISO in a virtual box running on an intel based x86
system. Exported the ova. Then, imported
the ova in a virtual box running on an AMD based system. Make
sure the imported
virtual machine instance can boot fully to login prompt, and I
can log in.
- Also install the ISO in a virtual box running on an AMD based
system. Exported the ova. Then,
imported the ova into a virtual box running on an intel based
system. Make sure the imported
virtual machine instance can boot fully to login prompt, and I
can log in.
Thanks,
--Karen
_______________________________________________
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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
