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