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

[Anand Avati]

On 04/25/2013 02:32 AM, Volker Lendecke wrote:

> > +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);
>
> Is this correct? Shouldn't that be
>
> tmp = realloc (glfd_fd, (glfd_fd_size + 1024) * sizeof(glfs_fd_t *));

Doh! you're right. This wouldn't even show up as a problem until you 
have more than 128 (or 256 on 32bit) concurrently open fds. Thanks for 
catching :-)

> > +static glfs_fd_t *
> > +glfd_fd_get (int i)
> > +{
> > +       return glfd_fd[i];
>
> I'd feel better with a size check here.

Ack.

> > +}
> > +
> > +static glfs_fd_t *
> > +glfd_fd_clear (int i)
> > +{
> > +       glfs_fd_t *glfd = glfd_fd[i];
>
> Same here for the size check.

Ack.

> > +
> > +       glfd_fd[i] = 0;
> > +       glfd_fd_used--;
> > +       return glfd;
> > +}
> > +
> > +
> > +/* Helper to convert stat to stat_ex */
> > +
> > +static void
> > +smb_stat_ex_from_stat (struct stat_ex *dst, const struct stat *src)
> > +{
> > +       memset (dst, 0, sizeof (*dst));
>
> More Samba-like would be ZERO_STRUCTP(dst).


OK.

> > +static struct dirent *
> > +vfs_gluster_readdir (struct vfs_handle_struct *handle, DIR *dirp,
> > +                    SMB_STRUCT_STAT *sbuf)
> > +{
> > +       static char direntbuf[512];
> > +       int ret;
> > +       struct stat stat;
> > +       struct dirent *dirent = 0;
> > +
> > +       ret = glfs_readdirplus_r ((void *)dirp,&stat, (void *)direntbuf,
> > +                               &dirent);
> > +       if (ret)
> > +               dirent = NULL;
> > +
> > +       if (sbuf)
> > +               smb_stat_ex_from_stat (sbuf,&stat);
>
> Do you initialize the stat buf even in case of an error?

We don't. This would result in garbage getting written into sbuf 
"safely" (without crashing, without expectation of sbuf having any 
expected values). But I will fix it anyways.

Thanks for the review!
Avati