Re: [PATCH v6 12/16] ntfs: add reparse and ea operations

[Namjae Jeon]

On Tue, Feb 3, 2026 at 3:38 PM Christoph Hellwig <hch@xxxxxx> wrote:
>
> Suggested commit message:
>
> Implement support for Extended Attributes and Reparse Points, enabling
> Posix ACL support and, and compatibility with Windows Subsystem for
> Linux (WSL) metadata.
Okay, I will use it.
>
> > +struct WSL_LINK_REPARSE_DATA {
> > +     __le32  type;
> > +     char    link[];
> > +};
> > +
> > +struct REPARSE_INDEX {                       /* index entry in $Extend/$Reparse */
>
> Why are these using all upper case names unlike the rest of the
> code?
Right, I will change it.
>
> > +     ok = ni && reparse_attr && (size >= sizeof(struct reparse_point)) &&
> > +             (reparse_attr->reparse_tag != IO_REPARSE_TAG_RESERVED_ZERO) &&
> > +             (((size_t)le16_to_cpu(reparse_attr->reparse_data_length) +
> > +               sizeof(struct reparse_point) +
> > +               ((reparse_attr->reparse_tag & IO_REPARSE_TAG_IS_MICROSOFT) ?
> > +                0 : sizeof(struct guid))) == size);
>
> A bunch of superflous braces.  But in general decomposing such complex
> operations into an inline helper using multiple if statements and
> adding comments improves the readability a lot.
Okay, I will update and add the comments.
>
> > +     if (ok) {
>
> ... and just return here for !ok and reduce the indentation for
> the rest of the function?
Right, I will change it like this.
>
> > +             switch (reparse_attr->reparse_tag) {
> > +             case IO_REPARSE_TAG_LX_SYMLINK:
> > +                     wsl_reparse_data = (const struct WSL_LINK_REPARSE_DATA *)
> > +                                             reparse_attr->reparse_data;
> > +                     if ((le16_to_cpu(reparse_attr->reparse_data_length) <=
> > +                          sizeof(wsl_reparse_data->type)) ||
> > +                         (wsl_reparse_data->type != cpu_to_le32(2)))
> > +                             ok = false;
> > +                     break;
> > +             case IO_REPARSE_TAG_AF_UNIX:
> > +             case IO_REPARSE_TAG_LX_FIFO:
> > +             case IO_REPARSE_TAG_LX_CHR:
> > +             case IO_REPARSE_TAG_LX_BLK:
> > +                     if (reparse_attr->reparse_data_length ||
> > +                         !(ni->flags & FILE_ATTRIBUTE_RECALL_ON_OPEN))
> > +                             ok = false;
> > +                     break;
>
> ... and then directly return from inside the switch as well?
Right, I will change it.
Thanks for the review!
>
>