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.
