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