Hi Clay.
Clay Baenziger wrote:
> Hi Jack,
> Looking at webrev2 your hg comment is repeated, please only fix
> the bug once :) Comments in-line, but it looks like we've hashed
> things nearly to completion. I'm happy the user experience should be
> nicely improved here for 0906.
>
Thanks again for your reviews. More inline from me below as well...
publish-manifest.py:
>> The original code had a string of 9's: "9999999...." to represent
>> infinity and "0" (as opposed to 0) to represent the lower bound.
>> What I've done keeps the same data types for bounds as were present
>> before.
>
> Okay, it's goofy (I should have used a number format not a string) I
> believe Python deals appropriately. If you've checked ranges (i.e. a
> manifest in the database has range a-b and you try to add a manifest
> with range a+1 - b-1 and it errors then I'm okay).
First I established a reference point: a good installation start using
the default manifest; I verified things were fine. Then I added my
custom manifest with a criteria out of range. (I used a MAC address
which was out of range.) Result: it kept the default. This is expected.
Then I started over with a new service and again verified that it
started with the default. I added a custom manifest but with the MAC
address which WAS in range, and it said it would use that manifest
instead. (ai_manifest.xml and ai_combined_manifest.xml in /tmp
reflected use of my custom manifest.) This shows my fix works during an
install.
>>>>
>>>> While doing this, I noticed comment references of "-inf". I
>>>> changed these references to 0, since that's what the code used
>>>> before I started working on this bug, and what it still uses. If
>>>> you think it should be "-inf" instead then I'll file a bug to fix
>>>> this later. Please let me know.
>>>
>>> I think the comments(?) which had -inf in them are more accurate. As
>>> a negative number would still be okay. (Though we don't have a
>>> criteria at the moment where a negative number would make sense --
>>> and I can't come up with such a case -- but the code currently works
>>> that way.)
>> I respectfully disagree. The code has "0" for a lower bound (see
>> publish-manifest.py line 205 in slim_source or line 207 in my changed
>> version), which excludes negative numbers, and the comments reflect
>> this. If we change the code, then we can change the comments. I'd
>> like to leave things as they are.
>>
>> For now, though, I suggest we keep this code as it is since, as you
>> say, there are no criteria where negative numbers make sense. I'll
>> file a bug to document that negative-valued criteria should be
>> accommodated. I don't want to implement that now though, as it is
>> too risky and too late.
>
> Okay, this isn't a regression, however, the database uses a NULL which
> would work for a negative value (so range detection may be broken for
> negative values).
Filed 8012 AI manifest criteria ranges don't handle negative values
Feel free to claim it if you want it.
>
>>>>> *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?
>>>> All "unbounded"s are already stored as lower case. The conversion
>>>> is done in verifyXML.py, here at lines 250-254:
>>>>
>>>> # Handle "unbounded" keyword; and
>>>> pass lowercase
>>>> lowered_value = one_value.lower()
>>>> if (lowered_value == "unbounded"):
>>>> new_values += lowered_value
>>>
>>> Ah, okay, I had seen that and not thought on it much further.
>>> Perhaps a comment referring to where the case processing is done
>>> would be good?
>> I changed the comment to say that "unbounded"s are already converted
>> to lower case during manifest processing.
>>
>> In general, I shy away from referencing a particular file in the
>> comment of another file, since that introduces a maintainability
>> issue. If someone changes the former file, they probably wouldn't
>> know to change the comment in the latter file, making the comment
>> incorrect and confusing.
>
> Yes, I just feel if someone changes the lower case check in the other
> file, it'd be good to know that somewhere upstream it was once done if
> it stops being done. (Perhaps, just that it's expected to come in
> lower-case.)
The comment says that a lower-case conversion has been done (and can
thus be assumed).
>
Looks like I'm done. I'll push Thursday after I get back.
Thanks for all your help.
Jack
>>>>>>
>>>>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=4325
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.opensolaris.org/~schwartz/090402.1/webrev/
>>>>>>
