Hi Niall,
Please see my response inline.
On 12/23/11 18:25, Niall Power wrote:
> usr/src/lib/install_common/__init__.py.src
Lines 482-500: We already have a function that determines
the amount of memory a system have in lib/install_target/controller.py.
The function is called _get_system_memory(). Looks like it is only
used in the controller.py file. Can we either remove the
other one and use this? Just don't want to have duplicate code.
Actually, this is the function that was in controller.py. We saw it
there and it was felt that since it was a generic function not
specifically related to target controller at it belonged better in a
general location so that it could be reused. So it has been moved to
this location. Dave also ported it from forking prtconf to using a C
typed system library (it appears that could be replaced by os.sysconf
according to Darren's comments.
ah, ok. I didn't check the controller.py file in the code review. It's
good to know that it is removed
from there.
> usr/src/lib/install_boot/boot.py
line 676-678: there's no handling of the case where you can't find
any nodes from 673-675. Is that OK?
If that occurs, it means that target instantiation and transfer
checkpoints must have failed also because this checkpoint is executed
after those. So I can't see how it could occur because the previous
checkpoints would have to fail before we get here. It will blow
shortly after when we try to install the boot loader anyway :-)
Well, all the above is true for this case. However, each checkpoint is
supposed to operate independently.
Users should be able to execute a checkpoint by itself, and the
checkpoint should behave "well".
If a checkpoint is expecting a certain piece of data, it should check
and make sure that piece of
data exists. If not, it should fail gracefully with a nice error
message, not just traceback.
line 856: commented out line
yeah. I left that there to be uncommented when ident file issues is
fixed by Seth
line 855-859: Looks like the "ident" file is created at the root of
the image. Is this file only used when we boot from media?
As far as I know yes.
Is this file also necessary for the installed system?
If it is not necessary for the installed system, it needs to
be cleaned up by the cleanup_cpio_install checkpoint.
I don't think it is. Good catch.
Seth: The installed system doesn't need this ident file, right?
Based on Seth's reply, this file is not needed in the install system.
In that case, you can add the necessary code in cleanup_cpio_install
checkpoint.
If it is used by both ISO and installed system, would it be better to
deliver
the file as part of a package instead of creating it dynamically here?
It's supposed to be unique based on a timestamp or similar, so if we
have to deliver it
I don't think pkg would be a suitable vehicle. But I am almost certain
we can delete it
in cleanup_cpio_install
OK.
line 855-859: can we move these few lines inside the try block, after
line 865. That way, you don't need to do the unlink if the setprop fails
because of Legacy Grub.
I believe we could move also lines from 850 inside and avoid an
unnecessary open() and read() if the
boot loader doesn't support the property we are trying set, if I first
check the property is supported
in the boot loader rather than doing a try: except: to set the
property. eg.
eg:
860 if hasattr(self.config.boot_loader, "PROP_IDENT_FILE"):
# Read volset ID and setup boot ident file here
....
Would that sound like a good solution?
Yes, I think that will make the code easier to read.
> usr/src/cmd/text-install/disk_window.py
Drew made the code changes to Text Install, so I will comment on the
ones I think I can answer. And leave the nasty ones for him ;)
line 227: why not have this line immediately after line 223, in the else
block? Since it's obvious that we do not have gpt data at that point.
Actually I think it can be removed altogether since the constructor
sets it to False at line 147. Do you agree?
The set_disk_info() function in this object is being called all the time.
So, whatever value that's set in the constructor might be overwritten by
different calls to sett_disk_info(). We obviously don't want to use the
previous value.
So, it's best to reset it every single time.
Thanks,
--Karen
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
