Jack Schwartz wrote: > 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 >
As per the phone conversations with you and Joe I'm leaving this as is. Thanks! -evan
