On Fri, Sep 04, 2009 at 08:43:21AM +0100, Padraig O'Briain wrote: > Brock Pytlik wrote: >> Shawn Walker wrote: >>> [email protected] wrote: >>>> On Thu, Sep 03, 2009 at 07:55:25AM +0100, Padraig O'Briain wrote: >>>>> On 08/27/09 10:22, Padraig O'Briain wrote: >>>>>> The webrev, http://cr.opensolaris.org/~padraig/ips-10539-v1/, fixes >>>>>> >>>>>> 10539 Package Manager hangs after canceling Update All >>>>> Could someone have a look at this proposed change to api.py as it >>>>> causes severe problems to the GUI? >>>> >>>> Yes. Brock or Shawn should probably also look at this, but here are my >>>> suggestions. >>>> >>>> - lines 237 & 238: Since you've moved self.__reset_unlock() and >>>> self.__activity_lock.release() out of conditional that follows this, >>>> it probably makes sense to remove it here too, and just let it get >>>> called from the common location. >>>> >>>> - line 240: If you take the above suggestion, this should be an elif >>>> not an if, I think. >>> >>> I agree. I was initially concerned that there were places that >>> called plan_common_exception that didn't call plan_common_start, but >>> that doesn't appear to be the case (the concern being that release() >>> would be called without acquire() being called...). >>> >>> Cheers, >> Yep, I agree with the suggestions. I'd also suggest removing the # NOT >> REACHED comment. > > I have respun the webrev: http://cr.opensolaris.org/~padraig/ips-10539-v2/
The second of my two comments doesn't appear to be addressed by this webrev. The conditional on line 237 (in the newest webrev) should be an elif, so you don't log two different errors in this code path. Thanks, -j _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
