Clay, Looks good.
John 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 >>
