[Ocfs2-tools-devel] [PATCH 07/15] dx_dirs v11: more library support for directory indexing

[Coly Li]

On 04/26/2010 11:00 AM, Tao Ma Wrote:
> Hi Coly,
> 
> Coly Li wrote:
>>
>> On 04/26/2010 09:27 AM, Tao Ma Wrote:
>>> Hi Coly,
>>>
>>> Coly Li wrote:
>>>> On 04/23/2010 10:26 PM, Tao Ma Wrote:
>>>>> Hi coly,
>>>>>     the 2nd part of review for this really large patch. ;)
>>>>>
>>>>> Coly Li wrote:
>>>>>> -static void ocfs2_swap_dx_entry_list_to_cpu(struct
>>>>>> ocfs2_dx_entry_list *dl_list)
>>>>>> +static void ocfs2_swap_dx_entry_list(struct ocfs2_dx_entry_list
>>>>>> *dl_list)
>>>>>>  {
>>>>>>      int i;
>>>>>>  
>>>>>> -    if (cpu_is_little_endian)
>>>>>> -        return;
>>>>>> -
>>>>>>      dl_list->de_count    = bswap_16(dl_list->de_count);
>>>>>>      dl_list->de_num_used    = bswap_16(dl_list->de_num_used);
>>>>>>  
>>>>>>      for (i = 0; i < dl_list->de_count; i++)
>>>>>> -        ocfs2_swap_dx_entry_to_cpu(&dl_list->de_entries[i]);
>>>>>> +        ocfs2_swap_dx_entry(&dl_list->de_entries[i]);
>>>>>> +}
>>>>>> +
>>>>>> +static void ocfs2_swap_dx_entry_list_to_cpu(struct
>>>>>> ocfs2_dx_entry_list *dl_list)
>>>>>> +{
>>>>>> +    if (cpu_is_little_endian)
>>>>>> +        return;
>>>>>> +    ocfs2_swap_dx_entry_list(dl_list);
>>>>>> +}
>>>>>> +
>>>>>> +static void ocfs2_swap_dx_entry_list_from_cpu(struct
>>>>>> ocfs2_dx_entry_list *dl_list)
>>>>>> +{
>>>>>> +    if (cpu_is_little_endian)
>>>>>> +        return;
>>>>>> +    ocfs2_swap_dx_entry_list(dl_list);
>>>>>>  }
>>>>> Hey, why do you change the original one? Your code is buggy now. When
>>>> I change the original one, just want to avoid another
>>>> ocfs2_swap_dx_entry_from_cpu().
>>>>
>>>>> swap_entry_list_to_cpu you need to swap de_count/de_num_used first.
>>>>> While if we want to swap_entry_list_from_cpu we neet to swap the
>>>>> dx_entry first.
>>>> It should be safe to call ocfs2_swap_dx_entry_list() for both to_cpu
>>>> and from_cpu. And the code is tested, it works here.
>>> Do you test in a big endian platform like ppc? We should have a problem
>>> there.
>>
>> No, I don't have a big endian machine to test.
>> Could you please to explain more, how it's buggy for current code ? I
>> don't catch your mind yet.
> OK, so the function is:
> static void ocfs2_swap_dx_entry_list(struct ocfs2_dx_entry_list *dl_list)
> {
>         int i;
> 
>         dl_list->de_count       = bswap_16(dl_list->de_count);
>         dl_list->de_num_used    = bswap_16(dl_list->de_num_used);
> 
>         for (i = 0; i < dl_list->de_count; i++)
>                 ocfs2_swap_dx_entry(&dl_list->de_entries[i]);
> }
> 
> When swapping to the cpu, it is good since we swap de_count first.
> But when we swap from the cpu, the de_count is swapped first.
> So after this line
>         dl_list->de_count       = bswap_16(dl_list->de_count);
> 
> the de_count isn't correct any more since in a big endian platform, you
> have swapped the 'de_count' to a le16 value which is going to be stored
> in the disk. That means the following line
>         for (i = 0; i < dl_list->de_count; i++)
> You use the wrong de_count.

Very clear explanation. Yeah, here is a bug, which my test didn't cover.
Nice catch! The fix will be posted soon :-)


-- 
Coly Li
SuSE Labs