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... > 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. -- Jens Axboe
