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

[Stefan Roesch <shr@fb.com>]

On 12/23/21 12:11 PM, Linus Torvalds wrote:
> 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
> 

Linus, I added the xattr_name struct and removed the kname_sz field from
the xattr_ctx struct. In addition the xattr_name struct is used in xattr.c
and io_uring.c.