On 03/ 1/10 07:05 PM, Danek Duvall wrote:
Shawn Walker wrote:
http://cr.opensolaris.org/~swalker/pkg-529/
directory.py:
- line 74: why is this line necessary?
Technically it's not, but it seemed kinda lame that the only thing
preventing an undefined var error in this block is the order of
conditional evaluation for line 150.
...
- if makedirs() blows up not because the leaf directory isn't a
directory, but because something up the chain isn't, what happens?
Probably not what I want to happen. I could compare e.filename to path
at the beginning and simply re-raise immediately if it doesn't match?
(Could do that anywhere we call makedirs in the actions I'm changing
really...).
- Shouldn't the logic in file.py to prevent installing through a symlink
be used here as well?
Hadn't thought of it; I'll add it. Which reminds me, should I continue
to do the realpath check in install(), or should I do it in
preinstall()? I figured it was better to do in install, but I leave it
up to you.
file.py:
...
- line 437: why the islink() test?
Because os.path.isdir() will return true if it's a link pointing at a
directory. In that case, the error for unlink is saying that the user
doesn't have permission to remove the link.
- line 441, 443: why the existing exception in one case, and an
ActionExecutionError in the other?
Because EPERM gets translated into ApiPermissionsException by the
imageplan and any other error we've historically translated into
ActionExecutionError. I don't know why we have ActionExecutionError
really...
hardlink.py:
- line 92, 101: so what's the qualitative difference between an
ActionExecutionError and an ActionOperationError? I know the former is
an API error and the latter isn't (and appears to get wrapped up into
an ImageStateErrors), but if all it is is something that gets wrapped
up like that, then perhaps its name could be a little more specific.
ActionOperationError is in the pkg.actions class, isn't part of the API,
and is used to indicate that the action failed even though it was
otherwise valid.
I'm not sure what to do with ActionExecutionError honestly, I just know
it has a different meaning that what I intend for ActionOperationError
and it's part of api_errors instead of pkg.actions.
I could rename ActionOperationError to ActionStateError or do something
else. Suggestions?
link.py:
- line 123ff: this is looking pretty familiar. Candidate for moving into
generic.py?
Sure; I'll do that (file and link are nearly identical).
image.py:
- line 2485: while I understand lstripping "/" from path, I'm not sure I
understand stripping self.root.
Because if the path to the image is /tmp/ips.test.3245/image/, I don't
think you want the salvaged entry (e.g. /my/fancy/file) to be stored as:
/tmp/ips.test.3245/image/var/pkg/lost+found/tmp/ips.test3245/image/my/fancy/file
...I think you want:
/tmp/ips.test.3245/image/var/pkg/lost+found/my/fancy/file
In other words, stored salvage paths should be relative to the image
root since the image itself could move around if the image.root != '/'.
t_api_search.py:
- test_bug_8318: is this a mis-merge, or some other interaction with
Brock's change?
Intentional changes for 14940. It just makes the test suite faster and
chopped a lot of code out. Otherwise, it would have been going through
API object creation twice. There are probably other places where this
could be done with more work, but this was the simplest test to update.
t_pkg_api_install.py:
- line 828: "577"? This should be an octal constant, so humans can read
it. Looks like plenty of other places use a decimal constant, which
will result in bad modes. If your intent is that these bad modes will
be fixed up on package install, that's fine, but I'd rather see octal
modes which are incorrect, rather than relying on the decimal -> octal
conversion going awry.
Yes, the intent is to see that it corrects the bad modes. However, I'll
change these to be more comprehensible.
- line 831: why use self.plist here and not for install or uninstall?
Because verify requires an exact FMRI while install will do the matching
for you.
t_pkg_install.py:
...
- line 4908: isn't this, strictly speaking, "fails as expected"?
Yes, I've fixed up all the docstrings for the tests here to more
accurately reflect the cli testing.
- line 4931: this isn't ever used -- test seems incomplete
Intentionally incomplete; the comment for the test is misleading. The
happy medium I took with the cli test suite was that it only tests for
failure cases (that the client doesn't traceback) and assumes that all
the success cases will be handled by the API tets (the docstring for the
test suite says so).
Thanks for the review,
--
Shawn Walker
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
