On 2/14/24 08:37, Bill O'Donnell wrote:
> On Wed, Feb 14, 2024 at 07:31:09AM +0800, Ian Kent wrote:
>> On 12/2/24 20:12, Ian Kent wrote:
>>> On 12/2/24 09:13, Damien Le Moal wrote:
>>>> On 2/11/24 12:36, Ian Kent wrote:
>>>>>>> +static void zonefs_free_fc(struct fs_context *fc)
>>>>>>> +{
>>>>>>> + struct zonefs_context *ctx = fc->fs_private;
>>>>>> I do not think you need this variable.
>>>>> That's a fair comment but it says fs_private contains the fs context
>>>>>
>>>>> for the casual reader.
>>>>>
>>>>>>> +
>>>>>>> + kfree(ctx);
>>>>>> Is it safe to not set fc->fs_private to NULL ?
>>>>> I think it's been safe to call kfree() with a NULL argument for ages.
>>>> That I know, which is why I asked if *not* setting fc->fs_private to
>>>> NULL after
>>>> the kfree is safe. Because if another call to kfree for that pointer
>>>> is done, we
>>>> will endup with a double free oops. But as long as the mount API
>>>> guarantees that
>>>> it will not happen, then OK.
>>>
>>> Interesting point, TBH I hadn't thought about it.
>>>
>>>
>>> Given that, as far as I have seen, VFS struct private fields leave the
>>>
>>> setting and freeing of them to the file system so I assumed that, seeing
>>>
>>> this done in other mount api implementations, including ones written by
>>>
>>> the mount api author, it was the same as other VFS cases.
>>>
>>>
>>> But it's not too hard to check.
>>
>> As I thought, the context private data field is delegated to the file
>> system.
>>
>> The usage here is as expected by the VFS.
>
> Thanks for the reviews. I submitted a v2 patch.
> Cheers-
> Bill
Testing was all good, so I applied this to for-6.9 branch.
I did tweak the patch to re-add the local ctx variable in zonefs_free_fc() since
that seems to be the accepted pattern.
Thanks !
--
Damien Le Moal
Western Digital Research
