Re: [PATCH 5/9] replace do_setxattr() with saner helpers.

[Jens Axboe <axboe@kernel.dk>]

On 10/1/24 8:08 PM, Al Viro wrote:
> On Tue, Oct 01, 2024 at 07:34:12PM -0600, Jens Axboe wrote:
> 
>>> -retry:
>>> -	ret = filename_lookup(AT_FDCWD, ix->filename, lookup_flags, &path, NULL);
>>> -	if (!ret) {
>>> -		ret = __io_setxattr(req, issue_flags, &path);
>>> -		path_put(&path);
>>> -		if (retry_estale(ret, lookup_flags)) {
>>> -			lookup_flags |= LOOKUP_REVAL;
>>> -			goto retry;
>>> -		}
>>> -	}
>>> -
>>> +	ret = filename_setxattr(AT_FDCWD, ix->filename, LOOKUP_FOLLOW, &ix->ctx);
>>>  	io_xattr_finish(req, ret);
>>>  	return IOU_OK;
>>
>> this looks like it needs an ix->filename = NULL, as
>> filename_{s,g}xattr() drops the reference. The previous internal helper
>> did not, and hence the cleanup always did it. But should work fine if
>> ->filename is just zeroed.
>>
>> Otherwise looks good. I've skimmed the other patches and didn't see
>> anything odd, I'll take a closer look tomorrow.
> 
> Hmm...  I wonder if we would be better off with file{,name}_setxattr()
> doing kvfree(cxt->kvalue) - it makes things easier both on the syscall
> and on io_uring side.
> 
> I've added minimal fixes (zeroing ix->filename after filename_[sg]etxattr())
> to 5/9 and 6/9 *and* added a followup calling conventions change at the end
> of the branch.  See #work.xattr2 in the same tree; FWIW, the followup
> cleanup is below; note that putname(ERR_PTR(-Ewhatever)) is an explicit
> no-op, so there's no need to zero on getname() failures.

Looks good to me, thanks Al!

-- 
Jens Axboe