On 05/11/2020 20:26, Jens Axboe wrote: > On 11/5/20 1:04 PM, Pavel Begunkov wrote: >> On 05/11/2020 19:37, Jens Axboe wrote: >>> On 11/5/20 7:55 AM, Pavel Begunkov wrote: >>>> On 05/11/2020 14:22, Pavel Begunkov wrote: >>>>> On 05/11/2020 12:36, Dmitry Kadashev wrote: >>>> Hah, basically filename_parentat() returns back the passed in filename if not >>>> an error, so @oldname and @from are aliased, then in the end for retry path >>>> it does. >>>> >>>> ``` >>>> put(from); >>>> goto retry; >>>> ``` >>>> >>>> And continues to use oldname. The same for to/newname. >>>> Looks buggy to me, good catch! >>> >>> How about we just cleanup the return path? We should only put these names >>> when we're done, not for the retry path. Something ala the below - untested, >>> I'll double check, test, and see if it's sane. >> >> Retry should work with a comment below because it uses @oldname >> knowing that it aliases to @from, which still have a refcount, but I >> don't like this implicit ref passing. If someone would change >> filename_parentat() to return a new filename, that would be a nasty >> bug. > > Not a huge fan of how that works either, but I'm not in this to rewrite > namei.c... There are 6 call sites including do_renameat2(), a separate patch would change just ~15-30 lines, doesn't seem like a big rewrite. > >> options I see >> 1. take a reference on old/newname in the beginning. >> >> 2. don't return a filename from filename_parentat(). >> struct filename *name = ...; >> int ret = filename_parentat(name, ...); >> // use @name >> >> 3. (also ugly) >> retry: >> oldname = from; > > Not sure I follow - oldname == from, unless there's an error. Yes, this > depends on filename_parentat() returning oldname or IS_ERR(), but that's > how all the callers currently deal with it. I think it's more explicit but still ugly, let's forget about the third -- Pavel Begunkov
