Tiny nit: Add a space after the comma in (OSError, IOError) on line 2282. No need for a new webrev after changing that.
Everything else looks good to go now. Thanks, Keith On 03/ 2/10 08:55 AM, Evan Layton wrote: > On 3/2/10 9:40 AM, Keith Mitchell wrote: >> Hi Evan, >> >> Most of it looks good. Just some minor things: >> >> 2284: Catch IOError as well. Since you're not making use of "errno", >> then the line: >> "except (OSError, IOError) as err:" >> would be appropriate (replacing "strerror" with "str(err)") > > Fixed. > >> >> 2283, 2286: I'm never sure if webrev shows alignments properly (as it >> doesn't handle whitespace) - make sure that these lines are aligned just >> after the opening parentheses from the previous line to be PEP8 >> compliant. (The trailing slashes on the previous lines are also >> technically not needed as they're inside parentheses) > > Fixed both the indentation and removed the unneeded \ > >> >> 2283 vs 2286: In the shutil.copy2 call, you use self.rootpool - in the >> error message, you reference self.basedir - which is correct? > > The error message was incorrect. > > I've updated the webrev with these changes. > > Thanks for taking a look! > > -evan > >> >> Thanks, >> Keith >> >> On 03/ 1/10 09:13 PM, Evan Layton wrote: >>> I need to get a review for bug 14962. >>> >>> Bug: >>> http://defect.opensolaris.org/bz/show_bug.cgi?id=14962 >>> >>> Webrev: >>> http://cr.opensolaris.org/~evanl/14962/ >>> >>> Some background: >>> This bug resolves the issue related to installing in a logical >>> partition where the first time a BE is activated the activate >>> will fail due to running installgrub when it doesn't need to. >>> >>> This does not resolve the issues related to bug 14894. >>> >>> Thanks, >>> -evan >>> _______________________________________________ >>> caiman-discuss mailing list >>> caiman-discuss at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >
