[Gluster-devel] [PATCH 3/3] vfs_glusterfs: Samba VFS module for glusterfs

[Jeremy Allison]

On Thu, May 02, 2013 at 02:36:32PM -0400, Anand Avati wrote:
> Implement a Samba VFS plugin for glusterfs based on gluster's gfapi.
> This is a "bottom" vfs plugin (not something to be stacked on top of
> another module), and translates (most) calls into closest actions
> on gfapi.

A few (hopefully helpful :-) comments.

> +glfd_fd_store (glfs_fd_t *glfd)
> +{
> +	int i;
> +	void *tmp;
> +
> +	if (glfd_fd_size == glfd_fd_used) {
> +		tmp = realloc (glfd_fd, (glfd_fd_size + 1024) * sizeof (glfs_fd_t *));

Can you use talloc functions instead of malloc/realloc/free ?
Yes, they are functionally identical (except talloc is a little
slower :-) but even tallocing off the NULL context allows us
to keep track of talloced memory and query it remotely, which
can aid in debugging if there's anything wrong.

> +		if (!tmp) {
> +			errno = ENOMEM;
> +			return -1;
> +		}
> +
> +		glfd_fd = tmp;
> +		memset(glfd_fd+glfd_fd_size, 0, 1024 * sizeof(glfs_fd_t *));
> +		glfd_fd_size += 1024;

A minor nit here. Doing glfd_fd_size += 1024 and then
using it in the realloc later doesn't protect against
integer wrap. Yes I know this is an internal variable,
not accessible to external users, but it's bad programming
practice to not check for integer wrap on anything that's
used for allocation - just as a matter of principle.

(Yeah we probably do it ourselves elsewhere in the code,
but it's still a bad thing and I fix it to add wrap tests
whenever I come across it :-).

> +smb_stat_ex_from_stat (struct stat_ex *dst, const struct stat *src)
> +{
> +	ZERO_STRUCTP(dst);
> +
> +	dst->st_ex_dev = 42;
> +	dst->st_ex_ino = src->st_ino;

Is hard-coding st_ex_dev to 42 a good idea ?
Can you not use a hash of the external volume
name or something similar (I must confess I'm
haven't completely studied how gluster does
this).

Is it possible to have 2 Samba shares which
point at 2 separate gluster shared storage
instances (which are completely separate
backend systems) ? If so then you need to
have a distinct st_ex_dev for each instance.
dev/ino pairs are used as hash keys for tdbs.

Other than these things looks good to me !

Jeremy.