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

[Anand Avati]

On May 3, 2013, at 10:41 AM, Jeremy Allison <jra at samba.org> 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) * 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.

OK.

>> +		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 :-).

OK.

>> +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 !


This is a debugging relic left over when I was getting spurious "dev/ino" mismatch in smbd.log. Will remove it in next submission.

Is it important for the st_ex_dev to be "stable" across reboots and across various smbds in clustered samba (for ctdb)?

Thanks for the review

Avati