Hi Sundar,
To allow delivery and merge with your changes from Monday in
installadm, I've pulled it out of this delivery.
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
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
>>>
>
>
