Ok, I have looked at some of your ctypes interfaces. Specifically, everything
except libefi and libnvpair, per your suggestion, and libdiskmgt, because it is
big and I haven't gotten to it yet. I will do so hopefully sometime in the next
day or so. For now...
libadm:
cfunc.py
for both read_extvtoc and write_extvtoc, i think you have a
double pointer, where you require a single.
more explicitly, in extvtocp, you already have a
C.POINTER(extvtoc), so C.POINTER(extvtocp) is a **extvtoc,
where you require a *extvtoc.
interesting that you have the correct arg for the underlying
C library in extvtoc.py... I'd have thought the interpreter
should bark at that, since it doesn't match your cfunc.py, but no?
libbe:
cfunc.py
nit - for clarity's sake, it might be good to simply use
'BENodeListp' when you have a single pointer to BENodeList. It
may cause confusion comparing, say, C.POINTER(BENodeListp) and
C.POINTER(BENodeList), where you could just use BENodeListp in the
latter.
libdevinfo:
devinfo.py:
admittedly paranoid, however... I'm concerned with running a
re.split on a line that is not guaranteed to contain the re.
we'll bomb there if not, right? Of course, this takes a
failure of a library call that should not fail... do what you
will :)
Also, I just looked at vdevs.py in passing... I'm concerned that if your
handle initialization code fails, you'll simply bomb out. should you check for
return, rather than ignoring the returns from init, open and get_config? I
know, unlikely they'd fail, but if they did...
Like I said, hopefully I'll get onto libdiskmgt soon.
/jb
On Mar 25, 2011, at 6:53 PM, Drew Fisher wrote:
> I need to find a few folks willing to look at round 2 of the TI/TD code
> review. I'll break this section down into the same 5 chunks Alok did:
>
> a) Target discovery
> b) Target Instantiation
> c) Target controller API and target validation
> d) ctypes interfaces
> e) Other
>
> Webrev location:
> http://cr.opensolaris.org/~drewfish/cud_ti_2/
>
> Due to Alok's untimely departure, I do not have his existing webrev directory
> to do an incremental webrev. If this is problematic, let me know and I'll
> work with Glenn to get Alok's machine back so I can try to spin an
> incremental webrev.
>
> The major changes between round 1 and round 2 are:
> - how disk geometry is figured for non VTOC labeled disks. In debugging
> instantiation issues with Karen's installer, we found a pretty large bug in
> libdiskmgt (shocking, I know!). Sanjay is still in the process of examining
> what is wrong in libdiskmgt and how to correctly fix the issue.
> - how slices are created on the disk.
> - extremely basic GPT label detection and support. We are *not* yet ready to
> handle full creation of a GPT labeled disk with slices, but discovery now
> correctly finds them and adds them to the DOC. Consider GPT support in TI/TD
> to be read-only.
>
> **NOTE**
> I desperately need people to look at the ctypes code. I was unable to find
> reviewers for the first pass of the code. I know it's complicated and I am
> more than willing to sit on the phone with people to explain it if need be.
>
> I would love for people to get comments by April 1st. Fool's Day
> jokes/comments are acceptable and encouraged.
>
> Thanks,
> - Drew, Alok and Jean
>
> (a) usr/src/lib/install_target/td.py
> usr/src/lib/install_target/test_td.py
> usr/src/lib/install_target/vdevs.py
> usr/src/lib/install_target/size.py
> usr/src/lib/install_target/test/test_zpool_vdevs.py
>
> b) usr/src/lib/install_target/ti.py
> usr/src/lib/install_target/physical.py
> usr/src/lib/install_target/logical.py
> usr/src/lib/install_target/test/test_target_instantiation.py
> usr/src/lib/install_target/test/ti_full.py
>
> c) usr/src/lib/install_target/shadow/*
> usr/src/lib/install_target/test/test_shadow_list.py
>
> d) usr/src/lib/install_target/libadm/*
> usr/src/lib/install_target/libbe/*
> usr/src/lib/install_target/libdevinfo/*
> usr/src/lib/install_target/libdiskmgt/*
> usr/src/lib/install_target/libnvpair/*
>
> e) Makefiles, DC changes, MP test changes, engine and
> errorsvc changes, packaging
> _______________________________________________
> 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
