Re: [PATCH v7 2/5] fs: split off setxattr_copy and do_setxattr function from setxattr

[Linus Torvalds <torvalds@linux-foundation.org>]

On Thu, Dec 23, 2021 at 11:57 AM Stefan Roesch <shr@fb.com> wrote:
>
> +       /* Attribute name */
> +       char *kname;
> +       int kname_sz;

I still don't like this.

Clearly the "just embed the kname in the context" didn't work, but I
hate how this adds that "pointer and size", when the size really
should be part of the type.

The patch takes what used to be a fixed size, and turns it into
something we pass along as an argument - for no actual good reason.
The 'size' isn't even the size of the name, it's literally the size of
the allocation that has a fixed definition.

Can we perhaps do it another way, by just encoding the size in the
type itself - but keeping it as a pointer.

We have a fixed size for attribute names, so maybe we can do

        struct xattr_name {
                char name[XATTR_NAME_MAX + 1];
        };

and actually use that.

Because I don't see that kname_sz is ever validly anything else, and
ever has any actual value to be passed around?

Maybe some day we'd actually make that "xattr_name" structure also
have the actual length of the name in it, but that would still *not*
be the size of the allocation.

I think it's actively misleading to have "kname_sz' that isn't
actually the size of the name, but I also think it's stupid to have a
variable for what is a constant value.

             Linus