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