Thanks for the review Karen, comments below :
On 07/07/11 21:18, Karen Tung wrote:
Hi Matt,
I saw that you already generated updates to the webrev link you
initially posted.
My comments are based on my review of:
https://cr.opensolaris.org/action/browse/caiman/mattman/7048015.7049157.7049160.2/var-shared/
- For both GUI installer and text installer, you added code to register
the VarSharedDataset checkpoint.
You did not update the "backend" of these 2 installers. As it is, the
installers will still work,
but it is not optimal. For text installer, please update:
usr/src/cmd/text-install/ti_install.py, around line 250. Currently, that
code
will execute the prepare transfer checkpoint only, because the target
instantiation
checkpoint comes immediately after it. If you leave the code as it is,
it will work,
but it will also execute the VarSharedDataset checkpoint. Can you change it
to pause_before the VarSharedDataset checkpoint?
usr/src/cmd/gui-install/progress_screen.py, around line 277: same change as
the text installer.
Done
---------------------------------------------------
/usr/src/lib/install_target/varshared.py
---------------------------------------------------
- line 81: I think the not_found_is_err flag should be included in this
call.
At this point, at least one zpool should be in the desired target.
Done
- Question: why not look for the root pool in parse_doc() function, and
raise exception
if it is not found, since it should be there in the desired target at
that point. Seems to
be better if we report errors sooner.
Agreed, however root pool is only checked for if we are actually added a
new Filesystem object to the Desired tree, putting it into parse_doc()
would mean it's always looked for, I think it might be better to leave
as is.
- line 200: this debug message makes it sound like you are actually
creating the var share datasets, but
you are just added the var share datasets to the desired target. Can you
adjust the message to
be more accurate?
What about this message :
"Filesystem 'var' is being specified outside of root pool Boot
Environment"
Thanks
Matt
Thanks,
--Karen
On 07/07/11 07:31, Matt Keenan wrote:
Hi,
Can I get a code review for bugs :
7048015 Automated Installer should create a separate /var and shared area
7049157 Text installer should create a separate /var and shared area
7049160 GUI installer should create a separate /var and shared area
http://monaco.sfbay.sun.com/detail.jsf?cr=7048015
Webrev :
https://cr.opensolaris.org/action/browse/caiman/mattman/7048015.7049157.7049160
All three installers need to create /var within the installed BE and
/var/shared globally available. This is achieved by adding two
Filesystem objects to the DESIRED root Zpool object before Target
Instantiation is called. TI will then simply create them.
A new checkpoint "VarSharedDataset" is being created to handle the
additions, and will be called by all three installers.
cheers
Matt
_______________________________________________
caiman-discuss mailing list
caiman-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
caiman-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
