Hi Keith,
My apologies for the wonky merge. I think the webrevs should be
fixed up now; same URLs:
Differential webrev:
http://cr.opensolaris.org/~clayb/11510/webrev4.diff/
Full webrev:
http://cr.opensolaris.org/~clayb/11510/webrev4/
Thank you,
Clay
On Mon, 15 Mar 2010, Keith Mitchell wrote:
> The following comments are based on the differential webrev, although it
> appears that the differential webrev has a few oddities related to merging
> (e.g., the find_TFTP_root changes show up in the diff).
>
> delete_service.py lines 207, 539, 565, 664, 711:
> Were these reverted intentionally?
> 525 & 528: Print statements need cleaning
>
> 778: Not sure what's going on with this comment.
>
> - Keith
>
> On 03/12/10 06:43 PM, Clay.Baenziger at Oracle.COM wrote:
>> Hi Keith,
>> I've made the correction you suggest as certainly it make much more
>> sense for the file objects to implement these functions but I've also fixed
>> bug 15175 "installadm delete-service can errantly error about boot
>> archives". So, I think you have roughly 6 lines of code to please check and
>> I'll finally get this off our plates.
>>
>> Differential webrev:
>> http://cr.opensolaris.org/~clayb/11510/webrev4.diff/
>>
>> Full webrev:
>> http://cr.opensolaris.org/~clayb/11510/webrev4/
>> Thank you,
>> Clay
>>
>>
>> On Wed, 3 Mar 2010, Keith Mitchell wrote:
>>
>>> 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
>>>>>
>>>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>
