Clay Baenziger wrote: > I changed it as SUNWpython-lxml appears available, and looked to be the > convention used by CherryPy, which I had not realized when we discussed > Do you have a package SUNWpython-lxml? How do we get the package? > it earlier. > Thank you, > Clay > > On Wed, 15 Oct 2008, Sundar Yamunachari wrote: > > >> Clay Baenziger wrote: >> >>> Hi Sundar, >>> To allow delivery and merge with your changes from Monday in >>> installadm, I've pulled it out of this delivery. >>> >> Do you have a bug filed for installadm integration? >> You didn't answer this question. You removed the installadm integration changes. We need it make the installadm work. When will it be done? Is there a bug filed to track the installadm changes?
Thanks, Sundar >>> Looking at setup-service.sh I see on line 46 the path being >>> /usr/sbin/installadm/webserver (no .py) >>> I've got in the depend file the following additions: >>> P SUNWsqlite3 SQLite3 >>> P SUNWpython-cherrypy >>> P SUNWpython-lxml >>> >> Is it supposed to SUNWlxml-cython? or you have changed it to >> SUNWpython-lxml? >> >> Thanks, >> Sundar >> >>> If you would like to see more see the webrev at >>> http://cr.opensolaris.org/~clayb/AI_server/webrev_final_rc2/. >>> >>> Thank you, >>> Clay >>> >>> On Wed, 15 Oct 2008, Sundar Yamunachari wrote: >>> >>> >>>> Clay, >>>> You need to do the webrev comparing your code the slim-gate at >>>> hg.opensolaris.org/hg/caiman/slim_source and not against my workspace. >>>> >>>> >>>> *installadm.c*: >>>> >>>> The code for the installadm.c in the slim-gate is changed and that will >>>> affect your code. Please take the latest code and make your changes. >>>> >>>> 634, 804, 846 : In the case of error, you may have to display a proper >>>> error message before exiting. The error messages are defined in >>>> installadm.h and you have to add specific error messages to suit your >>>> needs. >>>> >>>> *setup-service.sh*: >>>> >>>> 46: AIWEBSERVER is defined as webserver.py. Please change it to webserver >>>> since the binary delivered is 'webserver' and not 'webserver.py' >>>> >>>> *SUNWinstalladm-tools:* >>>> >>>> The SUNWinstalladm-tools/depend file is missing. SUWNpython-Cherry, >>>> SUNWsqlite3 and package for lxml are needed for your ai webserver and >>>> publish manifest to work. Please add them to the package depend file. >>>> >>>> Thanks, >>>> Sundar >>>> >>>> >>>> Clay Baenziger wrote: >>>> >>>>> Hi Dave, >>>>> I've fixed the webrev, you'll find the newest one (with the head >>>>> merged at http://cr.opensolaris.org/~clayb/AI_server/webrev_final_rc2. >>>>> I've filed the following bugs to address comments not addressed in the >>>>> code at this time: >>>>> Bug 3953 Need protocol in A/I webserver/client >>>>> Bug 3954 Should limit queue sizes for programs interacting with >>>>> AI_database >>>>> Bug 3955 Please use sanity in setting and getting variables from classes >>>>> >>>>> Please let me know what you think. >>>>> Thank you, >>>>> Clay >>>>> >>>>> On Wed, 15 Oct 2008, Dave Miner wrote: >>>>> >>>>> >>>>>> Clay Baenziger wrote: >>>>>> >>>>>>> Hi Dave, >>>>>>> Thank you very much for taking the time to code review this! My my >>>>>>> please let me know if I'm okay to push as soon as you can. My current >>>>>>> webrev is at: >>>>>>> http://cr.opensolaris.org/~clayb/AI_server/webrev_final_rc0/. >>>>>>> >>>>>>> Thank you, >>>>>>> Clay >>>>>>> >>>>>>> >>>>>> I started to re-review, but stopped, as there are significant >>>>>> discrepancies between your responses and what appears in the webrev, so >>>>>> I think there was an error in generating the webrev. Additionally, this >>>>>> needs to be merged against the current gate, proper bug numbers added to >>>>>> the commit comments, and a crying need to run cstyle. Please do all of >>>>>> these things before asking for another review, we won't get to closure >>>>>> without them. >>>>>> >>>>>> Some additional comments on your responses in-line below, and comments >>>>>> at the end on pieces I hadn't seen before. >>>>>> >>>>>> >>>>>>> On Thu, 9 Oct 2008, Dave Miner wrote: >>>>>>> >>>>>>> >>>>>>>> General stylistic issues: >>>>>>>> >>>>>>>> - inconsistent with the existing python code in the project, which >>>>>>>> uses >>>>>>>> 8 character indent. >>>>>>>> >>>>>>> PEP 008 says to use 4 spaces, as a recommendation by the Python >>>>>>> community >>>>>>> and I see some continuation indents in the distro constructor use 4 >>>>>>> spaces, however, most of the code uses tabs instead of spaces. Again >>>>>>> the >>>>>>> PEP (and -t/-tt flags to Python) strongly discourage mixing tabs and >>>>>>> spaces. I'd like to defer to the Python community's recommendations >>>>>>> here; >>>>>>> but have implemented the standard of tabs and 4 space continuation >>>>>>> indents >>>>>>> which the DC and other AI Python code follows. >>>>>>> >>>>>>> >>>>>> Establishing style is a collective decision, so you should work with the >>>>>> others on the team to sort out a recommendation for style going forward. >>>>>> I have no problem using the PEP standards, but I'm more interested in >>>>>> consistency among our own code. I also continue to see a lack of >>>>>> consistency in what you've don here; continuations are all over the map >>>>>> still in AI_database.py, for example. >>>>>> >>>>>> >>>>>>>> - use more vertical whitespace to separate stanzas within functions >>>>>>>> and >>>>>>>> methods to improve readability >>>>>>>> >>>>>>> I've tried to add some for readability's sake. >>>>>>> >>>>>>> >>>>>> I'd like much more. Long functions such as findManifest, getCriteria, >>>>>> etc., need to be much more readable than they are. >>>>>> >>>>>> >>>>>>>> - much more commenting, especially of algorithms and assumptions. My >>>>>>>> feedback would likely be more extensive if I better understood the >>>>>>>> theory of operation in many of these functions. >>>>>>>> >>>>>>> After realizing I was misguided to have been told to tone my comments >>>>>>> back >>>>>>> I went through commenting >>>>>>> >>>>>>> >>>>>>>> AI_database.py >>>>>>>> >>>>>>>> 95: "needs" >>>>>>>> >>>>>>> Thank you >>>>>>> >>>>>>> >>>>>>>> 104, 125: I don't understand the perceived advantage in combining both >>>>>>>> set and get in a single method, which is generally undesirable. This >>>>>>>> would be much clearer as separate methods, which is basically how it's >>>>>>>> written, anyway. >>>>>>>> >>>>>>> The difference between response and finished are for if success and we >>>>>>> have a response, or an error condition. They could probably be >>>>>>> condensed >>>>>>> with a better more wholistic approach. I don't see the advantage to >>>>>>> breaking them out to be a set_response and get_response method though. >>>>>>> It >>>>>>> seems to be a more traditional way of coding it up, versus a way using >>>>>>> Python's interesting control flow abilities. (Jack persuaded me that >>>>>>> using >>>>>>> set and get methods would be advantageous for being able to init things >>>>>>> like _ans in the class initializer.) I think this can be done in an RFE >>>>>>> as it's a maintainability enhancement as I perceive it. >>>>>>> >>>>>>> >>>>>> Bluntly, I don't regard maintainability issues as RFE's. I don't see >>>>>> any "interesting control flow abilities" here; the same thing could be >>>>>> done (equally badly) in any language I've used. Using presence or >>>>>> absence of an argument to completely switch the sense of a method from >>>>>> get to set is bad design anywhere. >>>>>> >>>>>> >>>>>>>> 137: is there a need to use timeouts to prevent the service from being >>>>>>>> unresponsive? >>>>>>>> >>>>>>> If we don't get a database response it won't be gracefully handled and >>>>>>> CherryPy will spawn a new thread per request (or publish_manifest will >>>>>>> get >>>>>>> killed by the user). Though, to prevent stuck processes just hanging >>>>>>> out >>>>>>> forever, I did change this to 15 seconds. >>>>>>> >>>>>>> >>>>>> A comment as to why 15 seconds would be helpful. >>>>>> >>>>>> >>>>>>>> 166: please put errors on stderr >>>>>>>> >>>>>>> Ah yes; I would agree, but CherryPy doesn't seem to pass STDERR. I >>>>>>> should >>>>>>> be using a more robust logging interface, but for the prototype there >>>>>>> simply isn't time. I've taken that out of the comment, however, as this >>>>>>> is >>>>>>> a bigger problem. The print function shouldn't be used anywhere where >>>>>>> it >>>>>>> shouldn't be replaced with a logging subsystem. >>>>>>> >>>>>>> >>>>>> I need to see an RFE filed for that. Karen can probably help you with >>>>>> logging since she just overhauled this in DC. >>>>>> >>>>>> >>>>>>>> 170: a comment as to why IMMEDIATE would be useful >>>>>>>> >>>>>>> I added: >>>>>>> # use SQLite IMMEDIATE isolation to prevent any writers changing >>>>>>> # the DB while we are working on it (but don't use EXCLUSIVE since >>>>>>> # there may be persistent readers) >>>>>>> >>>>>>> >>>>>>>> 184,190: there's no error information we can pass on from the >>>>>>>> execute() >>>>>>>> that would help diagnose the problem? >>>>>>>> >>>>>>> Hi Dave, I'm not sure but some exceptions raised will pass an error >>>>>>> string >>>>>>> in. I've modified the function to pass that error string in case it's >>>>>>> of >>>>>>> use. >>>>>>> >>>>>>> >>>>>>>> 232ff: please add whitespace between comment delimiter and comment >>>>>>>> >>>>>>> Thank you >>>>>>> >>>>>>> >>>>>>>> 238-end: lots of inconsistent (and excessive) continuation indents to >>>>>>>> clean up. this applies to other files and I won't repeat for them. >>>>>>>> >>>>>>> I've been trying to indent to the last parenthesis of the block above, >>>>>>> but now follow pretty strictly using a 4 space indent per Jack's >>>>>>> explanation of the standard. >>>>>>> >>>>>>> >>>>>> As noted above, need to get more strict. >>>>>> >>>>>> at line 250: "instance" >>>>>> >>>>>> >>>>>>>> 272,312: a comment here seems necessary >>>>>>>> >>>>>>> I have commented the use of the SQL HEX() function in >>>>>>> getSpecificCriteria() and thoroughly commented getCriteria() >>>>>>> >>>>>>> >>>>>> I see no comment related to HEX() in the webrev. >>>>>> >>>>>> >>>>>>>> 353: a boolean flag would be preferable to an indirect test on length >>>>>>>> of >>>>>>>> a string >>>>>>>> >>>>>>> I could test off getCriteria.next() != None instead of the string >>>>>>> length. >>>>>>> It's equivalent to my intention of if queryStr != "SELECT " (as it was >>>>>>> set >>>>>>> on 346). I've put in the test against getCriteria for now. >>>>>>> >>>>>>> >>>>>> My comment was against the line: >>>>>> 353 if len(queryStr) > 7: >>>>>> >>>>>> which still remains, now as 361 in the new webrev. >>>>>> >>>>>> >>>>>>>> 371: you're not providing the default here, that would be at a higher >>>>>>>> level, whose behavior shouldn't be assumed here >>>>>>>> >>>>>>> Thank you for catching the layering error. I've changed the comment >>>>>>> appropriately. >>>>>>> >>>>>>> >>>>>> I still see the original comment in this webrev. >>>>>> >>>>>> 399,410: the error messages here still are a layering violation >>>>>> >>>>>> >>>>>>>> webserver.py >>>>>>>> >>>>>>>> general comment here is that some versioning of the protocol should be >>>>>>>> considered, perhaps something like the IPS scheme >>>>>>>> >>>>>>> I think that's a good RFE that should be filed and done before this >>>>>>> hits >>>>>>> 1.0 >>>>>>> >>>>>>> >>>>>> It's a bug, actually. Please file it. >>>>>> >>>>>> this file also continues to need more vertical spacing in the larger >>>>>> functions. >>>>>> >>>>>> >>>>>>>> 174ff: excessively indented continuation here. >>>>>>>> >>>>>>> Yes, I was trying to align with the abspath, but shouldn't have for the >>>>>>> serve_files options. >>>>>>> >>>>>>> >>>>>>>> 177,187: perhaps we should define a specific x- mime type for our >>>>>>>> usage? >>>>>>>> >>>>>>> The download is useful for admins testing with a web browser so that it >>>>>>> doesn't try to interpret it. I don't know if they would try downloading >>>>>>> the file were it a different type? Plus we intend to download this file >>>>>>> with the A/I client so it seems to make sense. Perhaps, I'm a bit >>>>>>> confused >>>>>>> what advantages a specific MIME type could buy us though? >>>>>>> >>>>>>> >>>>>> I think it would help to validate that you aren't directed to a server >>>>>> (perhaps due to admin mis-configuration) that's providing something >>>>>> other than what you're expecting. Any unknown x- type would seemingly >>>>>> provide the same web browser advantages you get from x-download. >>>>>> >>>>>> >>>>>>>> 192: so what are we attempting to return here? >>>>>>>> >>>>>>> This is the XML-ish criteria list for the AI client to know what's >>>>>>> requested of it. However, perhaps splitting this up wouldn't be a bad >>>>>>> idea. Also, this shouldn't be testing against the postData string, but >>>>>>> using cherrypy handlers for POST versus GET (as per >>>>>>> http://www.cherrypy.org/wiki/RequestObject). >>>>>>> >>>>>>> I've added a comment for now >>>>>>> >>>>>>> >>>>>> Again, I don't see this comment in the posted webrev. >>>>>> >>>>>> >>>>>>>> 246: seems like a questionable redirection to me (218 seems a little >>>>>>>> less odd, though still somewhat questionable). A 404 seems more >>>>>>>> appropriate for what looks like a malformed request. >>>>>>>> >>>>>>> I agree for 246. I've changed not to a 404, but a 403 with the message >>>>>>> "Index listing denied". >>>>>>> >>>>>>> >>>>>>>> 269,271: these should be configurable >>>>>>>> >>>>>>> I have made these command line options >>>>>>> >>>>>>> >>>>>>>> Comments below are against revised version from >>>>>>>> http://cr.opensolaris.org/~clayb/AI_server/webrev1/ >>>>>>>> >>>>>>>> publish_manifest.py >>>>>>>> >>>>>>>> Will this support publishing via non-CLI interfaces as presently >>>>>>>> structured? >>>>>>>> >>>>>>> A U/I can import publish_manifest and the much of the work in >>>>>>> parseOptions() can be done by the U/I, but the main logic of publishing >>>>>>> the manifest would be in publish_manifest not the U/I. >>>>>>> >>>>>>> >>>>>>>> 1: missing CDDL >>>>>>>> >>>>>>> CDDL'ized >>>>>>> >>>>>>> >>>>>>>> 132,143,159,169: so what Python would that be? Is it one we actually >>>>>>>> need to run on? >>>>>>>> >>>>>>> The thought was that A/I could be independent of Solaris at some point >>>>>>> (i.e. run on Linux/OS X or Windows). It seems IEEE754 support in Python >>>>>>> is a bit unstable and I preferred to play it safe for now. >>>>>>> >>>>>>> >>>>>>>> 135: Assuming we do actually need this, I don't really see the point >>>>>>>> in >>>>>>>> a separate upper-bound value for MAC vs. non-MAC. It's just a big >>>>>>>> number. >>>>>>>> >>>>>>> Good point I don't either. >>>>>>> >>>>>>> >>>>>>>> 156ff: need much more commenting here about what's happening >>>>>>>> >>>>>>> Yes, checkCriteria() is a scary, scary function. I have broken it up to >>>>>>> make it much more approachable. >>>>>>> >>>>>>> >>>>>>>> 214: "determine" >>>>>>>> >>>>>>> Thank you, I'll remember to aspell(1) more often >>>>>>> >>>>>>> >>>>>>>> 303: not obvious why they must be the same; if so, why would we bother >>>>>>>> copying in the new one? >>>>>>>> >>>>>>> We don't have a way to support having two manifests with the same name, >>>>>>> so >>>>>>> we warn the user that there's a difference but that the criteria was >>>>>>> still >>>>>>> added to the database already. There was a bug, we shouldn't copy the >>>>>>> new manifest in if it's already (same or not). >>>>>>> >>>>>>> >>>>>>>> 307ff: how about doing this join() once and not 5 times? >>>>>>>> >>>>>>> Yes, this does clean up the code nicely. >>>>>>> >>>>>>> >>>>>>>> 401: this comment doesn't appear to match the code >>>>>>>> >>>>>>> My intention for the comment matches the code but it's unclear. I >>>>>>> change >>>>>>> the comment and simplified it >>>>>>> >>>>>>> >>>>>>>> 440: comment here, please >>>>>>>> >>>>>>> I changed tag to a more descriptive name and added comments >>>>>>> >>>>>>> >>>>>>>> 539: "separate" >>>>>>>> >>>>>>> Yes late night my spelling goes down hill >>>>>>> >>>>>>> >>>>>>>> 561: "validation" >>>>>>>> >>>>>>> A lot... >>>>>>> >>>>>>> >>>>>>>> verifyXML.py >>>>>>>> >>>>>>>> 1: missing CDDL >>>>>>>> >>>>>>> CDDL'ized >>>>>>> >>>>>>> >>>>>>>> 38: "receive" >>>>>>>> >>>>>>> Zzz... Zzz... Type... Type... Zzz... Zzz.. Thank you again for the idea >>>>>>> to >>>>>>> run aspell against my code >>>>>>> >>>>>>> >>>>>>>> Dave >>>>>>>> >>>>>>>> >>>>>> So what's the AI.db file? >>>>>> >>>>>> usr/src/cmd/Makefile: >>>>>> 36: Alphabetize lists such as this. This also hasn't been merged >>>>>> against the current version yet. >>>>>> >>>>>> usr/src/cmd/ai-webserver/Makefile: >>>>>> continuations in Makefiles conventionally are indented to align with the >>>>>> item they're continuing, and we don't spread content that fits in a >>>>>> single line across a continuation. Consult existing Makefiles for >>>>>> stylistic examples. Copying existing practice is much preferred over >>>>>> making up your own. >>>>>> >>>>>> 95 et al: 744 is a most unusual mode. 755 for executables, please, >>>>>> though in general we just don't bother with setting modes in these rules >>>>>> and do so on the install phase. >>>>>> >>>>>> default.xml >>>>>> why do we set swap size in the default manifest? Can't we use the >>>>>> algorithms in the orchestrator to compute this automatically in the >>>>>> default case? >>>>>> >>>>>> SUNWinstalladm-tools/prototype_com: >>>>>> I don't think the schema files belong in /var; something in /usr/share, >>>>>> perhaps, seems more appropriate. This would affect Makefile.cmd and >>>>>> Targetdirs. >>>>>> >>>>>> installadm.c: >>>>>> >>>>>> There are so many cstyle errors here I hardly know where to begin, but >>>>>> there are incorrect continuation indents in all of the new code. The >>>>>> rules are simple: 4 space indent on a continuation line, period. Style >>>>>> of switch statements is also incorrect: cases are to be at the same >>>>>> indent level as the switch. Please review a copy of the C style >>>>>> guidelines from http://www.opensolaris.org/os/community/on/, and run >>>>>> cstyle on your code before sending for review. >>>>>> >>>>>> 782,815: incorrect usage of MAXPATHLEN. These aren't path names, so use >>>>>> a constant specific to your usage. >>>>>> >>>>>> >>>>>> Dave >>>>>> _______________________________________________ >>>>>> 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 > -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.opensolaris.org/pipermail/caiman-discuss/attachments/20081015/5dbb2642/attachment.html>
