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
