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
>
