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.
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
