Re: [PATCH] zonefs: convert zonefs to use the new mount api

[Bill O'Donnell <bodonnel@redhat.com>]

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

> 
> 
> Ian
> 
> > 
> > 
> > Ian
> > 
> > > 
> > > > 
> > > > This could be done but so far the convention with mount api code
> > > > 
> > > > appears to have been to add the local variable which I thought was for
> > > > descriptive purposes but it could just be the result of cut and pastes.
> > > Keeping the variable is fine. After all, that is not the fast path :)
> > > 
> > > 
> > 
>