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?
image.py:
- Can you explain the use case for the allow_unprivileged option?
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
get a permission exception, you're not going to be able to lock the
file exclusively if it's not writable. However, I don't see this
code using a read lock instead. What are you trying to protect
against in this case?
- 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.
- line 363: As per Stephen, would you please add the owner's hostname
to the lockfile?
- 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.
api_errors.py:
- The new error messages seem a little ambiguous to me.
The image is currenty in-use by another package client,
(pid_name) (pid), and cannot be modified.
Is it the image or the packge client that cannot be modified? I might
re-write this as:
The image cannot be modified because it is currently in use by
another package client: (pid_name), (pid).
Thanks,
-j
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
