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 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. 99-100, 145: Please remove the commented out lines 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. 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) 160: Nit/PEP8 - No spaces around '=' in keyword args. 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). 319: Why was this moved after the information about how to file a bug? installadm_common.py: 65: Comment needs an update 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
