Hi Evan.
On 03/11/09 09:49, Evan Layton wrote:
> Joseph J. VLcek wrote:
>> Evan Layton wrote:
>>> Joseph J. VLcek wrote:
>>>> Evan Layton wrote:
>>>>> I need to get a review the following simple fix.
>>>>>
>>>>> This problem was caused by the inappropriate use of strncmp() in
>>>>> my fix for 3332. Because of the way strncmp() was being used we
>>>>> end up comparing only the length of the currently active BE name.
>>>>> What this causes is if the current BE is named say snv_108 and the
>>>>> BE we are attempting to rename is called snv_108-1 we only check
>>>>> the length of the current BE and of course it says these are the
>>>>> same so the rename fails. The fix in this case is to simply
>>>>> replace the strncmp() with strcmp().
>>>>>
>>>>> 7269 beadm rename can fail.
>>>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=7269
>>>>>
>>>>> Webrev:
>>>>> http://cr.opensolaris.org/~evanl/7269/
>>>>>
>>>>> Thanks!
>>>>> -evan
>>>>> _______________________________________________
>>>>> caiman-discuss mailing list
>>>>> caiman-discuss at opensolaris.org
>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>>>>
>>>> I have a safer suggestion:
>>>>
>>>> Maybe it would be safest to continue using strncmp with the length
>>>> of the longest string passed as "n" (lenght).
>>>>
>>>>
>>>>
>>>> e.g.:
>>>> #define MAX(a, b) ((a) > (b) ? (a) : (b))
>>>>
>>>> ...
>>>> if (strncmp(bt.obe_name, cbt.obe_name,
>>>> MAX(strlen(bt.obe_name), strlen(cbt.obe_name))) == 0) {
>>>>
>>>> ...
>>>>
>>>> What do you think? Would that be safer or unnecessary?
>>>>
>>>> Joe
>>>>
>>>
>>> I thought of something like this as well be decided against it sine
>>> the name of the currently active BE is already known to be a valid
>>> name it's length is fine. If the name we are attempting to use is
>>> too long it won't matter because we'll check it against the current
>>> BE and it will at most stop checking at the end of the currently
>>> active BE name. If they don't match the new BE name is checked for
>>> validity just after this check.
>>>
>>> I can certainly add this check for max length but I didn't think it
>>> was necessary.
>>>
>>> -evan
>> Fair enough.
>
> And then I changed my mind and did what you suggested anyway. :-)
>
> Webrev is updated.
I believe your original fix is clearer and more efficient.
Here's why:
The original code is this:
if (strncmp(bt.obe_name, cbt.obe_name,
strlen(cbt.obe_name)) == 0) {
which will compare up to as many characters in cbt.obe_name. If
strlen(cbt.obe_name) <= strlen(be.obe_name), this will work just like
strcmp, which is what you would want in this case. However, if
cbt.obe_name is longer but all characters being compared are the same in
both strings, it will errantly say the strings are equal, ignoring the
unscrutinized characters of cbt.obe_name.
I think what you want is to account for those other unscrutinized
characters. strcmp of the original fix will do this.
I would have thought the new fix
if (strncmp(bt.obe_name, cbt.obe_name,
MAX(strlen(bt.obe_name), strlen(cbt.obe_name))) == 0) {
would act differently than it does... I thought it would act like the
original code, but I wrote a test program to try it out.
It turns out that if you only check for zero/non-zero values from
strncmp, it's the same (zero/non-zero value) as strcmp. (See my test
program attached and output below.)
s1 = abc
s2 = abcdefg
strncmp(s1, s2, strlen(s1)) = 0
strncmp(s1, s2, strlen(s2)) = -1
strncmp(s1, s2, MAX(strlen(s1), strlen(s2))) = -1
strncmp(s2, s1, MAX(strlen(s1), strlen(s2))) = 1
strcmp(s1, s2) = -100
strcmp(s2, s1) = 100
So, bottom line, strcmp is much more efficient and clear.
Thanks,
Jack
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: f.c
URL:
<http://mail.opensolaris.org/pipermail/caiman-discuss/attachments/20090311/1592c3b0/attachment.c>
