Jack,
The fix looks OK to me.
thanks,
-ethan
On 09/19/11 11:31, Jack Schwartz wrote:
Hi Darren and Sue.
On 09/19/11 06:26 AM, Darren Kenny wrote:
Looks cleaner alright :)
With respect to Sue's comments on testing for None, it would be more
correct to
say that we're "testing for None or empty string" with that test. If
it was just
None we were testing for then "is None" would be more correct in the
if statement.
I should make one more small change, to restore allowing an empty
string for a value. Empty string paths would still be rejected.
I have updated the code for both set and add to look like this:
# Explicitly test for a None value as etree accepts it and we
do not.
- # Test other values for consistency.
- if not path or not value:
+ # "" value is OK. A None or "" path is not accepted.
+ if not path or value is None:
raise milib.MimInvalidError(milib.ERR_ARG_INVALID)
Delta webrev with just this change is here:
https://cr.opensolaris.org/action/browse/caiman/schwartz/7090604_4_3/webrev/
Please let me know if you have any questions/comments about this.
Thanks,
Jack
Thanks,
Darren.
On 16/09/2011 23:05, Jack Schwartz wrote:
Hi everyone.
During the review period I came to find a flaw in the code, and had
to revise
some of it. Please see the bug report for details. This review
accounts for
this plus all other relevant code review comments previously received.
Hopefully this is the last update to this code review. I have
retested this
code and it now works for the cases it failed before, as well as for
the ones
which passed. See below for testing info. It is also 15 lines
simpler than before.
webrev vs slim_source:
https://cr.opensolaris.org/action/browse/caiman/schwartz/7090604_3
webrev vs previous webrev:
https://cr.opensolaris.org/action/browse/caiman/schwartz/7090604_3_2
I would like to ask Ethan and Darren to re-review this code as they
are most
familiar with it at this point, and for Dave to bless it so that it
gets in
before 175 closes.
Thanks,
Jack
P.S. Note: due to curtailment of new messages at this point, I could
only reuse
the error messages I already had. The new method in process_dtd.py
could use a
better one, and I will be filing a bug to fix this later once this
wad is
approved for push.
P.P.S. New passing "aimanifest add" cases with the following paths:
disk
- Errs due to ambiguity of possible parent paths
disk/disk_name@name
- Errs due to ambiguity of possible parent paths
target
- Succeeds when manifest doesn't already have a target.
- Errs when manifest already has a target.
target/disk/disk_name@name
- succeeds. Adds new disk each time
zvol@action
- succeeds. Adds a new zvol to the target/logical each time
badname
- Errs out
disk/badname
- Errs out
I also regression tested other cases which used to work, including:
- simple absolute cases such as:
/auto_install/ai_instance/software/source/publisher/origin
- non-simple absolute cases such as:
/auto_install/ai_instance[@name=default]/software/software_data[@action=install]/name
- non-simple, non-absolute cases such as:
ai_instance[@name=default]/software/software_data[@action=install]/name
software/software_data[@action=install]/name
On 09/14/11 07:59 PM, Jack Schwartz wrote:
Hi everyone.
Seeking two reviews by Thursday COB and Dave's approval please.
I'd like to
request Ethan be one of the reviewers.
Here is a code review that fixes a hole in aimanifest path processing.
Without this bugfix,
aimanifest add diskname@name c0t0d0s0
or other references to non-absolute paths which have no values in
them fails.
webrev:
https://cr.opensolaris.org/action/browse/caiman/schwartz/7090604_1
bug report:
http://monaco.us.oracle.com/detail.jsf?cr=7090604
While it introduces a new method to find the path leading to the
referenced
element, all code introduced by this fix affects only the failing
cases;
codepaths of working cases are not affected.*
* While in there, I added some additional args checking for set(),
get() and
add(), which I have tested but can remove if it makes the
difference between
being allowed in or not.
Thanks for your time,
Jack
_______________________________________________
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
