[PATCH] Samba: CephFS Snapshots VFS module

Jeremy Allison jra at samba.org
Fri May 10 18:58:41 UTC 2019


On Fri, May 10, 2019 at 03:16:01PM +0200, David Disseldorp wrote:
> On Wed, 8 May 2019 15:47:40 -0700, Jeremy Allison via samba-technical wrote:
> 
> > On Fri, Mar 29, 2019 at 06:45:31PM +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.
> > > 
> > > Feedback appreciated.  
> > 
> > Mostly looks good - a few comments inline below. Hope you don't think
> > I'm being too picky, push back if so. I really want this functionality, just
> > want to make sure I can maintain it going forward.
> 
> Thanks for the review, Jeremy. I've attached a V2 patchset with the
> changes below squashed in...

A couple of comments left (sorry) - in:

+static int ceph_snap_gmt_convert(struct vfs_handle_struct *handle,
+                                    const char *name,
+                                    time_t timestamp,
+                                    char *_converted_buf,
+                                    size_t buflen)

You have:

+       /*
+        * found snapshot via parent. Append the child path component
+        * that was trimmed... +1 for path separator.
+        */
+       if (strlen(_converted_buf) + 1 + strlen(trimmed) >= buflen) {
+               return -EINVAL;
+       }
+       strncat(_converted_buf, "/", buflen);
+       strncat(_converted_buf, trimmed, buflen);

strncat is potentially dangerous here as it doesn't zero-terminate
by default if there's no space. I'd be much happier with strlcat instead.

Having said that, and looking at the arithmetic carefully I *think* it's
safe as you exit on >= buflen. But I had to think
about it carefully in the review. I don't want
other people to have to do that :-).

Can you change the comment to be:

+       /*
+        * found snapshot via parent. Append the child path component
+        * that was trimmed... +1 for path separator + 1 for null termination.
+        */
+       if (strlen(_converted_buf) + 1 + strlen(trimmed) + 1 > buflen) {
+               return -EINVAL;
+       }

Just to use the expected idion of '>' rather than the rarer
'>=' when checking string overruns.

So the result would be:

+       /*
+        * found snapshot via parent. Append the child path component
+        * that was trimmed... +1 for path separator + 1 for null termination.
+        */
+       if (strlen(_converted_buf) + 1 + strlen(trimmed) + 1 > buflen) {
+               return -EINVAL;
+       }
+       strlcat(_converted_buf, "/", buflen);
+       strlcat(_converted_buf, trimmed, buflen);

Second comment - in ceph_snap_gmt_opendir() you do:

+       dir = SMB_VFS_NEXT_OPENDIR(handle, conv_smb_fname, mask, attr);
+       saved_errno = errno;
+       TALLOC_FREE(conv_smb_fname);
+       errno = saved_errno;
+       return dir;

- NB, you're saving errno and restoring over the TALLOC_FREE(conv_smb_fname);
I think that's the right thing to do (you never know
if TALLOC_FREE might do a syscall to overwrite errno).

I think you also need to do this in:

ceph_snap_gmt_unlink()
ceph_snap_gmt_chmod()
ceph_snap_gmt_chown()
ceph_snap_gmt_chdir()
ceph_snap_gmt_ntimes()
ceph_snap_gmt_readlink()
ceph_snap_gmt_mknod()
ceph_snap_gmt_realpath()
ceph_snap_gmt_mkdir()
ceph_snap_gmt_rmdir()
ceph_snap_gmt_chflags()
ceph_snap_gmt_getxattr()
ceph_snap_gmt_listxattr()
ceph_snap_gmt_removexattr()
ceph_snap_gmt_setxattr()
ceph_snap_gmt_disk_free()
ceph_snap_gmt_get_quota()

for consistency.

Sorry for being picky, but I think we're getting there !

Jeremy.



More information about the samba-technical mailing list