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
