[U-Boot] [PATCH v2 2/8] fs/fat: introduce new director iterators

[Simon Glass]

Hi Rob,

On 5 September 2017 at 03:54, Rob Clark <robdclark at gmail.com> wrote:
> On Tue, Sep 5, 2017 at 4:56 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Rob,
>>
>> On 3 September 2017 at 00:37, Rob Clark <robdclark at gmail.com> wrote:
>>> Untangle directory traversal into a simple iterator, to replace the
>>> existing multi-purpose do_fat_read_at() + get_dentfromdir().
>>>
>>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>>> ---
>>>  fs/fat/fat.c | 326 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 326 insertions(+)
>>>
>>> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
>>> index e1c0a15dc7..c72d6ca931 100644
>>> --- a/fs/fat/fat.c
>>> +++ b/fs/fat/fat.c
>>> @@ -1245,6 +1245,332 @@ exit:
>>>         return ret;
>>>  }
>>>
>>> +
>>> +/*
>>> + * Directory iterator, to simplify filesystem traversal
>>> + */
>>> +
>>> +typedef struct {
>>
>> Please avoid using a typedef here. It seems like it could just be a struct.
>
> I'm typically not a fan of 'typedef struct' but that seems to be the
> style that fs/fat code was already using, and "when in Rome..."

Sure, but let's move to Venice as it is less dusty.

>
>> Also pleaee add a comment about what the struct is for
>>
>>> +       fsdata    *fsdata;
>>> +       unsigned   cursect;
>>> +       dir_entry *dent;
>>> +       int        remaining;     /* remaining dent's in current cluster */
>>> +       int        last_cluster;
>>> +       int        is_root;
>>> +
>>> +       /* current iterator position values: */
>>> +       char       l_name[VFAT_MAXLEN_BYTES];
>>> +       char       s_name[14];
>>> +       char      *name;          /* l_name if there is one, else s_name */
>>> +
>>> +       u8         block[MAX_CLUSTSIZE] __aligned(ARCH_DMA_MINALIGN);
>>
>> Some members are missing comments.
>>
>> Also I'm not too sure how the alignment works here. I don't see you
>> creating one of these objects in this patch, but could you add a
>> comment to the struct about how to create one? Do you use memalign()?
>
> I was just creating them on the stack normally.. expecting the
> compiler to dtrt because of the __aligned() on block[].

Well I think that works. OK.

Regards,
Simon