Looks good, thanks Jack.
On 19/09/2011 19: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 _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
