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.
