On 03/ 2/10 12:14 PM, Danek Duvall wrote:
Shawn Walker wrote:
On 03/ 1/10 07:05 PM, Danek Duvall wrote:
Shawn Walker wrote:
http://cr.opensolaris.org/~swalker/pkg-529/
- 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...).
You could do it in makedirs itself, too, no?
Add the EBUSY, EEXIST, etc. error handling I have in the actions
themselves? Sorry for getting lost here.
- 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.
I'd figured that we would eventually end up with a phase running prior to
any action removal/update/install that would validate the image against
what we expected to be there -- not a full verify step, but something that
would go and check to make sure that directories hadn't turned into
symlinks by accident, and the like.
Currently, preinstall() is called by preexecute() which is triggered
whenever a client calls prepare() for a given operation plan and is done
for all pkgplans in the imageplan.
After preexecute() succeeds, then we call download() for all pkgplans in
the imageplan.
That would mean that if I add the check to preinstall() that we'd check
before doing the download.
Having that test during install is still important, but if we can give
users a heads-up that the plan will fail on execution, or even tell them so
with pkg image-update -n, then it's useful in a different way than catching
the problems reactively.
If we were to split the preexecute() up so that it was strictly the
preinstall, etc. routines and have download separately, and then allow
callers to control the behaviour of that via api.prepare() flags, this
would enable clients to easily do this.
So the answer to your question, roughly, is "we want both".
If we do the check in preinstall(), I assume the reason for having the
check again in install() is to ensure that some package in the plan
hasn't broken things since we first checked?
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.
Okay. That shouldn't be an issue if you just do the lstat() once and look
at the results of S_ISDIR() and S_ISLNK().
Note that lines 145-147 in install() have this islink() check too for
the same reason (from what I can tell).
Note that if os.path.isdir() calls stat() instead of lstat(), then you're
using it incorrectly to make the decision on line 2512 of image.py. There
may be other places this is happening, too.
So are you saying that anywhere we call islink() and isdir() both we
should just do a single stat instead?
I wonder if we should have our own isdir() in portable instead to
simplify this for cases where the only stat check we need is that...
- 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...
The intent was to wrap exceptions raised during the execution of an action
so that the top level could distinguish them from exceptions raised in
other parts of the code, and emit sane errors about a particular action
causing a problem. It only got implemented in a few places, though.
If you want separate exception classes for "something went wrong during
action execution" and "the image didn't match what we expected during
action execution", then I'd name the latter ImageStateException (or Error;
it's not clear to me how to choose between the two). I'm not sure why
you'd want one to be an API exception and the other not.
I suppose since this an exception related to the image and the image is
really part of the client API that it makes sense to put it into
pkg.client.api_errors. I'll do some consolidation or renaming here
after I have a think about this.
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 !=
'/'.
Ah. And salvagedir() is (or can be) called with the full path, rather than
just the path relative to the image root. This hasn't been the case so
far, has it?
It has been only in one place -- where I was using it during image
catalog rebuild in image.py. I didn't realise that the salvage code
didn't accept full paths when I first used it.
It was a bug waiting to happen either way since if you didn't realise or
know it was expecting only relative paths it would be wrong and you
wouldn't know it.
However, when testing these new changes, I realised it wasn't expecting
a full path and fixed it to allow either.
This had the added bonus of not having to worry about which type of path
gets passed into it in the various actions when calling salvage.
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.
I'm talking about the call to time.sleep() and the change in num_expected,
neither of which seem to have much to do with 14940. You explained the
sleep call in your other message, but I still don't get the change to
num_expected.
Right, the call to time.sleep(1) is because of the depot index not being
ready and the test suite having no way to poll that at the moment.
The num_expected changed because the pkg5unittest.image_create() is
using the client API to create the image which means the transport API
makes one less connection to the server to check versions.
So really, not only does the change to image_create() speed up the unit
test because it avoids the exec of pkg(1), but it also does one less
connection to the depot server.
t_pkg_install.py:
- 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).
Okay, but there's no need to create variables that aren't ever used.
Yes, and I've fixed that already. Thanks.
I like this set of fixes, but I'm uncomfortable putting them into a respin
of the last build of the release. There are too many changes in finicky
parts of the code, and this just needs more soak time before it can go into
the release. I'd be much happier targeting this for a backport to 2010.03
in a couple of months (or whenever we start opening the 2010.03 gate for
that kind of work).
I figured as much, especially given the string changes (although
arguably, an untranslated message is better than an exploding client).
My hope was that we'd have a sustaining gate for 2010.03 soon and this
could just go back to the main gate. Then, at some later point, we
could evaluate it as a backport for the sustaining gate.
Cheers,
--
Shawn Walker
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
