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