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 >>
