Jack,
mim.py
421 and 580 You are no longer explicitly testing for None, so comment should be
changed.
(haven't code reviewed this version, but happened to notice the comments)
Sue
On 09/16/11 15: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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
