[PATCH] Samba: CephFS Snapshots VFS module

David Disseldorp ddiss at samba.org
Fri May 3 11:49:10 UTC 2019


On Thu, 2 May 2019 14:09:48 -0700, Jeremy Allison wrote:

> On Wed, May 01, 2019 at 11:20:47PM +0200, David Disseldorp via samba-technical wrote:
> > On Fri, 29 Mar 2019 18:45:31 +0100, David Disseldorp via samba-technical wrote:
> >   
> > > The attached patchset adds a new ceph_snapshots Samba VFS module which
> > > handles snapshot enumeration and timewarp/@GMT token mapping.
> > > 
> > > This patchset depends on a new CephFS virtual xattr to obtain the
> > > snapshot creation time, which is pending merge:
> > > https://tracker.ceph.com/issues/38838  
> > 
> > The Ceph changes have been merged upstream, so I'd like to proceed with
> > this patchset in it's current state.
> > 
> > Review/push appreciated.  
> 
> Quick question during review.
> 
> There are a several uses of:
> 
> +       char snaps_path[PATH_MAX + 1];
> ..
> +       ret = snprintf(snaps_path, sizeof(snaps_path), "%s/%s",
> +                      parent_dir, snapdir);
> 
> You have a talloc context available (or talloc_tos()),
> isn't it easier to just do a
> 
> char *snaps_path = NULL;
> snaps_path = talloc_asprintf(mem_ctx,
> 			"%s/%s",
> 			parent_dir,
> 			snapdir);
> 
> and then TALLOC_FREE after use ?
> 
> I'm not really worried about optimization here
> (unless you've got the numbers), just that
> talloc_asprintf() seems to be the idiom we
> commonly use inside source3/smbd/open.c to
> construct pathnames.
> 
> Not really a criticism, just wondered if
> this was a concious decision here ?

Thanks for the feedback Jeremy.
As these uses are just temporary buffers I decided to use the stack.
I don't really have a strong preference here, though I'd normally like
to avoid talloc_tos().

Cheers, David



More information about the samba-technical mailing list