HUGE! Thanks for your support and patients! Pushing.
Joe Ethan Quach wrote: > Joe, > > Looks good :-) > > thanks > -ethan > > > Joseph J VLcek wrote: >> The webrev has been updated. >> http://cr.opensolaris.org/~joev/bug7807/ >> >> Retested as previously described. >> >> Please let me know if this is acceptable. >> >> Thank you. Joe >> >> >> >> >> Joseph J VLcek wrote: >>> Ethan Quach wrote: >>>> Joe, >>>> >>>> This is just a nit - since a trailing slash is appended to >>>> $dirname_target at line 224, then for cases where the >>>> code enters the else at 232, wouldn't you be setting >>>> $target with a value with a trailing slash again? >>>> >>>> Looks like it work if you just removed the chunk from >>>> 222-225 all together, and change 230 to be: >>>> >>>> target="${dirname_target}/${basename_target}" >>> >>> >>> This will result in //a returning //a >>> >>> But as we discussed on the phone the flow of the logic is confusing. >>> I will rework it to reflect our discussion. >>> >>> Thank you! >>> >>> Joe >>> >>>> >>>> >>>> thanks, >>>> -ethan >>>> >>>> >>>> Joseph J VLcek wrote: >>>>> Ethan Quach wrote: >>>>>> Hi Joe, >>>>>> >>>>>> The changes look okay, but seems like it implies that the >>>>>> target_directory argument passed into create-service >>>>>> cannot be the root dir, "/". >>>>>> >>>>>> Either this should be a) allowed, if its legal, or b) checked >>>>>> as a special case, and told to the user with a specific error >>>>>> message. As it stands now, it seems we simply spit out the >>>>>> usage when "/" is passed in, and that doesn't really tell the >>>>>> user what's going on if its not legal. >>>>>> >>>>>> >>>>>> thanks, >>>>>> -ethan >>>>>> >>>>>> >>>>>> Joseph J VLcek wrote: >>>>>>> Could I please get a code review for a small change to fix bug >>>>>>> 7807 ? >>>>>>> >>>>>>> Bug: >>>>>>> ----- >>>>>>> 7807 installadm create-service crashes with no args >>>>>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=7807 >>>>>>> >>>>>>> >>>>>>> Webrev: >>>>>>> ------- >>>>>>> http://cr.opensolaris.org/~joev/bug7807/ >>>>>>> >>>>>>> >>>>>>> The modules affected and tested: >>>>>>> -------------------------------- >>>>>>> installadm >>>>>>> >>>>>>> Testing done: >>>>>>> -------------- >>>>>>> >>>>>>> 1. >>>>>>> I built a stand alone C program to exercise the changed function, >>>>>>> strip_ending_slashes. The stand along program passed various >>>>>>> strings including NULL and the empty string to strip_ending_slashes. >>>>>>> >>>>>>> 2. >>>>>>> I built and ran installadm create-service on x86 with both no >>>>>>> arguments and valid arguments. >>>>>>> >>>>>>> >>>>>>> >>>>>>> Thank you! >>>>>>> >>>>>>> Joe >>>>>>> _______________________________________________ >>>>>>> caiman-discuss mailing list >>>>>>> caiman-discuss at opensolaris.org >>>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>>> >>>>> >>>>> Please review the updated webrev for bug 7807. >>>>> >>>>> Updated webrev: >>>>> --------------- >>>>> http://cr.opensolaris.org/~joev/bug7807/ >>>>> >>>>> After discussing this with Ethan I am taking a different approach >>>>> to resolve this. >>>>> >>>>> I am reworking the solution to bug 3773 - installadm output ln link >>>>> error message >>>>> >>>>> Bug 3773 inadvertently lead to this bug. >>>>> >>>>> The solution is to not to define the new function >>>>> strip_ending_slashes() in installadm_utils.c >>>>> >>>>> Instead setup-image.sh will have code to remove consecutive and >>>>> trailing slashes from the directory specified by the user. >>>>> >>>>> I have tested the new logic, being added to setup-image.sh, using >>>>> the attached test script. >>>>> >>>>> I have also tested using installadm as I had done for the initial >>>>> code review request. >>>>> >>>>> Thank you! Joe. >>>>> >>>>> >>>>> >>>>> >>>>> >>> >>> _______________________________________________ >>> caiman-discuss mailing list >>> caiman-discuss at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>
