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