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 >
