On 02/ 3/11 12:57 PM, John Fischer wrote:
Keith,
Thanks. My testing was incorrect. I thought that I had tested a new
client with an old
service with a manifest when in reality I tested a new client with a
new service that had
a manifest. Ugh. So I didn't catch that the code had a huge bug in
it. Totally embarrassing.
Anyhow, thanks for pointing out the known_criteria issue below. Other
responses in-line.
I'll update the webrev when I get things working correctly.
Thanks,
John
On 02/ 3/11 11:30 AM, Keith Mitchell wrote:
Hi John,
A few questions:
In ai_get_manifest.py, the signature for ai_get_http_file has
*nv_pairs. Based on the usage, that should simply be "nv_pairs"
without the asterisk (and the code below should replace "nv_pairs[0]"
with "nv_pairs").
Sure. Old code just being used from before but that makes it much
cleaner.
ai_get_requested_criteria_list: The comment on lines 730-745 should
probably be moved up to this function instead, to be more clear.
Beyond that, what if the criteria file was hand-edited by an admin
and has extra spaces, e.g., <Criteria Name="abc" />? (Or
is the XML normalized at some point before this?) Additionally, the
docstring mentions that it returns 0 for success, and -1 for failure,
but the function never directly returns either of those. (probably
best to just return the required_criteria list and omit the return
value)
Right. I've moved the comment up to the function that actually does
the "parsing" if you
can call it that. You are correct that this doesn't really parse the
file correctly. This part
of the code was sucked back into the tree from before my AI webserver
changes yesterday.
Essentially, it is doing what it did before. It really needs to be
replaced with an XML
parser.
With that said. The criteria file is not actually coming from a
system administrator directly.
It is coming from the webserver. It can only get into the webserver
if it has passed through
the installadm add-manifest command. Thus it has been parsed for
correctness at that
end. So we are relatively safe here.
I corrected the docstring.
Parsed and validated isn't quite the same as "normalized." i.e.,
<Criteria Name="abc" /> is valid XML and would parse
correctly, but would not get caught by the given regex. So unless the
extra spaces are normalized at some point prior as a result of XML
processing, there's potential for latent bugs. However, it's very likely
that the XML gets normalized before that point. If it's how the code was
before, and the bug to replace with XML parsing is resolved quickly
enough, I'm not as concerned.
- Keith
753-755: ai_criteria_known is not defined in this scope - should this
be "known_criteria"?
yep. That is the testing issue.
752 - 758: Pythonization suggestion:
for criteria in criteria_required:
if known_criteria.get(criteria, None):
ai_crit_response[criteria] = known_criteria[criteria]
That does look nicer.
- Keith
On 02/ 3/11 10:05 AM, John Fischer wrote:
All and Sue (specifically for the AI_database change),
After I delivered the AI webserver design project Mary found that
the new ISOs could
not get a manifest from build 157 and 158 cherrypy webservers. The
failure was because
a new bug fix went into the AI_database.py code that was not
anticipated.
The webrev is located at:
http://cr.opensolaris.org/~johnfisc/7016997-get-manifest/
The solution is 2 fold. First, the database criteria list needs to
be checked within the
findManifest() function prior to it being used. Secondly, the
ai_get_manifest.py compatibility
code needs to be enhanced to do the handshaking it used to do prior
to the protocol
changes. Essentially, the compatibility re-introduces the handshake
code.
Thanks,
John
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
