* James Falkner <[EMAIL PROTECTED]> [2007-11-07 23:18]:
> http://cr.opensolaris.org/~jhf/ips-multi/index.html
Okay, I finally cleared some time to review this batch. Sorry for the
delay.
*. I would really prefer to not have dynamic detection of the
portable components. That is, we're going to know what platform
we're on by virtue of having already filtered out inappropriate
files. So I would like to see the dynamism simplified away in a
future changeset--at most, these should just be derived classes.
*. Please rename "plat" to "portable". The term, "platform", has a
precise meaning on OpenSolaris and, if we ignore that definition,
is overused.
*. I think pkg.portable needs to be expanded into a set of modules
over time. For instance, the cryptographic operations need to be
collected so that the OpenSolaris instance can take advantage of
the various facilities that allow access to hardware acceleration,
hardware keystores, etc. Similarly for use of ELF-specific
versioning and for the snapshot/BE manipulation.
*. Finally, I would expect that the introduction of pkg.portable
would allow us to hide the os.path.join() business, in favour of
pkg.portable.io or .file or whatever. I shouldn't have to do
both.
modules/plat/os_util.py:
32. I'm pretty sure that rename() should be in os_base.py and not
here, and that it should differ in os_windows.py from os_unix.py.
68. "solaris" -> "opensolaris", or leave it as "sunos".
modules/plat/os_base.py:
*. These functions should be explicitly grouped with comments. I
also wonder if you need to document the exceptions you expect to
be raised on common failure cases.
63,
69. "for_name" -> "by_name".
75,
81. I don't believe "from_file" is accurate and suggest eliding.
(But see os_unix.py comments.)
modules/plat/os_solaris.py:
25 - 27. Are these imports needed?
modules/plat/os_unix.py:
48. I would expect os_solaris to need a more sophisticated is_admin()
test.
57. Well, I see why you called it "from_file". I know Bart has been
prototyping user and group actions, so maybe we can close on what
name services are consulted and decide whether privately parsing
for lookups is what we want.
modules/plat/os_windows.py:
*. I think the block comment needs some expansion, like "the file
and directory actions (or any action potentially involving a
credential) do not utilize credential metadata when acting on
Windows-compatible systems". Or something like that.
modules/actions/directory.py:
95. If I read this right, we're dynamically loading the OSBase class
in the first caller. I wonder if it would be similar to just
have a global instance.
modules/actions/file.py:
125. Yes, .rename() needs to move. :)
modules/actions/generic.py:
*. Is there a separate bug on the .cleanup and file_closer issue, or
is that just missing from the comment on the constructor?
282ff. This code is an example of the problem I mean in *(4) above.
There should be get_root() function to provide this information.
modules/client/image.py:
147 - 154. Bug?
463. (Deletion.) And now I see why we needed a passwd parser. Okay,
let's leave this for now and get to a proper summary position
with the user action work.
modules/server/face.py:
*. Another example of stuff a portability class should eliminate. I
should just be calling pkg.portability.open() with a
"/"-separated path.
modules/server/transaction.py:
*. I'll let this exception-based approach go for now, but the
comment on 217 is still governing.
I think everything else looked fine.
- Stephen
--
[EMAIL PROTECTED] http://blogs.sun.com/sch/
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
