On 01/13/10 03:47 PM, [email protected] wrote:
On Wed, Jan 13, 2010 at 02:07:59PM -0600, Shawn Walker wrote:
Greetings,
The following webrev contains fixes for the following issue:
1668 image locking needed to prevent race conditions
webrev:
http://cr.opensolaris.org/~swalker/pkg-lock/
api.py:
- line 2095: Any chance you could address bug 12744 with your changes
to update_publisher?
I'd rather keep that in a separate changeset, but I do plan to try to
address it soon.
image.py:
- Can you explain the use case for the allow_unprivileged option?
Yes, to ensure these commands could work (assuming everything else is
ok) if they are not a privileged user:
pkg install -nv
pkg image-update -nv
...etc.
That's the only reason. If that isn't worthwhile, I can remove that
bit. Of course, this only works right now if all the manifests have
already been retrieved anyway.
I noticed pkg info -r no longer works for an unprivileged user too.
fcntl(2) will never let you set an exclusive lock on a file unless
you were able to open the file for writing. In any case where you
Yes, but an unprivileged user fails before that when attempting to open
the lock file with 'a+'.
- I don't think you need the timeout code in this set of locks. IMO,
it complicates the code path and makes it harder for callers to get
the lock in contended cases. Having the argument block=True/False
would probably be sufficient for most cases.
As an example, if updatemanager goes nuts and gets stuck in a
refresh loop, the image locking code may never get the lock. This is
because instead of blocking on the lock and getting a turn, the
client goes to sleep and may get woken up at an arbitraily later
point in time. If you block on the lock, you'll at least ensure
that you get to run in between updatemanager refreshes.
This code is also doing its own spinning for the NRLock, which is
similarly less than ideal. In this case, threads within the API
that are contending for this lock don't get the benefit of the
mutex's fairness algorithms, nor are they awakened when the lock is
released.
This code does its own spinning on purpose to ensure that the total
amount of time waited includes both acquiring the NRLock and acquiring
the file lock. If clients don't want to use that logic at all, that's
what timeout=0 is for.
I'm still not convinced that there isn't value here in the timeout
logic. Since the logic here is nearly identical to that used by
python's own mechanism for threading conditions, couldn't the same
problems happen?
- line 370: If you've opened the file for reading but not writing and
try to get an exclusive lock, you'll get an EBADF. However, that
case isn't handled here.
The file is always opened with 'a+', what am I missing? The pydoc for
lockf says I should check for EAGAIN and EACCES.
api_errors.py:
- The new error messages seem a little ambiguous to me.
I've changed this.
--
Shawn Walker
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
