Hi Shawn,
On Wed, Feb 03, 2010 at 03:20:01PM -0600, Shawn Walker wrote:
> Greetings,
>
> The following webrev contains fixes for the following issues:
>
> 11522 pkg should require publisher prefix to match repository information
> 7156 client image api needs image creation interface
> 12744 update_publisher over-zealously testing publisher validity
> 14203 image-create usage doesn't mention mirror / origin options
>
> webrev:
> http://cr.opensolaris.org/~swalker/pkg-11522/
Most of this looks fine to me. The only comments I had pertain to the
api and transport.
api.py:
- lines 2118-2120: You should replace this with
'self.__enable_cancel()'
- line 2125: It's not entirely correct to call __cancel_done() here.
If you're going to support cancellation, this routine needs to do
the following things:
1. The try block should have a specific handler for
api_errors.CanceledException. This handler should call
self.__cancel_done() before re-raising the CanceledException.
2. The try block needs a generic exception handler. This needs to
call self.__cancel_cleanup_exception() to wake any threads waiting
for a cancelation that they will not receive because of an another
exception.
3. Call self.__disable_cancel() after the call to
get_publisherinfo(). If you place this call inside the try/except
block, any cancelation exceptions generated as part of
__disable_cancel will get handled correctly.
For a good example of a function that does this neatly, take a look
at prepare() lines 535-584.
- line 2210: Will you please add an exception handler for
api_errors.CanceledException before the finally block. The handler
should call self.__cancel_done(). You don't need any other handlers
in this code, since cancelation is disabled before the operation is
performed.
- line 2333: Thanks for fixing this.
api_errors.py:
- lines 1249-1256: I believe this exception is redundant.
UnsupportedRepositoryOperation is what the Transport currently uses
when it cannot find any repositories from the requested publisher
that support a particular operation.
transport/transport.py:
- Does your code use both get_publisherdata and get_publisherinfo?
- line 569: Is url defined? I couldn't find it. If it isn't it
should be 'url = d.get_url()' inside the for loop on line 549.
- line 572: The content exception should tell the stats object that an
error occurred. This applies a penalty to the depot that gave you
the bad content. Repository selection tries to choose a repo that
has fewer errors. To get this to work right, add the following
lines in your exception handler between 571 and 572:
repostats = self.stats[url]
repostats.record_error(content=True)
- line 584: This should be an UnsupportedRepositoryOperation
exception like line 492.
Thanks,
-j
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
