Hey Drew,
Just did a first pass, mostly focusing on ctypes and targets, comments below:
usr/src/cmd/auto-install/auto_install.py:
Why is "log_chap_message" a property?
I kind of expect there to be something on the LHS of an '=' when a
property is invoked, but on 447 and 660 you are using it as a method.
usr/src/lib/install_target/libima/cfunc.py:
"IMA_GetInitiatorAuthParms" and "IMA_SetInitiatorAuthParms" both have
IMA_UINT as their second parameter, which is a c_ulong.
From the prototype for those function, its supposed to be an int (well
enum but in C all enums are ints).
The reason its working now is we have python compiled 32-bit where
C.sizeof(C.c_ulong) == C.sizeof(C.c_int).
You don't have IMA_FreeMemory in your function list, see ima.py below.
usr/src/lib/install_target/libima/cstruct.py:
line 39"objectSequenceNumber" is probably better specified as
C.c_uint64.
I know that ctypes on Solaris turns that into a C.c_ulonglong
So I don't feel as strongly about this as the enum
line 65 and 76 "reserved" is an array of 512 bytes in C.
This is potentially harmful but nothing in ima.py uses it an a way that
will blow up.
IMA_INITIATOR_AUTHPARMS is supposed to derive from a C.Union not
C.Structure.
This works in ima.py because you use the first field. But if you ever
extend ima.py and need to use something else it will fail.
usr/src/lib/install_target/libima/ima.py:
I would combine 38 and 39 to
oid_list_p = C.POINTER(cstruct.IMA_OID_LIST)()
You are supposed to call IMA_FreeMemory() on oid_list_p before leaving
set_chap_password().
usr/src/lib/install_target/physical.py:
line 1969, you check for a string that comes from iscsiadm_main.c, and
localized with gettext().
So my question is, don't you need to make sure LANG=C for run() ? Maybe
give it a preexec_fn ?
Actually that may be true for some of the other times you run() and
then look in output.
I did a quick test booting text install to shell after choosing French:
>>> from solaris_install import run
>>> print run(["/usr/bin/env",]).stdout.strip()
…
LANG=fr_FR.UTF-8
Time to extend run() ?
cheers!
-Dave
On Feb 15, 2012, at 11:30 AM, Drew Fisher wrote:
> Good morning!
>
> Dermot and I would like to get a first-round code review for the following
> PSARC/CRs:
>
> PSARC/2012/023 Interactive Installation to iSCSI
> 6974246 Automated Install should provide mechanism for setting iSCSI
> initiator-id-node
> 7004719 Opensolaris LiveCD installation should give a GUI for installing
> Solaris onto iSCSI LUN
> 7004720 Opensolaris text installer should give a screen for installing
> solaris onto iscsi lun
> 7114789 unlabelled iSCSI drives and setting an iSCSI boot-device require
> special handling on SPARC
> 7121245 iscsi paths don't translate to bootable OBP paths/strings
> 7132111 sample ai manifest file does not mention setting whole_disk in disk
> target section
> 7132457 Race condition in AI involving the target discovery and the check for
> the new OS Device Name
> 7145512 pulling iSCSI information from the DHCP server does not support iSCSI
> boot
>
> https://cr.opensolaris.org/action/browse/caiman/drewfish/iSCSI/
>
> Karen: Could I get you to look specifically at the text-installer changes?
> Niall/John: Could I get you both to look specifically at the GUI changes?
> Jesse: Can you look at code in $SRC/lib/install_target as that's where all
> the calls to iscsiadm and libima.so exist
>
> Outstanding issues:
>
> - QE has not yet started a test cycle. I've been working with Angela Li on
> this. I'm hoping to start a test cycle soon
> - ai_manifest.4 is still being edited. I've emailed proposed changes to Alta
> who will translate them into actual English
> - The fix for 7132457 is slated to be tested by PIT soon for confirmation.
>
> ISO / USB access can be found at Hudson:
> http://indiana-build.us.oracle.com/view/iSCSI/ Choose the job for the
> installer you want to look at and you'll see links along the job's page
> pointing to the latest ISO / USB image
>
> Screenshots of the GUI and Text-Installer (for those not interested in firing
> up an entire ISO) can be found here:
> http://mox.us.oracle.com/code/screenshots/gui/
> http://mox.us.oracle.com/code/screenshots/ti/
>
> If people want to try out the ISOs, let me know and I can provide target
> IP/LUN information to use.
>
> Thanks!
>
> -Drew and Dermot
> _______________________________________________
> 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
