Stephen Hahn wrote:
> * 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.
No worries. Thanks for taking the time!
> *. 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.
They are derived classes now (derived from OSBase). I'll see what the
most effective way is to get rid of the dynamic detection, but I'd still
like the ability to add support for a new platform by simply dropping a
new os_foo.py file into the package, and possibly updating a package
manifest somewhere else.
> *. Please rename "plat" to "portable". The term, "platform", has a
> precise meaning on OpenSolaris and, if we ignore that definition,
> is overused.
Done.
> *. 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.
Agreed. I think Darren's work on signed packages could help point
this mechanism in the right direction.
> *. 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.
Well, keep in mind that using os.path.join usually works regardless of
if it's passed /-separated or \-separated components. It's when developers
use string-based mechanisms to manually break up paths that is the problem,
and in reality its only a big problem near the "top" of the filesystem (where
the Windows-isms like drive letters start to interfere and require some
if/else statements and special-case code).
So while I wouldn't advocate we provide a custom os.path.join, I think
that certain higher-level operations could be encapsulated in the
pkg.portable APIs, such as relocating a path under a new root.
> 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.
OK.
> 68. "solaris" -> "opensolaris", or leave it as "sunos".
I'd prefer leaving it as sunos until there is a real need/example of
IPS having different codepaths on OpenSolaris vs. Solaris.
> 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.
Yeah, I should and will.
> 63,
> 69. "for_name" -> "by_name".
OK.
> 75,
> 81. I don't believe "from_file" is accurate and suggest eliding.
> (But see os_unix.py comments.)
See below response.
> modules/plat/os_solaris.py:
>
> 25 - 27. Are these imports needed?
Nope. Cut and paste error.
> modules/plat/os_unix.py:
>
> 48. I would expect os_solaris to need a more sophisticated is_admin()
> test.
This was the extent of admin checking done in the previous code (see
directory.py). I'd be happy to expand but the original code in directory.py
may start creating u+w directories where it wouldn't have before.
Probably not the end of the world, though. Do you have suggestions
on the expansion? Or perhaps it should be removed and replaced with a
more specific
> 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.
Ok. I'll follow up with Bart and see what his plans are for lookups from
IPS actions.
> 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.
Ok.
> 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.
Yep, I will be changing this from Krister's comments.
> 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?
I need to document that more (see my next response to Krister) in
both generic.py and filelist.py.
> 282ff. This code is an example of the problem I mean in *(4) above.
> There should be get_root() function to provide this information.
Yep, and this is the type of API I would rather put into the pkg.portable
area, vs. our own implementation of os.path.join().. I'll make the change.
> modules/client/image.py:
>
> 147 - 154. Bug?
Yeah, on Windows, where the root is never "/" (and therefore this method
would infinitely loop, since os.path.join("C:\\", os.path.pardir) still
gives you c:\\.
> 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.
Right. Ok.
> 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.
Yeah, well, in this case I think I went a little overboard with the
os.path.join. It could have been
os.path.join(sys.path[0], "usr/share/lib/pkg")
and it would have worked fine. More importantly, I think what is needed
is some kind of notion of the install root for the pkg tools, since it's
not always "/". The presumption is that the pkg binary (err.. the top-level
entry script) resides in <install_root>/bin, and that you can get to the
default content by going to ../share/lib/pkg. That was the intention here,
so I'll add a generic "get_install_root()" method into os_util to tell
where the package tools are installed.
> 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.
Thanks again for the review. I have made a few additional changes since
this original code review, as I have been merging ever since and have had
to fix a few uses of non-portable APIs. I'll call those out in a "diff of
diffs" that will accompany my updated code review.
-jhf-
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
