Looks ok to me. - Keith
On 03/15/10 02:48 PM, Clay.Baenziger at Oracle.COM wrote: > Hi Keith, > My apologies for the wonky merge. I think the webrevs should be > fixed up now; same URLs: > Differential webrev: > http://cr.opensolaris.org/~clayb/11510/webrev4.diff/ > > Full webrev: > http://cr.opensolaris.org/~clayb/11510/webrev4/ > > Thank you, > Clay > > On Mon, 15 Mar 2010, Keith Mitchell wrote: > >> The following comments are based on the differential webrev, although >> it appears that the differential webrev has a few oddities related to >> merging (e.g., the find_TFTP_root changes show up in the diff). >> >> delete_service.py lines 207, 539, 565, 664, 711: >> Were these reverted intentionally? >> 525 & 528: Print statements need cleaning >> >> 778: Not sure what's going on with this comment. >> >> - Keith >> >> On 03/12/10 06:43 PM, Clay.Baenziger at Oracle.COM wrote: >>> Hi Keith, >>> I've made the correction you suggest as certainly it make much >>> more sense for the file objects to implement these functions but >>> I've also fixed bug 15175 "installadm delete-service can errantly >>> error about boot archives". So, I think you have roughly 6 lines of >>> code to please check and I'll finally get this off our plates. >>> >>> Differential webrev: >>> http://cr.opensolaris.org/~clayb/11510/webrev4.diff/ >>> >>> Full webrev: >>> http://cr.opensolaris.org/~clayb/11510/webrev4/ >>> Thank you, >>> Clay >>> >>> >>> On Wed, 3 Mar 2010, Keith Mitchell wrote: >>> >>>> Hi Clay, >>>> >>>> Short version - the changes look good! (A few compliments and >>>> thank-you's below) >>>> >>>> One last question though: Seems like "is_readable" and >>>> "is_writeable" belong on the File_ and StringIO_ classes? (With >>>> FileMethods possibly implementing intelligent "defaults", i.e. >>>> writable returning False and readable returning True) >>>> >>>> (As a stylistic note, since they're properties and not plain >>>> methods, I would have called them "readable" and "writable") >>>> >>>> Thanks, >>>> Keith >>>> >>>> On 03/ 2/10 03:29 PM, Clay Baenziger wrote: >>>>> Hi Keith, >>>>> Thank you for this review! And your ability to remember and >>>>> catch small details is not annoying -- it's a God-send for someone >>>>> like me. My comments in-line. I've fixed your comments and added a >>>>> fix for a bug I found in installadm_common.py when running >>>>> delete-service as well: >>>>> Differential webrev: >>>>> http://cr.opensolaris.org/~clayb/11510/webrev3.diff >>>>> Full Webrev: >>>>> http://cr.opensolaris.org/~clayb/11510/webrev3 >>>>> >>>>> Thank you, >>>>> Clay >>>>> >>>>> On Tue, 2 Mar 2010, Keith Mitchell wrote: >>>>> >>>>>> Hi Clay, >>>>>> >>>>>> I have a few comments about these fixes. >>>>>> >>>>>> First, a meta-question about 14735 - is "SystemExit" truly the >>>>>> appropriate exception to be raising here, given that there's no >>>>>> intent to exit the Python interpreter? I believe SystemExit >>>>>> should only be raised with the intention of exiting the program, >>>>>> and that it should never be caught (unless re-raised). There must >>>>>> be a more appropriate exception to raise - if not, it's trivial >>>>>> to define one as needed. >>>>>> http://docs.python.org/library/exceptions.html#exceptions.SystemExit >>>>> >>>>> I certainly feel SystemExit is more appropriate than SystemError. >>>>> However, the big case here of run_cmd() returning SystemExit >>>>> should issue its own exception. I have added an explicit call out >>>>> for run_cmd() in 4016 - A/I Python code error architecture should >>>>> be looked at for better reuse (i.e. in a different U/I). I would >>>>> like to avoid massive code changes in this bug due to schedule >>>>> constraints right now. >>>> >>>> Certainly SystemExit is better than SystemError! I definitely >>>> wasn't implying a reversal of the fix :) Adding the cases in >>>> question to bug 4016 seems like an appropriate action, given the >>>> time constraints. I do wonder at what point we can get away from >>>> SystemExit though. Perhaps I'll put my foot down a bit more after >>>> this release... >>>> >>>>> >>>>>> create_client.py: >>>>>> function: parse_options(): optparse.OptionParser defines a >>>>>> function, error(), which prints out a message and usage >>>>>> appropriately (as well as exiting). I think utilizing that >>>>>> functionality may be desired here, instead of manually raising >>>>>> SystemExits. >>>>> >>>>> I have changed to parser.error() thank you for pointing this out. >>>>> >>>>>> 99-100, 145: Please remove the commented out lines >>>>> >>>>> Thank you >>>>> >>>>>> 121: My count may be off, but if you follow PEP8 and remove the >>>>>> spaces around the '=' inside parentheses (keyword arguments >>>>>> shouldn't have the space around the '=' as I recall), then this >>>>>> fits on one line cleanly. >>>>> >>>>> Thank you, I've fixed the parenthesized " = "'s to "=" throughout >>>>> the file. >>>>> >>>>>> 150: Not related to your change, but this should be "...arch == >>>>>> 'SPARC':" instead of 'is' - technically, two equivalent strings >>>>>> don't have to be the same object. >>>>>> (Ditto for line 274) >>>>> >>>>> Yes, relying on the class's hash function is a better way to go in >>>>> the face of later change. >>>>> >>>>>> 160: Nit/PEP8 - No spaces around '=' in keyword args. >>>>> >>>>> Thank you for the reminder >>>>> >>>>>> 295-311: >>>>>> Seems like some of this hoop jumping could be avoided by having >>>>>> separate except clauses for SystemExit and >>>>>> com.AIImage.AIImageError. (The remainder of the hoop jumping >>>>>> would not be needed if we didn't abuse SystemExit instantiation >>>>>> by trying to pass in both a message and an error code in other >>>>>> parts of our code, and instead created a simple sub-class of >>>>>> SystemExit that had room for both if we really need it). >>>>> >>>>> Yes this hoop jumping was to deal with OptionParser exiting. If I >>>>> can let it handle all exit cases, it makes life easier. >>>> >>>> It looks much better now! :) >>>> >>>>> >>>>>> 319: Why was this moved after the information about how to file a >>>>>> bug? >>>>> >>>>> This was done because otherwise the message says to see the >>>>> exception printed below which was actually printed above. >>>> >>>> Thanks, I see that now. I told you I sometimes can't read ;) >>>> >>>>> >>>>>> installadm_common.py: >>>>>> 65: Comment needs an update >>>>> >>>>> Ah thank you for the catch >>>>> >>>>>> Thanks, >>>>>> Keith >>>>>> >>>>>> On 02/26/10 07:05 PM, Clay Baenziger wrote: >>>>>>> Hi John, >>>>>>> Thank you for the feedback! My responses in-line. >>>>>>> New webrev: >>>>>>> http://cr.opensolaris.org/~clayb/11510/webrev2/ >>>>>>> Differential webrev: >>>>>>> http://cr.opensolaris.org/~clayb/11510/webrev2.diff/ >>>>>>> >>>>>>> Thank you, >>>>>>> Clay >>>>>>> >>>>>>> On Wed, 24 Feb 2010, John Fischer wrote: >>>>>>> >>>>>>>> Clay, >>>>>>>> >>>>>>>> Lots of great work here. A few comments/questions follow. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> >>>>>>>> John >>>>>>>> >>>>>>>> >>>>>>>> create_client.py >>>>>>>> It seems that mac_address/options.mac_address could be >>>>>>>> converted to >>>>>>>> a str(com.MACAddress(mac_address/option.mac_address)) once and >>>>>>>> then >>>>>>>> the converted string used within the remainder of the code. >>>>>>>> Either >>>>>>>> in the setup_.... functions or in __main__. >>>>>>> >>>>>>> Good point. I've changed the options parsing code to store the >>>>>>> verified and processed MAC address string. >>>>>>> >>>>>>>> 153 def setup_tftp_links(service_name, image_path, >>>>>>>> mac_address, boot_args = "null"): >>>>>>>> 166 client_id = "01" + str(com.MACAddress(mac_address)) >>>>>>>> >>>>>>>> 175 def setup_sparc_client(image_path, mac_address): >>>>>>>> 186 client_id = "01" + str(com.MACAddress(mac_address)) >>>>>>>> >>>>>>>> 201 def setup_dhcp(mac_address, arch): >>>>>>>> 212 client_id = "01" + str(com.MACAddress(mac_address)) >>>>>>>> >>>>>>>> 267 if options.image.arch is "X86": >>>>>>>> 268 if options.boot_args: >>>>>>>> 269 setup_tftp_links(options.service_name, >>>>>>>> 270 options.image.path, >>>>>>>> 271 str(com.MACAddress(options.mac_address)), >>>>>>>> 272 ",".join(options.boot_args)+",") >>>>>>>> 273 else: >>>>>>>> 274 setup_tftp_links(options.service_name, >>>>>>>> 275 options.image.path, >>>>>>>> 276 str(com.MACAddress(options.mac_address))) >>>>>>>> 277 else: >>>>>>>> 278 setup_sparc_client(options.image.path, >>>>>>>> 279 str(com.MACAddress(options.mac_address))) >>>>>>>> 280 >>>>>>>> 281 >>>>>>>> setup_dhcp(str(com.MACAddress(options.mac_address)), >>>>>>>> 282 options.image.arch) >>>>>>>> >>>>>>>> installadm_common.py >>>>>>>> Why not have the __init__ for class AIImage setup the _arch >>>>>>>> after _path is >>>>>>>> checked/setup? _dir_path is setup in the __init__. >>>>>>> >>>>>>> Though it happens right now we really only care about validation >>>>>>> and architecture, I see no reason to hard code checking the >>>>>>> architecture; it's cached on first hit. As I originally wrote >>>>>>> this code it handled far more in anticipation of create_client >>>>>>> being pythonized. >>>>>>> >>>>>>>> nit Add sun4v to comment >>>>>>>> >>>>>>>> 106 # check if sun4u >>>>>>>> 107 if os.path.isdir(os.path.join(self.path, >>>>>>>> "platform", "sun4u")) or \ >>>>>>>> 108 os.path.isdir(os.path.join(self.path, >>>>>>>> "platform", "sun4v")): >>>>>>>> 109 self._arch = "SPARC" >>>>>>>> 110 return self._arch >>>>>>> >>>>>>> Done >>>>>>> >>>>>>>> nit Add comment since sun4u has comment >>>>>>>> >>>>>>>> 111 if os.path.isdir(os.path.join(self.path, >>>>>>>> "platform", "i86pc")) or \ >>>>>>>> 112 os.path.isdir(os.path.join(self.path, >>>>>>>> "platform", "amd64")): >>>>>>>> 113 self._arch = "X86" >>>>>>>> 114 return self._arch >>>>>>> >>>>>>> Done >>>>>>> >>>>>>>> If line 203 is true then why does line 217 work? >>>>>>>> >>>>>>>> 203 # XXX MRO doesn't seem to like this: line = >>>>>>>> super(self.__class__, self).readline() >>>>>>>> 204 if issubclass(self.__class__, file): >>>>>>>> 205 line = file.readline(self) >>>>>>>> 206 elif issubclass(self.__class__, StringIO.StringIO): >>>>>>>> 207 line = StringIO.StringIO.readline(self) >>>>>>>> 208 else: >>>>>>>> 209 raise AssertionError("Unimplemented readline >>>>>>>> for the ancestor " >>>>>>>> 210 "of %s." % self.__class__) >>>>>>>> 211 while line: >>>>>>>> 212 # apply newline function (either strip "\n" >>>>>>>> or leave as is) >>>>>>>> 213 line = newlineFn(line) >>>>>>>> 214 # loop to the next line if we have a comment >>>>>>>> or blank line and are >>>>>>>> 215 # not returning such lines >>>>>>>> 216 if commentFn(line) or blanklineFn(line): >>>>>>>> 217 line = super(self.__class__, >>>>>>>> self).readline() >>>>>>>> 218 continue >>>>>>>> 219 return line >>>>>>> >>>>>>> This was rather puzzling but Keith turned out to be a star and >>>>>>> figured the secret out, I believe it was: >>>>>>> >>>>>>> At 203, line = super(self.__class__, self).readline() caused the >>>>>>> MRO (method resolution order) to reset for some reason causing >>>>>>> infinite recursion. While after isinstance/issubclass then the >>>>>>> MRO was set so that line 217 worked. >>>>>>> >>>>>>> The right answer, however, is that both super calls should be: >>>>>>> line = super(FileMethods, self).readline() >>>>>>> >>>>>>> This, despite the FileMethods class having no ancestor with a >>>>>>> readline function (but all of its subclasses do). It works >>>>>>> happily and resolves the correct readline with no funky recursion. >>>>>>> >>>>>>>> On 02/23/10 02:01 PM, Clay Baenziger wrote: >>>>>>>>> Hi all, >>>>>>>>> I've written a bit of code to change out the create-client.sh >>>>>>>>> shell >>>>>>>>> script to a Python version. This gets us yet closer to a 100% >>>>>>>>> Python >>>>>>>>> installadm(1). Please let me know what looks rough here; this >>>>>>>>> is only >>>>>>>>> going back to the default branch of slim_source. >>>>>>>>> >>>>>>>>> Bugs: >>>>>>>>> 11510 create-client: accepts a SPARC image path for an X86 >>>>>>>>> service 12159 >>>>>>>>> create-client accepts non-existent service names >>>>>>>>> 14735 delete-service: should use SystemExit instead of >>>>>>>>> SystemError >>>>>>>>> exception >>>>>>>>> >>>>>>>>> Webrev: >>>>>>>>> http://cr.opensolaris.org/~clayb/11510/ >>>>>>>>> >>>>>>>>> Thank you, >>>>>>>>> Clay >>>>>>>>> _______________________________________________ >>>>>>>>> caiman-discuss mailing list >>>>>>>>> caiman-discuss at opensolaris.org >>>>>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>>>>>> >>>>>>> _______________________________________________ >>>>>>> caiman-discuss mailing list >>>>>>> caiman-discuss at opensolaris.org >>>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>>>> >>>> >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>
