RE: [PATCH v2 02/10] exfat: add exfat_get_empty_dentry_set() helper

["Sungjong Seo" <sj1557.seo@xxxxxxxxxxx>]

> Wataru.Aoyama@xxxxxxxx
> Subject: RE: [PATCH v2 02/10] exfat: add exfat_get_empty_dentry_set()
> helper
> 
> > From: Sungjong Seo <sj1557.seo@xxxxxxxxxxx>
> > Sent: Monday, March 4, 2024 4:43 PM
> > To: Mo, Yuezhang <Yuezhang.Mo@xxxxxxxx>; linkinjeon@xxxxxxxxxx
> >
> > >
> > > The following code still exists if without this patch set. It does
> > > not allow deleted dentries to follow unused dentries.
> >
> > It may be the same part as the code you mentioned, but remember that
> > the first if-statement handles both an unused dentry and a deleted
> > dentry together.
> 
> Thanks for your detailed explanation.
> 
> I will update the code as follows, and I think it is very necessary to add
> such comments.
I think so :)

> 
>         for (i = 0; i < es->num_entries; i++) {
>                 ep = exfat_get_dentry_cached(es, i);
>                 if (ep->type == EXFAT_UNUSED)
>                         unused_hit = true;
>                 else if (IS_EXFAT_DELETED(ep->type)) {
>                         /*
>                          * Although it violates the specification for a
>                          * deleted entry to follow an unused entry, some
>                          * exFAT implementations could work like this.
>                          * Therefore, to improve compatibility, allow it.
>                          */
>                         if (unused_hit)
However, I don't think it's good idea to check for unused_hit here.
How about moving the comment at the start of the for-loop and changing
the code as follows?

/*
 * ONLY UNUSED OR DELETED DENTRIES ARE ALLOWED:
 * Although it violates the specification for a deleted entry to
 * follow an unused entry, some exFAT implementations could work
 * like this. Therefore, to improve compatibility, let's allow it.
 */
 for (i = 0; i < es->num_entries; i++) {
         ep = exfat_get_dentry_cached(es, i);
         if (ep->type == EXFAT_UNUSED) {
                 unused_hit = true;
                 continue;
         }
         if (IS_EXFAT_DELETED(ep->type))
                 continue;
         if (unused_hit)
                 goto err_used_follow_unused;
         i++;
         goto count_skip_entries;
 }

Or we could use if / else-if as follows.

for (i = 0; i < es->num_entries; i++) {
        ep = exfat_get_dentry_cached(es, i);
        if (ep->type == EXFAT_UNUSED) {
                unused_hit = true;
        } else if (!IS_EXFAT_DELETED(ep->type)) {
                if (unused_hit)
                        goto err_used_follow_unused;
                i++;
                goto count_skip_entries;
        }
}

>                                 continue;
>                 } else {
>                         /* Used entry are not allowed to follow unused
entry */
>                         if (unused_hit)
>                                 goto err_used_follow_unused;
> 
>                         i++;
>                         goto count_skip_entries;
>                 }
>         }