[PATCH] Samba: CephFS Snapshots VFS module

[David Disseldorp]

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...

> > +static int ceph_snap_fill_label(struct vfs_handle_struct *handle,
> > +				TALLOC_CTX *tmp_ctx,
> > +				const char *parent_snapsdir,
> > +				const char *subdir,
> > +				char *this_label)  
> 
> There is a typedef char SHADOW_COPY_LABEL[25] which
> described 'this_label'. Can you use that instead of
> char *, otherwise the
> 
> memset(this_label, 0, sizeof(SHADOW_COPY_LABEL));
> 
> below looks weird and potentially overflow'y
> (I know it isn't, but using SHADOW_COPY_LABEL
> makes that clear).

Done. FWIW I've dropped the memset now that it's done alongside
allocation.

...
> > +static int ceph_snap_enum_snapdir(struct vfs_handle_struct *handle,
> > +				  struct smb_filename *snaps_dname,
> > +				  bool labels,
> > +				  struct shadow_copy_data *sc_data)
> > +{
> > +	NTSTATUS status;
> > +	int ret;
> > +	DIR *d = NULL;
> > +	struct dirent *e = NULL;
> > +	int slots;  
> 
> slots should be unsigned, or a size_t.

Changed to uint32_t.

...
> > +	for (e = SMB_VFS_NEXT_READDIR(handle, d, NULL);
> > +	     e != NULL;
> > +	     e = SMB_VFS_NEXT_READDIR(handle, d, NULL)) {
> > +		char *this_label;
> > +
> > +		if (ISDOT(e->d_name) || ISDOTDOT(e->d_name)) {
> > +			continue;
> > +		}
> > +		sc_data->num_volumes++;
> > +		if (!labels) {
> > +			continue;
> > +		}
> > +		if (sc_data->num_volumes > slots) {
> > +			slots += 10;  
> 
> Can you do an overflow check here ?
> 
> Yes I know it's not possible. I'm still paranoid :-).

Added.

> 
> > +			DBG_DEBUG("%d slots for enum_snaps response\n", slots);
> > +			sc_data->labels = talloc_realloc(sc_data,
> > +							  sc_data->labels,
> > +							  SHADOW_COPY_LABEL,
> > +							  slots);
> > +			if (sc_data->labels == NULL) {
> > +				ret = -ENOMEM;
> > +				goto err_closedir;
> > +			}  
> 
> Should you zero-fill the 10 new slots here ? Not strictly needed
> I think. Your call here.

Makes sense. I prefer zero-fill alongside allocation - added.

...
> > +static int ceph_snap_get_parent_path(const char *connectpath,
> > +				     const char *path,
> > +				     char *_parent_buf,
> > +				     size_t buflen,
> > +				     const char **_trimmed)
> > +{
> > +	const char *p;
> > +	int len;  
> 
> len should be size_t I think. Below, do the assert the p >= path
> instead of len >= 0.

Fixed.

...
> > +/*
> > + * XXX play discard_const() games with const smb_filename structs, to avoid
> > + * allocation of a new struct just for this.
> > + */  
> 
> Can you do an allocation instead ? I really hate discard_const_p()
> tricks, eventually they bite :-). Isn't it possible a real const char
> is getting passed inside these smb_filenames ?
> 
> I don't think these are performance critical code paths.
> 
> Can't you change ceph_snap_gmt_strip_snapshot() to return
> a talloc'ed struct smb_filename from a passed in const one ?
> 
> Pass in a talloc context of talloc_tos() and remember to free
> in exit paths. That's what it's for (sorry I know you hate it,
> but it's appropriate here IMHO of course).

I've changed the const smb_filename handlers to use cp_smb_filename()
to avoid the discard_const ugliness. The copy only occurs if a @GMT
token is present, so it indeed shouldn't be performance critical.

Cheers, David
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vfs_ceph_snapshots_v2.patchset
Type: text/x-patch
Size: 55979 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20190510/cf2db189/vfs_ceph_snapshots_v2.bin>