Hi Jack,
Thank you very much for doing this work! And thank you too for all
the cool ideas as we've talked back and forth on these changes. My ideas
are below:
publish-manifest:
*Instead of using 9999... and 0 for infinity and a small value,
talking with Drew Fisher here in BRM, we should be using:
sys.maxint and -sys.maxint-1. I was originally not
sure if this worked on Windows but it'll work on the Unix world
and Windows. So, the hack should go away. (lines 204 & 205 could
go away then)
*line 183 comment nit: "check [that] ranges are valid"
*In many of the checks for unbounded since forcing case is painful
to the user, could you do a value.lower() or
manifestCriteria.lower() before doing the compare?
AI_database.py:
*This is perhaps a nit since it's more of a stylistic and reuse
idea for formatValue(), I'd like to see this take a dictionary of
keys and values, so that it can handle all keys and values and
only process the few it needs opposed to running for every key.
I.e.:
for key in ("mac", "ipv4", "ipv6"...):
for prefix in ('', 'MIN', 'MAX'):
try:
values[prefix+key]...
[...]
continue
except: KeyError
pass
Of course, however you want, as this pseudocode is still kludgy,
but I'd like to see it be more efficient than just one key
per call of the function. (Then, list-manifest.py line 174 could
just have:
criteria=formatValues(criteria.keys(),criteria) and
webserver.py line 124 could just have:
crit=formatValues(crit.keys(),crit), for example.)
*Line 530: Elsewhere MIN or MAC are stripped with
var.replace('MIN', '', 1) though I like the key = key[3:] perhaps
consistency is better? (Feel free to ignore this if you think you
should, since you have more experience than I do in maintaining big
code bases.)
*Line 533: This padding will produce a padding like 0001722024199
where as the padding necessary is by octet 172020024199 (for an
address of 172.20.24.199). Also, I'm not sure why you're padding
values here, since the string is padded in the database already.
Criteria_schema.rng:
*Thank you for setting the style to a more consistent one with how
other schemas have developed in the gate
*Line 55,58 Nit: I find unless a block is really big, making it a
name reference detracts from the schema readability from all the
jumping around. Do you have a tool to expand or collapse them
while reading?
*Lines 54-63: We only support one SC manifest and we need one SC
manifest with an AI manifest, let's take the zeroOrMore tag off
and wrap the AI and SC manifest in an optional tag:
<optional>
<ref name="nm_ai_manifest_src"/>
<ref name="nm_sc_manifest_src"/>
</optional>
verifyXML.py:
*For __checkIPv4 and __checkMAC, it's too bad these don't return
the same error one would get for a malformed MAC or IPv4 were
they in a range. If we used LXML to parse the values they could
by doing something like (for IPv4) -- this would also help when
we need to support IPv6 since they're a lot more complex:
# Load in the schema representation of an IPv4 address
s = StringIO('''\
<element name="a" xmlns="http://relaxng.org/ns/structure/1.0";>
<data type="token">
<param name="pattern">
((25[0-5]|2[0-4][0-9]|[01]?[0-9]?[0-9])\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9]?[0-9])
</param>
</data>
''')
# Parse schema
ipv4_schema=etree.parse(s)
relaxng = lxml.etree.RelaxNG(ipv4_schema)
# from __checkIPv4, values is the IP address in question
data=stringIO(values)
try:
root = lxml.etree.parse(data)
except lxml.etree.XMLSyntaxError, e:
return e.error_log.last_error
[the you'd just need to do the rest of the splitting and padding]
which could be as simple as (in fact this would be preferable
to calling newval += ... even in the current form):
return(''.join([octet.zfill(3) for octet in values.split(".")]))
*Line 161: Nit: Could you hand in criteriaRoot and the AI database
to make the dependencies of this function more obvious when
called?
*Lines 216-220: The schema should have line 146 changed to:
<data type="string"/> from <text/> and then this is redundant and
handled in XML validation
*Line 222-224: This is not necessary if a simple check is put in
publish-manifest (original line numbers 116 and 223) to report a
criteria is not a value criteria or range criteria, respectively.
*Line 225-228: In my testing with a range I can't get more than
two lines to pass the XML validator, is this necessary?
*Line 229-232: This should be handled in the schema, as only a
<range> or <value> tag are allowed?
Otherwise, everything looks reasonable. Let me know what you think
of these observations, as there's a lot of ideas here and you have better
experience than me which should be pursued and which are nits. Please call
me with questions too.
Thank you,
Clay
On Thu, 2 Apr 2009, Jack Schwartz wrote:
> Hi everyone.
>
> Here is a webrev of my proposed fixes for:
> 4325 Better syntactic treatment of IP and MAC address AI criteria
>
> A medium-level description of the changes is in the bug-report:
> http://defect.opensolaris.org/bz/show_bug.cgi?id=4325
>
> Webrev:
> http://cr.opensolaris.org/~schwartz/090402.1/webrev/
>
> Testing done:
>
> Ran publish manifest and verified with webserver:
> - MAC: min/unbounded, IP: unbounded/max, network: min/unbounded,
> mem: single <value> for range, minMAC has single digit btwn colons
> - range with three values: correctly failed validation
> - range with two unbounded values: correctly failed validation
> - range with 0 minimum: correctly passed validation
> - single IP addr given as a <range>: corectly failed validation
> - pair of values given as a <value>: correctly failed validation
> - 2 values for non range criterion, surrounded by <value>: correctly failed
> validation
> - single value for non range criterion, surrounded by <value>: passed
> validation
> - 2 values for non range criterion, surrounded by <range>: correctly failed
> validation
> - Try a combination with a non-numeric value (arch=sparc): worked
> Also verified list-manifests displayed data correctly.
>
> I'm requesting that Clay review, as he's most familiar with the code I've
> changed and he has a heads-up that it's coming. Others are invited to review
> as well.
>
> Tomorrow I'll verify that an install starts as it should, but I have
> confidence it will work as the webserver output looks fine.
>
> Sorry to rush. I'm requesting a review ASAP, by COB tomorrow if possible.
> I'm supposed to be done on Friday, but to give less than 1 day for this
> review is not really realistic.
>
> Thanks,
> Jack
>
