On 2/21/12 11:58 AM, David Marker wrote:
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.
Fixed. I think I was going to do something more with it and left the
decorator on.
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).
Fixed to be C.c_int
You don't have IMA_FreeMemory in your function list, see ima.py below.
Added.
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
Changed to c_uint64
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.
Changed to C.c_ubyte * 512 per the struct definition.
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.
Fixed to inherit from ctypes.Union
usr/src/lib/install_target/libima/ima.py:
I would combine 38 and 39 to
oid_list_p = C.POINTER(cstruct.IMA_OID_LIST)()
Done.
You are supposed to call IMA_FreeMemory() on oid_list_p before leaving
set_chap_password().
Done.
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() ?
I need to test this out. I think I can get away with doing something
like what we do in discovery.py:
# for each zpool, get all of its datasets. Switch to the C
locale
# so we don't have issues with LC_NUMERIC settings
cmd = [ZFS, "list", "-r", "-H", "-o",
"name,type,used,mountpoint", zpool_name]
p = run(cmd, env={"LC_ALL": "C"})
Thanks for looking at this, Dave. I really appreciate it!
-Drew
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
